-
Notifications
You must be signed in to change notification settings - Fork 37
Set "typeofs: false" to prevent mangling typeofs by uglify #439
Conversation
cc @vicapow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Would it be possible to add a unit test which illustrates where this would cause a problem? It would be great to ensure that we don't break this again in the future.
build/compiler.js
Outdated
@@ -505,6 +505,13 @@ function getConfig({target, env, dir, watch, cover}) { | |||
parallel: true, // default from webpack | |||
uglifyOptions: { | |||
compress: { | |||
// typeofs: true (default) transforms typeof foo == "undefined" into foo === void 0. | |||
|
|||
// This mangles mapbox-gl creating an error when used alongside with window global mangling: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not mention project-specific bugs, and probably avoid links to websites not accessible to the public within the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generically reproducible... I want to get the static server to run though... see slack :)
Do you have other compile configuration tests we can use as an example? |
It depends on the behavior of what happens if there was an error. It sounds like this possibly exhibits as a node.js runtime error? If we can add a fixture with logic that reproduces this to the fixtures directory, we should be able to add a test like the following which ensures SSR works: Lines 444 to 449 in 9e87239
Alternatively, if you help us create a minimal repo which reproduces the issue, we can incorporate it as a fixture with test. Maybe on top of |
Yes I can reproduce this generically. I'll add a "unit" test. |
build/compiler.js
Outdated
// typeofs: true (default) transforms typeof foo == "undefined" into foo === void 0. | ||
|
||
// This mangles mapbox-gl creating an error when used alongside with window global mangling: | ||
// https://jeng.uberinternal.com/browse/WPT-1185 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this
Supposedly this was fixed in the latest mapbox-gl: mapbox/mapbox-gl-js#6956. Can we simply stop using the old versions? |
Good catch @mlmorg i agree we should just update the dep if this is fixed upstream. |
This is still an issue unrelated to mapbox specifically, (but manifests itself there). People will be using older versions of mapbox and fusion in the wild, and it should be able to be compatible with these sort of cases. Besides, uglification changes like there won't affect the bundle size much |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am ok taking these changes in favor of correctness. I was unable to figure out what kind of code causes an error in Fusion, latest mapbox-gl seems to work fine. It's highly possible that this will break without a test in the future though.
typeofs: true (default)
transforms typeof foo == "undefined" into foo === void 0.This mangles mapbox-gl creating an error when used alongside with window global mangling:
webpack-contrib/uglifyjs-webpack-plugin#189