esbuild: esbuild minify fails some recent uglify-js tests

I have limited time this week so I was only able to fix some of the things so far.

My bad. I mistakenly assumed that the remaining test failures were there by design. No one truly cares about using await as an identifier, for example.

Also it doesn’t look like Rollup treats TDZ checks as a side effect.

I’d consider any lack of TDZ checking in Rollup to be a bug. But just using its REPL now I see it’s pretty common. It’s progressing though - rollup has recently added support for side effects for setters and getters, among other checks now.

A workaround to support TDZ with Rollup would be to use --no-treeshake and employ a minifier like uglify-js or terser that respects TDZ side effects. There’s a lot of overlap between Rollup optimizations and minifier optimizations. The only significant drawback with this approach is that webpack-style sideEffects: false code pruning would also be disabled, if the bundle happens to significantly rely upon it. The minifier wouldn’t be able to drop the side-effect code for bundled sideEffects: false modules - unless pure annotations were used.

Edit: terser does not reliably respect TDZ side effects:

$ node_modules/.bin/terser -V
terser 5.7.0

$ echo 'w; let w, x = 1+x, y = console.log(123), z;' | node_modules/.bin/terser --toplevel -mc
console.log(123);

uglify-js does however:

$ uglify-js -V
uglify-js 3.13.9

$ echo 'w; let w, x = 1+x, y = console.log(123), z;' | uglify-js --toplevel -mc
l;let l,o=1+o;console.log(123);

But I have no idea how to get it to produce .mjs suffixed outputs to work with node with top-level await.

The --out-extension feature is intended to be used for this.

But I think the failing destructuring catch scope test case issue_4420 was missed in the noise.

It wasn’t missed. That’s part of why this issue is still open. I have limited time this week so I was only able to fix some of the things so far.

By the way, is Rollup’s approach to TDZ semantics preservation as seen above something you would consider to support in esbuild? It doesn’t appear to impose any significant code size nor run-time speed cost.

This should be what happens in esbuild too with --splitting enabled. But code splitting can also be disabled, which may not be a feature that Rollup has.

Also it doesn’t look like Rollup treats TDZ checks as a side effect. For example bundling let a = a with Rollup generates an empty file even though that code will throw an exception, which is a side effect.

You don’t have to convince me - I’m no fan of TDZ and appreciate why it’s a hindrance to bundling. I’m just saying it should be documented, that’s all. Perhaps you already have.

One could make the argument that avoiding triggering V8/Chrome bugs is not much different than avoiding the Safari JS engine TDZ slowness with correct ES code.

Yes, one could argue that. However, it seems different to me because of the extreme scale of the problem (the VM locks up for many seconds and it affects virtually every modern code base due to the use of class and let/const). Although it only affects top-level variables so you could potentially argue that the minifier shouldn’t consider references side-effect free if they are either non-top-level or maybe inside a try/catch or something. I could see potentially making a change like that.

I’ve also noticed failures related to https://tc39.es/proposal-template-literal-revision/.

Yeah. I’m aware of that one but haven’t fixed it yet because no one has asked for it. I should probably fix it though. It comes up in test262 as well.

By the way the failures due to await are because of esbuild’s top-level await support. The parser does not make a distinction between the script and module goals so it always parses top-level await. This hasn’t ever been an issue with real-world code and is basically only a problem for conformance tests. But I think it’s an acceptable trade-off.

I have fixed the following issues so far, although there are still more issues to be fixed. Here is the text from the release notes:

• The in operator is now surrounded parentheses inside arrow function expression bodies inside for loop initializers:

  // Original code
  for ((x => y in z); 0; ) ;
  
  // Old output
  for ((x) => y in z; 0; ) ;
  
  // New output
  for ((x) => (y in z); 0; ) ;
  

  Without this, the in operator would cause the for loop to be considered a for-in loop instead.

• The statement return undefined; is no longer minified to return; inside async generator functions:

  // Original code
  return undefined;
  
  // Old output
  return;
  
  // New output
  return void 0;
  

  Using return undefined; inside an async generator function has the same effect as return await undefined; which schedules a task in the event loop and runs code in a different order than just return;, which doesn’t hide an implicit await expression.

• Property access expressions are no longer inlined in template tag position:

  // Original code
  (null, a.b)``, (null, a[b])``;
  
  // Old output
  a.b``, a[b]``;
  
  // New output
  (0, a.b)``, (0, a[b])``;
  

  The expression a.b`c` is different than the expression (0, a.b)`c`. The first calls the function a.b with a as the value for this but the second calls the function a.b with the default value for this (the global object in non-strict mode or undefined in strict mode).

• Verbatim __proto__ properties inside object spread are no longer inlined when minifying:

  // Original code
  x = { ...{ __proto__: { y: true } } }.y;
  
  // Old output
  x = { __proto__: { y: !0 } }.y;
  
  // New output
  x = { ...{ __proto__: { y: !0 } } }.y;
  

  A verbatim (i.e. non-computed non-method) property called __proto__ inside an object literal actually sets the prototype of the surrounding object literal. It does not add an “own property” called __proto__ to that object literal, so inlining it into the parent object literal would be incorrect. The presence of a __proto__ property now stops esbuild from applying the object spread inlining optimization when minifying.

• The value of this has now been fixed for lowered private class members that are used as template tags:

  // Original code
  x = (new (class {
    a = this.#c``;
    b = 1;
    #c() { return this }
  })).a.b;
  
  // Old output
  var _c, c_fn, _a;
  x = new (_a = class {
    constructor() {
      __privateAdd(this, _c);
      __publicField(this, "a", __privateMethod(this, _c, c_fn)``);
      __publicField(this, "b", 1);
    }
  }, _c = new WeakSet(), c_fn = function() {
    return this;
  }, _a)().a.b;
  
  // New output
  var _c, c_fn, _a;
  x = new (_a = class {
    constructor() {
      __privateAdd(this, _c);
      __publicField(this, "a", __privateMethod(this, _c, c_fn).bind(this)``);
      __publicField(this, "b", 1);
    }
  }, _c = new WeakSet(), c_fn = function() {
    return this;
  }, _a)().a.b;
  

  The value of this here should be an instance of the class because the template tag is a property access expression. However, it was previously the default value (the global object in non-strict mode or undefined in strict mode) instead due to the private member transformation, which is incorrect.

It’s up to you. TDZ is a part of the ES spec. One could make the argument that avoiding triggering V8/Chrome bugs is not much different than avoiding the Safari JS engine TDZ slowness with correct ES code. uglify-js does manage to preserve TDZ semantics while providing good minification.

I’ve also noticed failures related to https://tc39.es/proposal-template-literal-revision/.