Closed Tonkpils closed 7 years ago
@gdborton I can try to write some tests for the processAssets function but there's no current coverage on them.
@gdborton let me know if there's anything I can do to help get this merged!
@Tonkpils Thanks for the PR! Instead of using the names of the constructors could we import webpack and check for actual plugin instances? How do other libs do this detection?
Instead of using the names of the constructors could we import webpack and check for actual plugin instances?
I'm not sure that comparing two different instances would yield equality. I can test it out and see if it works. We'd also need to bring in webpack as a dependency to pull the plugins out which would require us to specify which versions of webpack are supported though I doubt they would remove/add more sourcemap plugins but it'd still be good to specify it in the package.json.
How do other libs do this detection?
I actually looked around to see what other plugins I could find with similar functionalities but was not able to find something that resembled this. If you have an idea, I'd be happy to take a look at it.
Actually thinking about this further, I don't think this is the correct way to go... By including the plugins individually you're able to provide a test/include/exclude config. https://webpack.js.org/plugins/source-map-dev-tool-plugin/
If we want to check for the presence of the source map plugins we'd also have to track which bundles are not getting filtered through their config.
The best way to go on this might be to include the same sourceMap option that is on the UglifyJSPlugin provided by webpack - https://github.com/webpack-contrib/uglifyjs-webpack-plugin#options
@gdborton I've changed it to your suggestion of using the uglifyJS.sourceMap option and added some tests. Let me know if it looks good, however this would break current functionality of using devtool
. We could maintain backwards compatibility by still checking devtool first but it's up to you if you want to to do that.
Let me know and I'll add it back in
@Tonkpils Thanks for the contribution! I went ahead and altered your commit to move the sourceMap option up, and updated the README. I've got a few more breaking changes in the pipeline, so this won't be released right away.
Thank @gdborton, I had been swamped and wasn't able to give this the attention it deserved. Appreciate you finishing taking over it! 🎉
devtools doesn't allow setting some options that the plugins provide. This checks for the currently available source map plugins in the configured plugins to ensure we use the SourceMapSource instead