Optimization with esbuild breaks the code since 12.2.0

[yGuy]

🐞 Bug report

Command (mark with an x)

• new
• build
• serve
• test
• e2e
• generate
• add
• update
• lint
• extract-i18n
• run
• config
• help
• version
• doc

Is this a regression?

Yes, the previous version in which this bug was not present was: ~12.1.0

Description

Our application started to break in what we found literally hundreds of places in the source code when we (accidentally) switched to ~12.2.0

(in fact this happened on a project without a package lock file, that used ~12.0.5 for all angular dependencies, but @angular-builders/custom-webpack internally uses ^12.0.0 which resulted in yarn installing 12.2 of @angular-devkit/build-angular which is the first version using esbuild - side note: so be careful if you use yarn and builders/custom-webpack without a lock file as it will not honor the version ranges!)

What happens since version 12.2.0 is that during final optimization/minification esbuild often renames variables that are used locally with names that are already defined in an outer scope and which are still accessed from within the local to the outer scope. This almost always invalidates all references to the outer scope resulting in code that immediately breaks at runtime.

esbuild tries to use names for variables that are used most frequently, but for some reasons it does not always seem to check whether these names don't clash with other variable usages in the same scope.

The code that breaks has the following structure (not exactly this code):

(function($, f, t, n) {
    function b(i) {
        var $ = new $.ClassName();
        return $.prop.m(i)
    }
    window.e = b;
})(window.a,window.b,window.c);

(this is a kind of a function declaration that uses external dependencies and the other dependencies are injected via the $ parameter in the outer scope. Then these dependencies are used in functions that will later get "exported".)

In the original code, before minification indeed the outer $ is already named $, but the inner var $ is something else in the original code - similar to this:

(function($, f, t, n) {
    function b(i) {
        var something = new $.ClassName();
        return something.prop.m(i)
    }
    window.e = b;
})(window.a,window.b,window.c);

And of course, in the optimized code new $.ClassName() does not work, because $ is undefined at that point because of the variable (re-)declaration.

We have identified literally dozens of cases in our code where the code is broken after being optimized by the 12.2.0 build. It seems it assigns a new name to local variables, without properly checking whether they are already used in the scope.

Another (part of an) example for a minified, obviously broken code:
(again $ is being accessed/used before its initialized)

function a($){
   function Jt(i) {
                for (var $ = new $.C.NDB.$x, n = i.$f.$RFd(); n.$QT; n.$zY())
                    $.$m6(new $.IGA.T2(n.$bT));
   }
}

and we also have code where due to hoisting a var $ = something resulted in previous usages of $ in the same scope also yielding undefined errors.
Similar to this:

function a($){
   function x(p) {
                new $.C().foo();
                var $ = p;
                $.bar()
   }
}

🔬 Minimal Reproduction

That's the problem. We have not been able to strip down the code to reasonable size, yet. This seems to happen with larger code bases, only, so far. If we remove parts of the code before and after the broken parts, sometimes the error does not show (because a different, non-clashing variable renaming happens). We have not been able so far to reduce the code to below a few thousand lines of code.

We hope that someone who knows more about esbuild or the integration with the angular build has some idea what might be going so terribly wrong here. We manually forced esbuild to the newest available release (0.12.23 instead of the currently used 0.12.17), but the problem remained. Disabling optimization or switching to versions before 12.2.0 immediately fixes the problem.

I am aware that this bug might get closed because it is not easily reproducible, and I will continue to find a smaller repro, but at the moment the repro contains commercially licensed code and thus cannot be easily published.

You can reproduce the "not at all minimized use-case" by downloading the trial of yFiles for HTML, building the angular CLI example (check the README for details) with angular >=12.2.0 in "production mode" and the app will immediately break on startup with errors similar to the ones above. Looking through the generated code, there are dozens, maybe hundreds of broken/wrong variable renamings. With <12.2.0 everything works as expected.

🌍 Your Environment

See above. The below combination breaks and we found that the very last (at the time of writing) 12.1.x release works, whereas 12.2.0 breaks.

Angular CLI: 12.2.2
Node: 14.17.5
Package Manager: yarn 1.22.11
OS: win32 x64

Angular: 12.2.0
... common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1202.0
@angular-devkit/build-angular   12.2.0
@angular-devkit/core            12.2.2
@angular-devkit/schematics      12.2.2
@angular/cli                    12.2.2
@schematics/angular             12.2.2
rxjs                            7.1.0
typescript                      4.2.4

Anything else relevant?

Any input that might help us in reducing this to a smaller repro would be greatly appreciated. E.g. could we reduce this to just the esbuild step so that bisecting does not always take minutes for each step? Is there an easy way to tweak the esbuild configuration?

[clydin]

Can you cause this issue without any custom builders present (or @yworks/optimizer specifically)?
Also, it appears that @angular-devkit/build-angular is not on the latest patch release (currently: 12.2.2), can you try updating that as well?

[micschro]

@clydin yes, the same issue can be reproduced without the @angular-builders/custom-webpack builder (and therefore no @yworks/optimizer) and the latest @angular-devkit/build-angular release:

Angular CLI: 12.2.2
Node: 14.17.0
Package Manager: yarn 1.22.10
OS: win32 x64

Angular: 12.2.3
... common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1202.2
@angular-devkit/build-angular   12.2.2
@angular-devkit/core            12.2.2
@angular-devkit/schematics      12.2.2
@angular/cli                    12.2.2
@schematics/angular             12.2.2
rxjs                            7.1.0
typescript                      4.2.4

[LeonEck]

The same thing described here happened to the angular-oauth2-oidc library. It was resolved by swapping out the sha256 implementation: manfredsteyer/angular-oauth2-oidc#1120
The previous one also used variables in functions that were defined in an outer scope. Sometimes esbuild would reuse the name of a global variable inside one of these functions and therefore break the code.
I was only able to reproduce this in larger code bases. At one point, I could toggle between an erroneous and a correct build by adding a random variable somewhere in my code.
I tested every build of @angular-devkit/build-angular and this bug was definitely introduced with the usage of esbuild: da32daa
When you go into the sources of @angular-devkit/build-angular and comment out the esbuild transform call, so it only runs terser, the build works again. Some more information can be found in my comment here: manfredsteyer/angular-oauth2-oidc#1120 (comment)

I hope this is at least somewhat useful information, that this bug is not isolated to one project.

[clydin]

@LeonEck By any chance did you try the opposite, (i.e., disabling terser and only running esbuild)?
We had to lower the number of terser passes to 2 due to some very strange function inlining issues with terser (PR: #21324).
@yGuy If you have some additional time, can you try a production build with buildOptimizer disabled? This will cause terser to use only 1 pass.

[yGuy]

@micschro can you look into this, tomorrow, please?
@LeonEck - thanks for chiming in - this really looks like a nasty bug that will affect more projects. It's really difficult to debug and can come and go with even the slightest changes to your input sources - we found that the engine renames variables based on frequency of use, so if the names (or even just the number of similar characters in your code base!) changes, it could break again with the next build.
@clydin - is there an easy way to inspect the intermediate outputs (in between and after terser runs and before/after esbuild)?

[LeonEck]

@clydin I just tried this out with the latest angular version and the previous angular-oauth2-oidc library to force the broken build. I commented out the optimizeWithTerser call and returned the esbuildResult.code in the end. With that, the build is still broken in the same way. It still reuses a variable name that it shouldn't.

Setting buildOptimizer to false (without my modifications above) results in a working build.

@yGuy You can go into node_modules/@angular-devkit/build-angular/src/webpack/plugins/javascript-optimizer-worker.js. From there esbuild and terser are called (the code that esbuild is called with is in asset.code) and the results are stored in esbuildResult and terserResult.

[clydin]

@LeonEck Thank you for helping to diagnose the issue. Would it be possible for you to try out an additional change? If so, can you keep buildOptimizer enabled but manually change the terser settings in node_modules/@angular-devkit/build-angular/src/webpack/plugins/javascript-optimizer-worker.js so that instead of passes: advanced ? 2 : 1 it is just passes: 1. If that doesn't result in a working build can you also try forcing pure_getters: false. This should help narrow down why setting buildOptimizer to false fixes the issue.
Thank you again for your help.

[LeonEck]

@clydin Here are the results:

• Setting passes to 1 doesn't fix it.
• Forcing pure_getters to false (in addition to passes: 1) also doesn't fix it.
• For completeness, I also tried just setting pure_getters to false without changing passes. That also didn't fix it.

Let me know if there is anything else that I should test.

[micschro]

@clydin we ran our build again with buildOptimizer enabled and disabled. For us, it does not make a difference whether this option is enabled. Looking at the source, we noticed that the buildOptimizer option does not have any effect on whether esbuild is applied to the sources: esbuild will run no matter whether buildOptimizer is enabled or disabled. However, when we modify the sources to turn of just the esbuild call (and keep the terser call), the build result works correctly. So, it really looks like the issue is related to esbuild rather than the specific parameters used for terser.

I'm not sure why disabling buildOptimizer resulted in a working build for @LeonEck, though. Maybe another parameter disabled esbuild? (e.g. --optimization=false?)

We ran the builds with these angular versions today:

Angular CLI: 12.2.3
Node: 14.17.0
Package Manager: yarn 1.22.10
OS: win32 x64

Angular: 12.2.3
... cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1202.3
@angular-devkit/build-angular   12.2.3
@angular-devkit/core            12.2.3
@angular-devkit/schematics      12.2.3
@schematics/angular             12.2.3
rxjs                            7.1.0
typescript                      4.2.4

[LeonEck]

@micschro I am also confused why disabling buildOptimizer all of a sudden fixes the build. I remember this not being the case when I originally tested it two weeks ago (at that time I had to disable both buildOptimizer and optimization). Since this whole error only surfaces when exact conditions are met, and even slight changes to the input code toggle between the error appearing or disappearing, this could be a red herring.

[evanw]

I just came across this, and I'm the developer of esbuild. It looks like this is a bug in esbuild, likely having to do with the use of eval. I can reproduce it using the following code:

return function($) {
  function foo(arg) {
    return arg + $;
  }
  // "eval" prevents "$" from being renamed
  // Repeated "$" puts "$" at the top of the character frequency histogram
  return eval(foo($$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$))
}(2);

and the command esbuild example.js --minify-identifiers. The incorrectly-generated output looks like this:

return function($) {
  function foo($) {
    return $ + $;
  }
  return eval(foo($$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$));
}(2);

The problem is that the use of eval flags the variable $ as not able to be renamed, but it's not currently added to the internal list of names to avoid collisions with inside esbuild's renamer. It's added to this internal list if it's in a top-level scope but not if it's in a nested scope, which is a bug in esbuild that I can fix in the next release.

Note that use of direct eval with a bundler is highly discouraged even when the bundler handles it correctly because it disables minification of the file and also disables bundler optimizations. If any of these uses of direct eval are under your control, I encourage you to use other alternatives instead. Use of direct eval is almost always a mistake and when it's not, there are better more precise alternatives. See https://esbuild.github.io/content-types/#direct-eval for more information.

[alan-agius4]

Thanks @evanw for taking a look at this, much appreciated 😀

[clydin]

@evanw Thank you for the help and insight. I was trying to narrow down a minimal reproduction using the js-sha256 library and the eval usage there did look suspicious but didn't connect it to the frequency analysis.

[evanw]

The newly-released esbuild version 0.12.24 should fix this issue.

[chandansamanta]

One my application got broken and seems like this was causing it because it failing in prod and local working where optimization is off. This version should be marked as broken, as there this will break lots of applications and developers will break their head unnecessary as this is a very complicated issue to capture.

[LeonEck]

I just tested it by forcing esbuild to version 0.12.24 and that produces working builds!
@evanw Thanks a lot for being able to provide a fix that quickly.

[alan-agius4]

Thanks for the quick turnaround @evanw.

Closed via #21665

[angular-automatic-lock-bot]

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