Production build optimization breaks code

[yGuy]

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).

[psamusev]

I also have a problem with buildOptimizer: true in case of using custom decorators. In result code I don't have them assignable for classes

[filipesilva]

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_getters setting, 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 optimization and buildOptimizer.

A similar example is how we use the mangle option 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 mangle and pure_getters provided 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]

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]

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]

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]

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]

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]

I totally agree with you, here:

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.

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]

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]

@clydin

May I kindly ask you to post the link to the official Angular "recommended guidelines for advanced optimizations"? Thank you.

[yGuy]

@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.