feat: support devtool (options.sourceMap)
#81
Conversation
…toons.devtool value
hulkish
requested review from
michael-ciniawsky,
bebraw,
alexander-akait and
joshwiens
test/all-options.test.js
Outdated
| expect(errors).toMatchSnapshot('devtool === "sourcemap" errors'); | ||
| expect(warnings).toMatchSnapshot('devtool === "sourcemap" warnings'); | ||
|
|
||
| for (const file in stats.compilation.assets) { |
There was a problem hiding this comment.
const assets = stats.compilation.assets
Object.keys(assets).forEach((asset) => {
const source = assets[asset].source()
expect(source).toMatchSnapshot(asset)
})There was a problem hiding this comment.
will do..but i dont think we want we return in that forEach
There was a problem hiding this comment.
Yup we don't want toreturn there, updated 😛
| @@ -157,6 +157,10 @@ class UglifyJsPlugin { | |||
| uglifyOptions.output = uglifyOptions.output || {}; | |||
| } | |||
|
|
|||
| if (typeof this.options.sourceMap === 'undefined' && ['sourcemap', 'source-map', 'hidden-source-map'].indexOf(compiler.options.devtool) > -1) { | |||
There was a problem hiding this comment.
Is the typeof this.options.sourceMap === 'undefined' necessary here ?
There was a problem hiding this comment.
Yes, if we're talking current usage. Because it can be user defined as true or false. This basically only auto-sets the sourceMap option value if u aren't manually overriding it.
This way, we aren't breaking current usage of sourceMap option
There was a problem hiding this comment.
If devtool should be automatically supported, the previous this.options.sourceMap 'doesn't matter' as it is overridden to true anyways in case compilation.options.devtool is set ? Maybe I'm missing a case here 🙃
There was a problem hiding this comment.
And if I missed one if (typeof this.options.sourceMap === "undefined") => if (!this.options.sourceMap) ?
There was a problem hiding this comment.
well, honestly I'm not certain of the history of this option.. I was just being careful to not break it's current usage.
That said, i see your point and im alright with removing the exposing of this option all together. Up to u.
There was a problem hiding this comment.
@michael-ciniawsky what u think?
michael-ciniawsky
changed the title
devtool (options.sourceMap)
michael-ciniawsky
added
pr_action: Rebase
pr_action: Review
and removed
pr_action: Review
pr_action: Rebase
labels
|
My vote is this logic should remain in webpack proper, as the logic has even changed since this PR was suggested. Just my 2 cents. |
|
Agreeing with @mikesherov here, this adds unnecessary side effects to the plugin |
As described here: #80 (comment)