fix(@angular/cli): Disable comparisons feature in uglify compression in prod by Wykks · Pull Request #7931 · angular/angular-cli

Wykks · 2017-10-04T14:42:37Z

…in production

googlebot · 2017-10-04T14:42:40Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

Wykks · 2017-10-05T08:51:36Z

I signed it!

googlebot · 2017-10-05T08:51:38Z

CLAs look good, thanks!

Wykks · 2017-10-05T20:58:43Z

Some size tests :
A small app (which use mapbox-gl) with comparisons: false

chunk {0} polyfills.d8d3d78a4deb2ab66856.bundle.js (polyfills) 66.2 kB {4} [initial] [rendered]
chunk {1} main.e0d1f68923b143f095a6.bundle.js (main) 40.9 kB {3} [initial] [rendered]
chunk {2} styles.cbb57e89e9ac6ad6f3fb.bundle.css (styles) 78.1 kB {4} [initial] [rendered]
chunk {3} vendor.4ad7ad49f3f7f165fff5.bundle.js (vendor) 1.08 MB [initial] [rendered]
chunk {4} inline.94dcaa1fbb222b2f330f.bundle.js (inline) 1.45 kB [entry] [rendered]

Without comparisons: false

chunk {0} polyfills.d8d3d78a4deb2ab66856.bundle.js (polyfills) 66.1 kB {4} [initial] [rendered]
chunk {1} main.e0d1f68923b143f095a6.bundle.js (main) 40.9 kB {3} [initial] [rendered]
chunk {2} styles.cbb57e89e9ac6ad6f3fb.bundle.css (styles) 78.1 kB {4} [initial] [rendered]
chunk {3} vendor.4ad7ad49f3f7f165fff5.bundle.js (vendor) 1.08 MB [initial] [rendered]
chunk {4} inline.94dcaa1fbb222b2f330f.bundle.js (inline) 1.45 kB [entry] [rendered]

(Using @angular/cli v1.4.4)

filipesilva · 2017-10-12T09:28:40Z

@Wykks can you check out my alternative solution in #5804 (comment) please? I prefer that one if it also works.

The node only `global` object had been left in because it used to cause a build size regression with Angular. This doesn't seem to be the case anymore so it should be removed because it causes problematic interactions with some libraries. Fix angular#5804 Supersedes angular#7931

The node only `global` object had been left in because it used to cause a build size regression with Angular. This doesn't seem to be the case anymore so it should be removed because it causes problematic interactions with some libraries. Fix #5804 Supersedes #7931

khanhvu161188 · 2017-11-07T01:48:23Z

the PR worked well. Hope it will be merged soon

Wykks · 2017-11-13T12:12:42Z

Tests with a slightly bigger project (angular/cli 1.5 & angular 5):
Without comparisons: false, prod buid (mapbox-gl doesn't works):

chunk {0} 0.7120f44b0b35c3f30ca3.chunk.js (common) 87.6 kB  [rendered]
chunk {1} 1.806b6d85871e191d087d.chunk.js () 334 kB  [rendered]
chunk {2} 2.84d2c2f503211e36c6b2.chunk.js () 633 kB  [rendered]
chunk {3} 3.8f203f13ef09c83a1cb4.chunk.js () 16.9 kB  [rendered]
chunk {4} 4.ee3dc3a237fdadde98e5.chunk.js () 38.2 kB  [rendered]
chunk {5} 5.c209b3bdcd81230bfdb7.chunk.js () 6.28 kB  [rendered]
chunk {6} 6.dd49bbc2f4e2cf3d3930.chunk.js () 72.1 kB  [rendered]
chunk {7} main.93622f1b31e94850762f.bundle.js (main) 2.69 MB [initial] [rendered]
chunk {8} polyfills.8f464027c5f775c4b960.bundle.js (polyfills) 66.5 kB [initial] [rendered]
chunk {9} styles.9487a01f65ac7a4f3242.bundle.css (styles) 179 kB [initial] [rendered]
chunk {10} inline.3d54bd4262d98168f168.bundle.js (inline) 1.63 kB [entry] [rendered]

With comparisons: false, prod build (mapbox-gl works):

chunk {0} 0.7120f44b0b35c3f30ca3.chunk.js (common) 87.6 kB  [rendered]
chunk {1} 1.806b6d85871e191d087d.chunk.js () 334 kB  [rendered]
chunk {2} 2.84d2c2f503211e36c6b2.chunk.js () 633 kB  [rendered]
chunk {3} 3.8f203f13ef09c83a1cb4.chunk.js () 16.9 kB  [rendered]
chunk {4} 4.ee3dc3a237fdadde98e5.chunk.js () 38.2 kB  [rendered]
chunk {5} 5.c209b3bdcd81230bfdb7.chunk.js () 6.28 kB  [rendered]
chunk {6} 6.dd49bbc2f4e2cf3d3930.chunk.js () 72.1 kB  [rendered]
chunk {7} main.93622f1b31e94850762f.bundle.js (main) 2.69 MB [initial] [rendered]
chunk {8} polyfills.8f464027c5f775c4b960.bundle.js (polyfills) 66.5 kB [initial] [rendered]
chunk {9} styles.9487a01f65ac7a4f3242.bundle.css (styles) 179 kB [initial] [rendered]
chunk {10} inline.3d54bd4262d98168f168.bundle.js (inline) 1.63 kB [entry] [rendered]

Yup, same size. I don't think there's any good reason to not disable comparisons at this point.

SublimeProphets · 2017-11-13T14:35:31Z

The PR works perfect for me.
I'm running angular 5.0.1 and angular-cli 1.5.0

khanhvuNOIS · 2017-11-13T14:41:34Z

The PR worked well

filipesilva · 2017-11-29T17:21:07Z

Heya, will be merging this into the next 1.6 release.

kzc · 2017-11-29T23:33:49Z

@filipesilva While I disagree with the root cause of this issue, if mapbox-gl will not address it then it would be better to disable the more specific uglify compress option typeofs rather than disabling comparisons.

There's a few optimizations unrelated to mapbox-gl that would be negatively affected with comparisons disabled.

Example code with default compress options:

$ echo 'function f(global){ var o = {}; console.log(o == o, typeof global === "undefined"); }' | bin/uglifyjs -c
function f(global){console.log(true,void 0===global)}

Same code with comparisons disabled:

$ echo 'function f(global){ var o = {}; console.log(o == o, typeof global === "undefined"); }' | bin/uglifyjs -c comparisons=false
function f(global){var o={};console.log(o==o,"undefined"===typeof global)}

And again the same code with typeofs disabled:

$ echo 'function f(global){ var o = {}; console.log(o == o, typeof global === "undefined"); }' | bin/uglifyjs -c typeofs=false
function f(global){console.log(true,"undefined"==typeof global)}

You'll notice that the contentious webpacked mapbox-gl "undefined"==typeof global is undisturbed while the other comparisons optimization is still performed.

Not a big deal in the grand scheme of things, but sometimes the comparisons transform can be used to eliminate unnecessary code.

kzc · 2017-11-29T23:52:58Z

packages/@angular/cli/models/webpack-configs/production.ts


-  const uglifyCompressOptions: any = {};
+  const uglifyCompressOptions: any = {
+      // Disabled because of an issue with Uglify breaking seemingly valid code:


That statement is inaccurate. See mishoo/UglifyJS#2520 (comment)

I'm not too proficient in english, but it's written "an issue with Uglify", and not "an issue in Uglify". It's two different things, no ?

It's a mapbox-gl issue.

filipesilva · 2017-11-30T14:25:53Z

@kzc I was unaware that there was a better way to target the specific problem, and agree that it would be better. Would you be willing to put up a similar PR to makes it more specific?

For the record our intention to address this class of problems is to wholly disable the node global in webpack for browser apps in CLI 2.0. Merging this fix was an interim compromise to support mapbox-gl users.

kzc · 2017-11-30T15:07:19Z

@filipesilva Since this project is one of the bigger users of uglify-es, I scan the issues occasionally and happened across this. I'll leave it to you whether or not to act on this information.

…rkaround See angular#7931 (comment)

filipesilva · 2017-11-30T15:19:57Z

@kzc we really appreciate that of you, your insight is very useful for keeping our Uglify usage correct and bug free.

I put up a PR with your suggested improvement: #8694

…rkaround See #7931 (comment)

The node only `global` object had been left in because it used to cause a build size regression with Angular. This doesn't seem to be the case anymore so it should be removed because it causes problematic interactions with some libraries. Fix angular#5804 Supersedes angular#7931

…rkaround See angular#7931 (comment)

angular-automatic-lock-bot · 2019-09-12T06:40:46Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

fix(@angular/cli): Disable comparisons feature in uglify compression …

5504e92

…in production

googlebot added the cla: no label Oct 4, 2017

googlebot added cla: yes and removed cla: no labels Oct 5, 2017

Brocco requested a review from filipesilva October 5, 2017 18:36

Brocco assigned filipesilva Oct 5, 2017

filipesilva mentioned this pull request Oct 12, 2017

mapboxgl is incompatible with the CLI in a production build #5804

Closed

filipesilva mentioned this pull request Oct 20, 2017

fix(@angular/cli): disable node global #8130

Merged

filipesilva added target: master labels Nov 29, 2017

filipesilva approved these changes Nov 29, 2017

View reviewed changes

filipesilva merged commit 8821520 into angular:master Nov 29, 2017

kzc reviewed Nov 29, 2017

View reviewed changes

filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Nov 30, 2017

fix(@angular/cli): use a more restrictive workaround for mapbox-gl wo…

e0c4e2e

…rkaround See angular#7931 (comment)

filipesilva mentioned this pull request Nov 30, 2017

fix(@angular/cli): use a more restrictive workaround for mapbox-gl wo… #8694

Merged

hansl pushed a commit that referenced this pull request Dec 1, 2017

fix(@angular/cli): use a more restrictive workaround for mapbox-gl wo…

ef52675

…rkaround See #7931 (comment)

clydin pushed a commit that referenced this pull request Dec 4, 2017

fix(@angular/cli): use a more restrictive workaround for mapbox-gl wo…

24d2520

…rkaround See #7931 (comment)

Wykks mentioned this pull request Mar 3, 2018

Uncaught ReferenceError: e is not defined alex3165/react-mapbox-gl#200

Closed

dond2clouds pushed a commit to d2clouds/speedray-cli that referenced this pull request Apr 23, 2018

fix(@angular/cli): use a more restrictive workaround for mapbox-gl wo…

ec8a92d

…rkaround See angular#7931 (comment)

jellis mentioned this pull request Jul 23, 2018

Patching 1.3 branch with ng build breaking on uglify per 1.5 branch #11623

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

Conversation

Wykks commented Oct 4, 2017

Uh oh!

googlebot commented Oct 4, 2017

Uh oh!

Wykks commented Oct 5, 2017

Uh oh!

googlebot commented Oct 5, 2017

Uh oh!

Wykks commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filipesilva commented Oct 12, 2017

Uh oh!

khanhvu161188 commented Nov 7, 2017

Uh oh!

Wykks commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SublimeProphets commented Nov 13, 2017

Uh oh!

khanhvuNOIS commented Nov 13, 2017

Uh oh!

filipesilva commented Nov 29, 2017

Uh oh!

kzc commented Nov 29, 2017

Uh oh!

kzc Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

Wykks Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

kzc Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

filipesilva commented Nov 30, 2017

Uh oh!

kzc commented Nov 30, 2017

Uh oh!

filipesilva commented Nov 30, 2017

Uh oh!

angular-automatic-lock-bot bot commented Sep 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Wykks commented Oct 5, 2017 •

edited

Loading

Wykks commented Nov 13, 2017 •

edited

Loading