comparisons compression option in uglifyjs webpack prod config breaks on react-mapbox-gl
davidascher opened this issue · 19 comments
Can you reproduce the problem with latest npm?
Yes (including npm 5).
Description
There seems to be an incompatibility between CRA 1.0+ and the react-mapbox-gl component. This bug was not present before CRA 1.0, and only shows up in production mode, which makes me think it's a webpack 2.0 related issue.
Expected behavior
See https://github.com/davidascher/mapbox-repro-bug for a minimal test case (single component render -- works in development mode, doesn't work in production mode)
Actual behavior
In production mode (yarn build + serve -s build) , the map doesn't show and there's an incomprehensible traceback (as it's minified).
Environment
Run these commands in the project folder and fill in their results:
npm ls react-scripts(if you haven’t ejected): react-scripts@1.0.6node -v: v7.9.0npm -v: 5.0.0 (but it happened with 4.x as well)
Then, specify:
- Operating system: OSX
- Browser and version: Chrome 58.0.3029.110
Reproducible Demo
Source: https://github.com/davidascher/mapbox-repro-bug
(trivial CRA repo with one dependency).
Live site showing bug: https://build-wnyrhmqiph.now.sh/
/cc @alex3165 as he is the maintainer of react-mapbox-gl AFAICT.
Aside: confirmed with the repo above that downgrading to react-scripts 0.9.5 fixes the problem.
Confirmed that if I eject, and change the build step to use the webpack.config.dev config, the bug goes away.
If I eject and simply comment out the UglifyJsPlugin from the plugins list in the production config, the bug goes away. Live site showing this: https://build-ijvjggnled.now.sh/
With @kzc's help, this has been traced it down to an uglify option (see mishoo/UglifyJS#2011 (comment)).
Setting
compress: {
comparisons: false
}
in the webpack prod config for the UglifyJS plugin fixes the problem.
I'll see if I can figure out how to override the webpack configs, I vaguely remember a bug going by about that new capability.
Want to send a PR disabling that option?
Please let us know if Uglify fixes this!
@gaearon Uglify won't fix this it seems, as the root cause seems to be in webworkify doing something not kosher / out of scope.
See tail end of the uglify thread: mishoo/UglifyJS#2011 (comment)
And webworkify bug: browserify/webworkify#35
I see. If it's not a bug we'll likely reenable the optimization in the future.
I don't personally yet understand where the actual bug lies. It's possible that webworkify's Blob-creation code can be fixed to not break uglify, we'll see what the webworkify maintainers say.
I understand that the uglifyjs maintainers need to define a scope and can be strict about it. From a CRA point of view, reenabling the optimization means from the user's PoV code that works in dev mode doesn't work in production and it's hard to understand & track down the root cause. I would recommend against re-enabling it until we understand a workaround other than ejecting. webworkify seems to have a bunch of usage: https://www.npmjs.com/package/webworkify. Certainly all mapbox + CRA users would feel it.
On my minimal test case (including react, react-mapbox-gl, mapbox-gl), the difference between the created JS using webpack.prod.js between 1.0.6 and 1.0.7 is:
1.0.6: 668579 bytes
1.0.7: 669116 bytes
So for this case anyway the choice is to save 537 bytes (0.08% of JS bloat) and have a production build that works, or the other way around.
webworkify is not the problem. The function in the package being stringified by webworkify is violating an assumption uglify makes about static code analysis - that the variable in question is defined in an outer scope. Of course that package in question would work correctly if not uglified - it has a different set of assumptions and clearly was not written with minification in mind. It's not correct to call it a bug in that package either - it's a confluence of factors. Can the code that makes use of webworkify be changed to work whether uglified or not? Quite possibly. If not, the end developer would have to disable uglify comparisons.
Ah, thanks for that explanation, @kzc. I'll see if I can figure out what that code is.
There seems to be a mapbox-gl-js issue on this topic: mapbox/mapbox-gl-js#4359
It's a confluence of factors:
- A module used by mapbox-gl-js has code that checks for the presence of
globalusingtypeof global !== "undefined". At the point whereglobalis used, there's no outer context in which a variableglobalis defined. - webworkify stringifies functions to send to the worker. This is risky, but is the only known solution for bundling worker code that:
- Doesn't require loading a second source file from the server
- Doesn't duplicate code that's used in both worker and main thread
- Something in create-react-app setup wraps mapbox-gl-js in
function (global) { ... }. - Uglify decides to mangle the resulting
globaland transform the resultingtypeof g !== "undefined"tovoid 0 !== g. Together, this produces broken code when the stringified function is executed outside of thefunction (g) { ... }wrapper.
Because of this confluence, it's likely to be one of those situations where the maintainers of each of the individual components involved insist that what their code does is perfectly valid, and it's the other stuff that's making invalid assumptions and must be fixed. At the risk of doing so myself, I would say the bug is in webpack: it provides no mechanism for a package to indicate what its build/packaging/bundling requirements are. If we, the authors of mapbox-gl-js, could put something in package.json indicating "don't mess with this code, it's already been bundled and minified in the way that it needs to be; redundant wrapping, bundling, and mangling is likely to break it", then issues like this wouldn't keep cropping up.
I'm not a user of mapbox-gl-js, but here's a couple of possible workarounds to avoid the use of global...
These are the contentious globals in question browserify'd by mapbox-gl-js:
var Buffer = global.Buffer || require('./buffer');https://github.com/mapbox/pbf/blob/v1.3.7/index.js#L5
That can be solved by upgrading pbf to the latest version which does not use global.
And util.deprecate which no one cares about in the context of mapbox-gl-js:
exports.deprecate = function(fn, msg) {
// Allow for deprecating things in the process of starting up.
if (isUndefined(global.process)) {https://github.com/defunctzombie/node-util/blob/b5d10a26402c3b60ce496e675a13c58bbed47801/util.js#L67
That can be solved by making a fork of util@0.10.3 to remove that function.
Perhaps util.deprecate can be retained but simply remove this code:
Alternatively, if browserify (or is it webpack?) can be modified to change the order of conditions in the following generated code to check for window first and global last it may also work:
typeof global !== "undefined" ? global : typeof self !== "undefined" ? self : typeof window !== "undefined" ? window : {}
I don't know where that code is.
Hey guys; I'm a little confused: this bug (and the mapbox-gl linked bug) are both "closed" but the problem still persists, right? And the disable micro-option doesn't seem to have been pulled in (yet? is the plan to pull that in for now?)...
Just trying to understand how to workaround this issue / when/what the real fix is (going to be)
@jliebrand looks like it was fixed in version 1.0.7
#2379
https://github.com/facebookincubator/create-react-app/releases/tag/v1.0.7
Is the issue fixed? Can we re-enable it now in our projects?
mapbox-gl fixed their webpack typeof global issue in mapbox/mapbox-gl-js#6956.
The workaround in https://github.com/facebook/create-react-app/pull/2379/files is no longer needed and can be reverted.