-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
'comparisons' compression option results in incorrect code with react-mapbox-gl #2011
Comments
Please post a link to a gist of the final javascript file after all the babeling but before uglify is run on it. Also provide which uglify options are to be used to build it, and identify the section of code in the final javascript input that you believe is transformed incorrectly. |
I believe this is the JS when it does not run through uglifyjs: https://gist.github.com/davidascher/da5ec954598cfa1a43c61d429bc4726d Unfortunately I have no idea which part of the JS input is problematic, and I realize it's large (it contains all of react, etc.). As to the options used to configure uglify, I don't know what https://github.com/webpack-contrib/uglifyjs-webpack-plugin does, but the webpack configuration in question does:
I realize this is not a minimal test case from uglify-js's point of view, but it's the best I can do. |
The broken website link above shows this in the error console:
but with no stack trace or line number. To aid debugging could you use the following uglify options:
and provide the error, line number and associated code? Your
Could you please run |
Thanks for digging in! For the uglify-js version question:
For the less obfuscated code: https://build-dxltejnpzf.now.sh is running the code w/ the options you specified.
The line in question is
which seems to be last line in this large chunk:
but you may prefer to look at the site for the entire stack trace & code. |
I think your issue might not be related to uglify at all. Please try these options and report back with the error:
|
Thanks, this is super helpful. With those options, the site works fine. As soon as I set
which breaks the build (with Here's the diff of the two JS files: https://gist.github.com/davidascher/6156c5abe357844a119d3420804aa07d I'm not sure of the wisdom of stripping the extra = in a bunch of those comparisons, but they seem benign. Some of the transformations seem more likely problematic: https://gist.github.com/davidascher/6156c5abe357844a119d3420804aa07d#file-difference-diff-L200 - void 0 === i && (i = 1e-6);
+ "undefined" == typeof i && (i = 1e-6); https://gist.github.com/davidascher/6156c5abe357844a119d3420804aa07d#file-difference-diff-L209 - void 0 !== module && module.exports ? module.exports = isSupported : window && (window.mapboxgl = window.mapboxgl || {},
+ "undefined" != typeof module && module.exports ? module.exports = isSupported : window && (window.mapboxgl = window.mapboxgl || {}, https://gist.github.com/davidascher/6156c5abe357844a119d3420804aa07d#file-difference-diff-L227 - }).call(this, void 0 !== global ? global : "undefined" != typeof self ? self : "undefined" != typeof window ? window : {});
+ }).call(this, "undefined" !== typeof global ? global : "undefined" !== typeof self ? self : "undefined" !== typeof window ? window : {}); https://gist.github.com/davidascher/6156c5abe357844a119d3420804aa07d#file-difference-diff-L245 - }).call(this, _dereq_("_process"), void 0 !== global ? global : "undefined" != typeof self ? self : "undefined" != typeof window ? window : {});
+ }).call(this, _dereq_("_process"), "undefined" !== typeof global ? global : "undefined" !== typeof self ? self : "undefined" !== typeof window ? window : {}); with the last one looking a lot like the original problematic line in the traceback. |
So this is against |
I don't see anything immediately wrong around the four instances of
|
If https://build-dxltejnpzf.now.sh/ is debugged in Chrome you will see that the main thread handles this line fine:
In fact, the debugger shows that However, the same line throws a ReferenceError for I've never used a web service worker. Are the laws of javascript variable resolution different for them compared to the main thread? |
Web Worker does not have access to |
Otherwise variable resolution is the same on main or web worker threads. One issue that could have happened is if the main thread sends the code to a worker thread via |
Because the variable Is a web service worker thread effectively forked from the main thread (in the UNIX process sense) and any references to |
Web Worker is a completely isolated execution environment. They share nothing and has to |
What I meant in the second half of #2011 (comment) is that in order to "share" a function say worker.postMessage(f.toString()); ... then on the worker end: addEventListener("message", function(event) {
eval(event.data);
}); But obviously that would break any scoping that the original function was in. |
The official way to execute code on Web Worker is to pass in the script's URL: new Worker("path/to/worker.js"); Anything else is just hacking around this limitation. (Blob URLs is a way to "embed" code, but is not supported on all major web browsers.) |
Now we're getting somewhere - references to Could we separate out the |
I knew what you meant. |
I don't understand what you mean here - what is being I thought |
|
The broken web page was doing something similar to that when I examined the stack. Of course now I can't find that section of code. |
I don't think
Undeclared variables and variables not being defined are the same thing. The value |
Agreed - was just trying to make sure we are on the same page without confusing terminologies. |
If you can extract the code that |
I think it was a You can fire up Chrome and take a look at https://build-dxltejnpzf.now.sh |
I went with IE's Developer Tools and got the worker's code copied out, which seems to be similar to https://gist.github.com/davidascher/da5ec954598cfa1a43c61d429bc4726d but somehow some variables are mangled but not others. |
Here's where the failing web worker was spawned: 196: [ function(_dereq_, module, exports) {
"use strict";
var WebWorkify = _dereq_("webworkify"), window = _dereq_("../window"), workerURL = window.URL.createObjectURL(new WebWorkify(_dereq_("../../source/worker"), {
bare: !0
}));
module.exports = function() {
return new window.Worker(workerURL);
};
}, { |
That doesn't look odd, so it's probably fine. The |
I still think #2011 (comment) is the explanation for the broken |
I think the current (Full disclosure: I use Web Workers extensively in one of my projects and I never had any violations with |
That also explains the URL of the failing web worker:
|
Thanks @alexlamsl and @kzc for digging into this, much appreciated! I believe I've traced the snippet https://github.com/substack/webworkify/blob/master/index.js I realize this isn't an uglify issue at this point, but can either of you point out what incorrect assumption that code is making, so I can make a useful bug report there? |
@davidascher here's an abstract example of what happened: var cache = function(G) {
return {
m1: function() {
return void 0 === G;
}
};
}({});
// works fine
cache.m1();
// throws error
eval("(" + cache.m1.toString() + ")()"); That last line is what happens when you wrap the code as Blob URL then send it to |
A simple workaround is to add a $ cat stringified_asm.js
var cache = function(G) {
return {
m1: function() {
console.log(1 + 2); // prove that uglify compress is enabled here
return typeof G === "undefined";
},
m2: function() {
"use asm";
console.log(1 + 2); // prove that uglify compress is disabled here
return typeof G === "undefined";
}
};
}({});
eval("(" + cache.m2.toString() + ")()");
console.log("SUCCESS"); $ bin/uglifyjs stringified_asm.js -c -b
var cache = function(G) {
return {
m1: function() {
return console.log(3), void 0 === G;
},
m2: function() {
"use asm";
console.log(1 + 2);
return typeof G === "undefined";
}
};
}({});
eval("(" + cache.m2.toString() + ")()"), console.log("SUCCESS");
|
That sounds great, but I'm not sure what the guilty function is. |
On second thought, Perhaps we could consider an uglify specific directive or comment annotation to serve the same purpose. Edit: But it'd be better to rewrite the code in question to avoid stringifying functions with references to variables outside the scope of the function. |
Problematic issue what would cause people to disable minification entirely. relates issues in other projects facebook/create-react-app#2379 sleepycat/old_usesthis@523c872 mapbox/mapbox-gl-js#4359 mishoo/UglifyJS#2011 (comment)
Bug report. ES5 (transpiled by babel)
Uglify version (
uglifyjs -V
)2.8.27
JavaScript input
See this repo: https://github.com/davidascher/mapbox-repro-bug
The original failure is that
create-react-app
1.0 production builds that usereact-mapbox-gl
fail, while development builds work fine (that repo is the minimal such config I could make).Further debugging indicates that the key difference between the two configurations is the use of the
WebpackUglifyJS
plugin, which calls out touglify-js
.(the
ejected
branch shows the commit which fixes the bug in the production configuration: davidascher/mapbox-repro-bug@b0c95d2)I'm afraid the bundles are not minimal from an uglify-js point of view, but I'm unable to narrow the test case any further.
Broken live site: https://build-wnyrhmqiph.now.sh/
Working live site: https://build-ijvjggnled.now.sh/
Tracking this bug in CRA as: facebook/create-react-app#2376
The text was updated successfully, but these errors were encountered: