Skip to content
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

Build optimizer breaks third party libraries #12128

Closed
alexeagle opened this issue Sep 4, 2018 · 5 comments
Closed

Build optimizer breaks third party libraries #12128

alexeagle opened this issue Sep 4, 2018 · 5 comments

Comments

@alexeagle
Copy link
Contributor

From @yGuy on April 3, 2018 11:56

Bug Report or Feature Request (mark with an x)

- [x] bug report -> please search issues before submitting
- [ ] feature request

Area

- [x] devkit
- [ ] schematics

Versions

all

Repro steps

  • Scaffold a project that uses code (e.g. a third party library) that contains code that cannot be properly "optimized" using uglifyjs. E.g. code that uses "non-pure getters" like this:
let object = {};
Object.defineProperty(object,"nonSideEffectFreeGetterBasedMember", 
    {get: function(){ console.log("side effect")}});

(function () {
  object.nonSideEffectFreeGetterBasedMember;
}());
  • Enable the angular build optimizer
  • Run the program with the build optimizer enabled and observe that the code does not work as expected anymore, because the non-pure getter has been removed and the side-effect does not happen anymore.

The log given by the failure

n/a

Desired functionality

It should be possible to either configure the build optimizer and disable possibly logic-breaking optimizations altogether or at least on a per dependency basis. Especially if the third party library is already optimized/minified/preprocessed the danger of breaking things is higher than the benefits of possibly saving a few more bytes.

Mention any other details that might be useful

Right now it is an either/or - either the code runs and you can rely on it, or it may be "optimized" but broken in some cases.

The tool-chain makes it easy to include all kind of packages from third parties via npm, etc. The "build-optimizer" cannot dictate how the code of all of these packages needs to be written in order to work with the "optimization" step. It's OK to forbid non-pure getters in the angular code base, but not in any third party code. Either add an option to only apply safe code transformations for third party code or make this an opt-in. The binary API right now is not sufficient.

Copied from original issue: angular/devkit#612

@alexeagle
Copy link
Contributor Author

From @seanbright on May 14, 2018 21:39

I'm experiencing this as well with https://github.com/onsip/SIP.js as a dependency. Building with Angular CLI without the --prod flag my application works, with the --prod flag it doesn't. No errors are emitted, so it seems that something is just getting silently optimized away.

@seanbright
Copy link

The issue with https://github.com/onsip/SIP.js was resolved by replacing their hand-rolled MD5 implementation with crypto-js:

onsip/SIP.js@9dde443

@alan-agius4
Copy link
Collaborator

Hi, the problem with the above, is that a getter value is not being used and hence it is being dropped.

If a library is non side effect free, the library author needs to specify this in their package.json using sideEffects field.

If the problem persists please open a new issue, provide a simple repository reproducing the problem, and describe the difference between the expected and current behavior.

@yGuy
Copy link

yGuy commented Mar 13, 2019

That's wrong - it should be:

"If a library is side effect free it may specify this in its package.json using sideEffects false"

Node module authors should not be forced to indicate that they are using valid ecmascript features. As an optimization, if they are not using a specific feature, they could indicate this to help tools that may optimize for this specific situation, but having to list valid features from the spec that you are using and assuming that they are not being used if they are not listed is just plain stupid and ignorant.
The problem is the build "optimizer" that breaks the code, not the code that has not been written to workaround issues in the build optimizer. Closing issues without a resolution to the actual problem and blaming others for their valid usage of a language feature does not make this any more correct.

It's still JavaScript in NPM, not AngularScript or GoogleScript. Feel free to make the build optimizer require AngularScript instead, but then clearly document this and tell the world that this is not a JavaScript tool, but an AngularScript tool which only works with AngularScript.

filipesilva added a commit to filipesilva/angular-cli that referenced this issue Apr 22, 2019
When we first started using Build Optimizer, we saw a lot of the savings were tied to using the Uglify/Terser `pure_getters` option.

This was intimately related with the structure and shape of the Angular codebase. The measurements we did at the time on angular.io showed a significant size reduction, from 1mb to about 600kb. Of these roughly 150kb were tied to using `pure_getters` if I remember correctly.

Meanwhile the Angular codebase has changed significantly and I don't really see these savings anymore, so I don't think it makes sense to keep it on given that it is known to cause problems with some libraries.

Closes angular#9231, angular#11439, angular#12096, angular#12128.
filipesilva added a commit to filipesilva/angular-cli that referenced this issue Apr 24, 2019
When we first started using Build Optimizer, we saw a lot of the savings were tied to using the Uglify/Terser `pure_getters` option.

This was intimately related with the structure and shape of the Angular codebase. The measurements we did at the time on angular.io showed a significant size reduction, from 1mb to about 600kb. Of these roughly 150kb were tied to using `pure_getters` if I remember correctly.

Meanwhile the Angular codebase has changed significantly and I don't really see these savings anymore, so I don't think it makes sense to keep it on given that it is known to cause problems with some libraries.

Closes angular#9231, angular#11439, angular#12096, angular#12128.
alexeagle pushed a commit that referenced this issue Apr 24, 2019
When we first started using Build Optimizer, we saw a lot of the savings were tied to using the Uglify/Terser `pure_getters` option.

This was intimately related with the structure and shape of the Angular codebase. The measurements we did at the time on angular.io showed a significant size reduction, from 1mb to about 600kb. Of these roughly 150kb were tied to using `pure_getters` if I remember correctly.

Meanwhile the Angular codebase has changed significantly and I don't really see these savings anymore, so I don't think it makes sense to keep it on given that it is known to cause problems with some libraries.

Closes #9231, #11439, #12096, #12128.
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants