Repository navigation
Production build optimization breaks code #11439
Description
Bug Report or Feature Request (mark with an x)
- [x] bug report -> please search issues before submitting
- [ ] feature request
Area
- [x] devkit
- [ ] schematics
Versions
Windows
node v9.4.0
npm v5.6.0
Repro steps
Use a third party library that contain non-side effect free getters.
Run a production build.
Observer that the code breaks at runtime, whereas it runs fine without "optimization".
E.g.:
- Scaffold a project that uses code (e.g. a third party library) that contains code that cannot be properly "optimized" using uglifyjs with it's default settings. 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 - it's a runtime issue in the application
Desired functionality
The optimization step should optimize, but never break code. The "pure_getters" option should be configurable and by default "false" should be used to guarantee best possible compatibility. If someone wants to optimize further and take the risk, that's fine, but don't break code by default: Don't be optimistic or dictate how code should work in your opinion.
Ideally make it possible to exclude specific libraries from this treatment. Users may have no control over these libraries and patching them often is not an option.
Mention any other details that might be useful
This has been reported before but was always closed without comments or with a comment that the third party code should be changed to workaround this limitation in angular. This is a one-liner fix in angular, though and instead of dictating how third party code should be written, the CLI should try not to break existing code, instead. Debugging "optimized production code" is a PITA the gains are minimal compared to that. Make this a configurable setting and make it possible to exclude certain files or third party libraries.
These are the previous issues that all had the same cause and where closed or ignored with no further explanation:
#9231
#9221
angular/devkit#937
angular/devkit#388
angular/devkit#612
and probably some more.
We are a library provider and we had several customers running into that issue. It's a nightmare to debug and analyze and devs expect that there is no behavior change when the "optimizer" is turned on. But there can be and is a change: it breaks their projects in all kinds of ways.
Proposed workaround for people affected by this bug
Go and hack the buildoptimizer options in the code. Find the line that sets the pure_getters option to true or "strict" and set it to "false" instead. If there is no such line, make sure that "false" is used (because the default is true and breaks code).
Activity
psamusev commentedon Jul 9, 2018
I also have a problem with
buildOptimizer: truein case of using custom decorators. In result code I don't have them assignable for classesfilipesilva commentedon Jul 20, 2018
I don't think it's very accurate to list all of those issues saying they had the same cause and were ignored/closed. While it is true that some of the issues you linked are related to the
pure_getterssetting, not all of them are, and all of them have either a resolution or an explanation except for angular/devkit#612, which is yours and was closed because the repository was archived.Our stance about the defaults options is that they should provide the best size reduction for the vast majority of cases. For cases where the default options do not work, they can be disabled. The specific options that alter code are
optimizationandbuildOptimizer.A similar example is how we use the
mangleoption in Uglify. This option changes function and variable names, and will always break code that relies on looking up the function name.While there are definitely cases for looking up function names and using side effects in getters, in our testing both
mangleandpure_gettersprovided significant size reduction while being problematic for a very small amount of code, so we default to them.So for situations like the one you describe, we feel the correct approach is to keep the defaults and manually disable
buildOptimizer.I agree it would be nice to have more fine-grained control over the optimization options in the future, so I am marking this as a feature request.
yGuy commentedon Jul 20, 2018
I do think it is accurate to list those issues. I agree that for the angular/devkit#388 it may be wrong here. But as far as I can tell all of them would not exist if the buid optimizer would only optimize and not break the code. People tend to turn on optimization features ("why would it hurt?") and then have no idea why their code breaks.
Which one of the issues I listed do you think would be there if the default settings would have been different?
Yes, there were resolutions to some of the issues. The resolution always was to disable the broken feature.
As such I agree that maybe this issue (and my "ignored/archived duplicate") is the only issue that actually tries to fix the root problem, while all others just tried to get their code to run.
How great are these size savings? I feel they must be enormous if they outweight the price for broken, extremely hard to debug code. Can you share some details? One could argue if it is really safe to remove such code in some projects, then this is rather a problem of these projects, rather than a problem of the buildBreaker here.
In general I would really like to see the build optimizer focusing on "optimizing" the code that the user has control over and not some third party library code.
fbernhard commentedon Sep 5, 2018
From the UglifyJS documentation (https://github.com/mishoo/UglifyJS2):
pure_getters (default: "strict") -- If you pass true for this, UglifyJS will assume that object property access (e.g. foo.bar or foo["bar"]) doesn't have any side effects.
In my humble opinion the Angular CLI should not assume that code is written in a certain way. As @yGuy said, "The optimization step should optimize, but never break code."
clydin commentedon Sep 5, 2018
It is an important optimization and if absolutely essential can be disabled by turning off build-optimizer. There are no current plans to change this behavior.
Further, any code that has side effects in a property access is asking for trouble. It is error-prone, highly unintuitive and unexpected from a developers point of view. Essentially, regardless of the use of uglify and the option in question, a developers application will end up with unexpected behavior at some point as a result of a library ignoring general best practices.
And finally, never break code is an impossible metric. In many languages, use of exotic constructs will result in broken code when full optimizations are enabled. If code follows recommended practices full optimization can be enabled.
yGuy commentedon Sep 5, 2018
For what is it an important optimization to remove code that has side-effects? How great are the gains that you get from this "optimization"? Are they truely worth it asking for troubles? Can you please elaborate on the importance of this optimization.
"Any code that has side effects in a property access is asking for trouble" - sorry, but this statement is just ignorant. That's not true in the general case. Any compliant JavaScript engine and any code mangling tool that is worth its name can deal with this case properly and it is a bug if it cannot. Any code that does this and which will be "optimized" by this tool is asking for trouble, for sure. There are a great number of very valid use-cases for this. If you don't like side-effects, then don't use JavaScript but some functional language. However as a tool author you should never force your preferences onto all third party library authors.
It is fine to restrict code in Angular or in code that you have control over, but if you want to support JavaScript, then you should at least make it possible to configure the "optimization" process not to break code. UglifyJS does have this option, why don't you just turn it on?
"Never break code" may be impossible, but always breaking code that is perfectly valid and part of a very widely used language construct is not acceptable. Make this an opt-in, or at the very least an opt-out, but don't leave it broken for the rest of the internet.
clydin commentedon Sep 5, 2018
As mentioned previously, it is opt-out. Advanced optimizations (i.e., build optimizer) can be disabled if desired. Considering the small number of cases of this actually causing an issue (and not defects in uglify itself) and the ability to manually modify the setting if absolutely required via a post install step, there are no plans to change this.
Please note that this statement "Any code that has side effects in a property access is asking for trouble" was in reference to users of a library that decides to take this approach as it is incredibly unexpected to have a property access perform additional side effects in a production scenario. Further, functional versus imperative is not a relevant distinction. Essentially every object-oriented language's best practices recommends not causing side effects within get accessors.
yGuy commentedon Sep 6, 2018
I totally agree with you, here:
But this is not what the optimizer is doing. It is not deciding to optimize your code that is using a library and fixes it for you (which really should not be the job of an optimizer, IMHO). It is changing the library and the way third party code behaves that the developer does not have control over. There is no way for most devs to "fix" third party code, just like it is very difficult for most of us to fix the angular tool chain.
I have seen quite a number of samples of this problem and believe me, these weren't just broken designs with unexpected side-effects. These were optimized pieces of code that resulted in very expected side effects inside the libraries. The problem with the optimization is that the expected effects don't happen anymore.
Please reconsider. I am happy to tell my team to create a pull-request. I would just like to spare the effort if the pull-request will be declined, anyway, because someone thinks the current solution is "better" in some way (for which no proof whatsoever has been shown, here, so far).
Would you be fine with a pull request that does:
set the default value to one that does not remove "unused" getters
by default does not perform breaking optimizations in node module dependencies
makes it possible to configure the setting
makes it possible to configure the setting per node module dependency
clydin commentedon Sep 6, 2018
Effort would most likely be better spent updating libraries/packages that are not following the recommended guidelines for advanced optimizations.
However, the team would be willing to consider a PR if a list was provided of libraries/packages that were not willing, or unable, to update their code to follow the recommended guidelines.
fbernhard commentedon Sep 6, 2018
@clydin
May I kindly ask you to post the link to the official Angular "recommended guidelines for advanced optimizations"? Thank you.
yGuy commentedon Sep 7, 2018
@clydin - I always thought that Angular wanted to benefit from the node.js community and not dictate the community how they think JavaScript works and what subset of JavaScript should be supported in their opinion.
That is one of the most ignorant statements I have ever read: "Why should we fix our software when everyone else in the internet can just adjust (not fix - it is not broken) their codes. If you continue to follow this road, then this is the beginning of the end of angular.
If it only was about angular specific libraries. This is about all third party code on the internet and with the huge dependency trees you get nowadays (think leftpad), making sure that all your dependencies in your angular-cli application follow the NG-Script subset of EcmaScript will be too much effort for most devs and they will decide to either ditch the "optimization" or angular as a whole and switch to a framework that works with the community and not against it.
Sorry for the rant, but this is really the worst kind of reaction and attitude I could imagine, here. I always thought good of angular, but this really makes me doubt the whole project leadership.
Find here a list of libraries that are unlikely to be willing to "update their code to follow the recommended guidelines of the angular devs, and who would rather stick to the EcmaScript standard". There might be a tiny fraction of about 0.1% or less of libraries on this list that have been included in error, feel free to fix it for the rest of them.
dschoeni commentedon Oct 9, 2018
We use libraries like bitcoinjs quite often in our projects, and as Ionic has now migrated to Angular-CLI (which I think is a great change), the overly strict uglifying behavior breaks our application at runtime. See this issue thread: bitcoinjs/bitcoinjs-lib#959
At least give us an option to properly configure the uglifier - It is one of the configuration flags that makes absolutely sense I think, in order to be compatible with the vast ecosystem provided by npm.
shileen commentedon Oct 23, 2018
I am really unable to figure out why this would be a stance. Giving finer control over the settings shouldn't be a matter of this much debate. I as a developer would definitely want the choice to opt out of a feature that causes some of third party code to break.
I always thought Angular was supposed to be an inclusive community and not an exclusive one. As a developer, a lot of things would be out of my control when it comes to third party library. The framework should be there to support me in these situations by at least allowing us more control over the build process.
I decided to actually go ahead and test the size differences between bundles generated by three different approaches.
So I do have the option to have no loss in functionality by having only a 30kb increase in my bundle, but the recommended solution by angular is to turn off the optimizer completely and deal with my bundle being almost 3x the size.
I really can't wrap my head around this.
11 remaining items
mordka commentedon Nov 2, 2018
Based on this conversation:
Switching optimization off is NOT an option. How to create a custom webpack config that will change
pure_gettersand retain all other default options? Angular ecosystem supposed to save millions of manhours from all the repetitive tasks such as writing configuration files.fbernhard commentedon Nov 3, 2018
As a short term solution we do the following until we have time to create our own Angular build system:
to install https://github.com/ALMaclaine/replace
Then, in package.json, we added a new script "disable-pure-getters" and run it before we build for production (excerpt):
The build server runs
npm run build:prod. Super ugly, but it works with our Angular 6 version. You might need to change the path to the webpack-configs/common.js file for different Angular versions.shileen commentedon Nov 3, 2018
@fbernhard This somehow didn't work for me. The replace library was not working as expected. So if someone needs another workaround, I use patch-package tool to patch the library. Here's a good example of how to use that.
earshinov commentedon Jan 29, 2019
Since when closing tickets without resolution became a common practice? Please stop pretending that ugly workarounds are a solution and reopen the ticket.
impliment hack fix due to
adrianhara commentedon Apr 8, 2019
@alexeagle can you please consider an updated stance on this and reopen the issue? All the opinions (and thumbs up) above show the community wants this (seemingly easy to implement) change. Or can you at least provide a compelling reason not to do it?
fix(@angular-devkit/build-angular): remove pure_getters
fix(@angular-devkit/build-angular): remove pure_getters
fix(@angular-devkit/build-angular): remove pure_getters
fix(@angular-devkit/build-angular): remove pure_getters
angular-automatic-lock-bot commentedon Sep 9, 2019
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.