Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract shared plugin code and share between rollup and webpack plugins #1013

Closed
BethanyBerkowitz opened this issue Feb 2, 2023 · 4 comments · Fixed by #1211
Closed

Extract shared plugin code and share between rollup and webpack plugins #1013

BethanyBerkowitz opened this issue Feb 2, 2023 · 4 comments · Fixed by #1211
Assignees
Labels
rollup/vite @honeybadger-io/rollup-plugin webpack @honeybadger-io/webpack

Comments

@BethanyBerkowitz
Copy link
Contributor

BethanyBerkowitz commented Feb 2, 2023

The rollup plugin was designed with code that can be shared with the webpack plugin. Extract the shared code and re-work both plugins so they share the core functionality (hbUtils.js and options.js in the rollup package). The only difference between the plugins should be how they hook into the build process and how they extract the necessary data from the bundle.

Make sure that we do not lose any functionality from the webpack plugin in this process. Check carefully -- I thin we should be able to avoid breaking changes and a major version bump.

@BethanyBerkowitz BethanyBerkowitz added rollup/vite @honeybadger-io/rollup-plugin webpack @honeybadger-io/webpack labels Feb 2, 2023
@BethanyBerkowitz BethanyBerkowitz self-assigned this Sep 7, 2023
@BethanyBerkowitz
Copy link
Contributor Author

ignorePaths is currently rollup only, ignoreErrors currently webpack only -- need to decide what to do about this

@BethanyBerkowitz
Copy link
Contributor Author

Currently planning to move webpack plugin from adding options validation errors to the list of build errors in the compilation to just throwing an error if passed bad options. I think this this change is a positive one -- the user is only writing their code to set up the options once, so making an error with the options really obvious is positive. An error like forgetting to pass the apiKey is not going to be something intermittent, it's a total failure of setup, so I think it's correct that it would cause the build to fail completely.

However, I'm not sure if this change could mean a major version update 😬

@subzero10
Copy link
Member

subzero10 commented Sep 28, 2023

ignorePaths is currently rollup only, ignoreErrors currently webpack only -- need to decide what to do about this

What about supporting both?

Currently planning to move webpack plugin from adding options validation errors to the list of build errors in the compilation to just throwing an error if passed bad options. I think this this change is a positive one -- the user is only writing their code to set up the options once, so making an error with the options really obvious is positive. An error like forgetting to pass the apiKey is not going to be something intermittent, it's a total failure of setup, so I think it's correct that it would cause the build to fail completely.

However, I'm not sure if this change could mean a major version update 😬

I'm leaning towards a minor version update, and we could play it extra safe if we implement ignoreErrors to simply log warning messages even if validation fails. What do you think?

@BethanyBerkowitz
Copy link
Contributor Author

ignorePaths is currently rollup only, ignoreErrors currently webpack only -- need to decide what to do about this

What about supporting both?

#1210
#1209

Currently planning to move webpack plugin from adding options validation errors to the list of build errors in the compilation to just throwing an error if passed bad options. I think this this change is a positive one -- the user is only writing their code to set up the options once, so making an error with the options really obvious is positive. An error like forgetting to pass the apiKey is not going to be something intermittent, it's a total failure of setup, so I think it's correct that it would cause the build to fail completely.
However, I'm not sure if this change could mean a major version update 😬

I'm leaning towards a minor version update, and we could play it extra safe if we implement ignoreErrors to simply log warning messages even if validation fails. What do you think?

It turned out this was actually not an issue -- plugin-core will throw an error on a failed upload, but webpack will catch it and put it in the compilation error array. So I don't think this is a major version change 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rollup/vite @honeybadger-io/rollup-plugin webpack @honeybadger-io/webpack
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants