Add explicit sourceMap option.#23
Conversation
1 similar comment
|
@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? |
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.
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 Let me know and I'll add it back in |
| function processAssets(compilation, options) { | ||
| const assetHash = compilation.assets; | ||
| const useSourceMaps = !!compilation.options.devtool; | ||
| const useSourceMaps = !!(options.uglifyJS && options.uglifyJS.sourceMap); |
There was a problem hiding this comment.
Can you move this up to the options object? Uglify2 doesn't use this option, and v3 does but it expects an object. I'd like for this to remain a true/false boolean on the options object, that way the uglifyJS object can continue to be passed straight to uglify.
Also, could you add an entry to the README for sourceMap?
There was a problem hiding this comment.
Sure, I'll get to it soon 👍
This makes more sense as users may be directly using SourceMap plugins rather than relying on the devtool setting. User's might also want to enable source maps for some sections of their code but not others. This change enables them to do that by using two instances of this plugin.
|
@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. |
1 similar comment
gdborton
changed the title
|
Thank @gdborton, I had been swamped and wasn't able to give this the attention it deserved. Appreciate you finishing taking over it! 🎉 |
3 participants
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