create-react-app: comparisons compression option in uglifyjs webpack prod config breaks on react-mapbox-gl

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:

  1. npm ls react-scripts (if you haven’t ejected): react-scripts@1.0.6
  2. node -v: v7.9.0
  3. npm -v: 5.0.0 (but it happened with 4.x as well)

Then, specify:

  1. Operating system: OSX
  2. 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.

About this issue

  • Original URL
  • State: closed
  • Created 7 years ago
  • Comments: 19 (12 by maintainers)

Commits related to this issue

Most upvoted comments

It’s a confluence of factors:

  • A module used by mapbox-gl-js has code that checks for the presence of global using typeof global !== "undefined". At the point where global is used, there’s no outer context in which a variable global is 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 global and transform the resulting typeof g !== "undefined" to void 0 !== g. Together, this produces broken code when the stringified function is executed outside of the function (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.

mapbox-gl fixed their webpack typeof global issue in https://github.com/mapbox/mapbox-gl-js/pull/6956.

The workaround in https://github.com/facebook/create-react-app/pull/2379/files is no longer needed and can be reverted.

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)

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:

https://github.com/defunctzombie/node-util/blob/b5d10a26402c3b60ce496e675a13c58bbed47801/util.js#L66-L71

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.

With @kzc’s help, this has been traced it down to an uglify option (see https://github.com/mishoo/UglifyJS2/issues/2011#issuecomment-304405775).

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.