search

TrySound / rollup-plugin-uglify

Rollup plugin to minify generated bundle
MIT License
260 stars 42 forks source link

Reject sourcemap before sending it to uglify #63

Closed uorbe001 closed 5 years ago

uorbe001 commented 5 years ago

At some point today, our builds started failing with a sourcemap error from this lib. Running the tests here makes it replicable:

  ● allow to disable source maps

    DefaultsError: `sourcemap` is not a supported option

      at DefaultsError.get (eval at <anonymous> (node_modules/uglify-js/tools/node.js:20:1), <anonymous>:71:23)

This fixes the error by rejecting the option uglify is now rejecting before the whole thing crashes. I have no idea what's changed today, as the option rejection has been in uglify for ages: https://github.com/mishoo/UglifyJS2/blob/v3.6.0/lib/utils.js#L93

TrySound commented 5 years ago

Could you add test please

uorbe001 commented 5 years ago

@TrySound how do you suggest I write a test for it? The only change here is that this.worker.transform doesn't receive the offending params, and that object is instantiated by the index itself. The only way I can think of to test that properly would be to refactor the minifiedOptions out of the index and test that independently.

In terms of the params being sent to uglify being correct, the pre-existing tests should ensure that. I would even argue that "allow to disable source maps" covers this change, as it was failing before it.

TrySound commented 5 years ago

serialize-javascript upgrade in master gave me failing test. Please upgrade to get this change covered.

uorbe001 commented 5 years ago

Extracted the normalize in order to add a test for it, and upgraded serialize-javascript to 1.9.0.

TrySound commented 5 years ago

Great! Thank you

TrySound commented 5 years ago

Published 6.0.3

TrySound commented 5 years ago

Also I recommend to use rollup-plugin-terser instead.