Fix sourcemap corruption - use composite map from UglifyJS

[Jimbly]

Conversation and rational at: #236 (comment)
Fixes #236

[Jimbly]

@terinjokes Can we get this PR merged? I've been using my fork in a number of projects with this fix, and all has been as good as before or better =).

[terinjokes]

Don't remove testdouble from the tests, but fill free to remove sourcemap logic that's in them.

[terinjokes]

This isn't going to be stable across versions.

[Jimbly]

Don't remove testdouble from the tests, but fill free to remove sourcemap logic that's in them.

I'm not quite sure what you mean? It seems like in that test all td was doing was providing a fake sourcemap. I could make it return the sourcemap which Uglify currently returns, but then this test would be testing absolutely nothing useful (e.g. if, say, someone undid my fix, or removed any of the relevant parameters to uglify.minify (e.g. options.sourceMap), the tests would still pass!). With this fix, gulp-uglify is now doing nothing other than passing through the sourcemaps to and from Uglify, it seems very important to test that we're passing the appropriate options to get the correct behavior from Uglify. If we upgrade what version of uglify-js that gulp-uglify depends on, I think it would be good to know if the sourcemap handling has changed or broken. Similarly, this test in master is only passing because this is "simulating" a call to Uglify by returning something that Uglify won't actually return (though possibly did before it's sourcemap handling changes, at which point gulp-uglify would have "rotted" without any tests failing).

This isn't going to be stable across versions.

This is a simple enough single line of code being minified that I would expect it to be stable, but if we can find a counter example, the test could be made more robust by using a module that parses source maps into something human readable and asserting on some heuristic checks that there are multiple mappings from a single line going back to multiple lines in multiple files.

[terinjokes]

This is a simple enough single line of code being minified that I would expect it to be stable

UglifyJS makes no promises on the identifiers it uses when minifying.

[Jimbly]

UglifyJS makes no promises on the identifiers it uses when minifying.

Of course! Luckily the identifiers are not included in the map, only the offsets. In general, as long as the two identifiers it chooses are both single character (pretty much guaranteed if it's doing it's job), it'll be the same map. That being said, the only identifiers in the code I'm including in the test are globally-scoped, which Uglify must not rename.

Anyway, just to get on the same page, I'd really like it if gulp-uglify included a test that would fail the next time the way it integrates with uglify-js breaks sourcemaps (e.g. if the version of uglify-js that gulp-uglify depends on is bumped and it has changed its behavior again). It'd be nice if no one had to go through the horrors of trying to track this kind of thing down again ^_^. However, that's less important than actually getting this fix live, so if you really feel the former is not important (or too much of a burden on maintenance), I can change the test to not call uglify-js at all, and just validate that the parameters being passed are as expected and that gulp-uglify isn't making any changes to the sourcemaps provided, and that'll certainly be stable (though won't catch breaking changes along the lines of what broke this previously).

[terinjokes]

I'd prefer tests not call out to UglifyJS. It's out of scope for this project to ensure we are seeing a specific byte pattern from them. In fact, in the next major version of this project, we're unlikely to be depending on UglifyJS at all, and users will expected to provide their own (like a few other Gulp plugins). If UglifyJS, or a fork, make another breaking change, that's going to be on them.

You can make that change, or I can do so when merging the rest. Thanks for digging into this and sending a PR.

[Jimbly]

Okay, got it.

For the next version, that sounds slightly error prone, since presumably the caller would need to inform you (or know and handle themselves) whether or not the underlying minifier is outputting sourcemaps that are relative to the supplied code or relative to the original, to know whether or not to call applySourceMap or just =, suddenly you need to provide a cookbook of recipes to users instead of just the simplicity of "use this if you want your code uglified".

Anyway, that's neither here nor there... if you want to make the change while merging, that sounds good, I am completely unfamiliar with test double, might take me a bit to figure it out, but I could get to it on Monday if you don't have time =).