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

Classes with static attributes and blocks are broken under --target=es2022 #2800

Closed
pvande opened this issue Jan 6, 2023 · 4 comments
Closed
Labels

Comments

@pvande
Copy link

pvande commented Jan 6, 2023

I haven't been able to fully track down this issue, but here's what I know.

esbuild version: 0.16.14

Input:

class Foo {
  static {
    this.prototype.thing = "value"
  }
}

class Bar {
  static thing = "value"
}

class Baz {
  static thing = "value"
  static {
    this.prototype.thing = "value"
  }
}

// This exists only to prevent dead code removal.
console.log(Foo, Bar, Baz)

The specific classes, properties, and values aren't important.

When esbuild test.js is run directly, these classes are compiled as follows:

  var _Foo = class {
  };
  var Foo = _Foo;
  (() => {
    _Foo.prototype.thing = "value";
  })();

  var Bar = class {
  };
  __publicField(Bar, "thing", "value");

  var _Baz = class {
  };
  var Baz = _Baz;
  __publicField(Baz, "thing", "value");
  (() => {
    _Baz.prototype.thing = "value";
  })();

This interpretation seems completely sensible, and functions as expected. The same results occur when run with --bundle.

When esbuild --target=esnext test.js is run, the output is (effectively) the same as the input, as might be expected.

When the --bundle and --target=esnext options are combined, however, the output is the following (commentary my own):

  // This seems a reasonable compromise.
  var Foo = class {
    static {
      this.prototype.thing = "value";
    }
  };

  // This doesn't seem like it should be necessary, but it still works.
  var Bar = class {
  };
  __publicField(Bar, "thing", "value");

  // This doesn't work.
  var _Baz = class {
    static {
      // This line is executed before the class body has been closed, so
      // the `_Baz` variable is still `undefined`, and this line fails.
      _Baz.prototype.thing = "value";
    }
  };
  var Baz = _Baz;
  __publicField(Baz, "thing", "value");

It appears that the behavior of rewriting static blocks for older browsers is behaving inconsistently with --target=esnext (or --target=2022) — standalone static blocks are neither rewritten nor extracted, but when a static attribute is introduced, the blocks are only rewritten and not extracted.

(Interestingly, this also seems to reveal an issue in the documentation — the target option is documented as defaulting to esnext, but there's clearly a change in behavior when the value is explicitly provided.)

@pvande pvande changed the title Static Attributes and Static Blocks are incompatible in certain cases Classes with static attributes and blocks are broken under --target=es2022 Jan 10, 2023
@hyrious
Copy link

hyrious commented Jan 11, 2023

Static fields are always lowered to avoid safari TDZ issue in bundle mode.

So the problem is: when enabled lowering for class-static-field, class-static-blocks should also be lowered too to avoid executing order problem.

On the other hand, referring class name in static block is ok only when the class name is declared via class <name> {} instead of var <name> = class {}.

class A { static { console.log(A) } } // class A {}

var A = class { static { console.log(A) } } // undefined

Note that this behavior only affects top level classes, which means a workaround when you put the class declaration in some scope (see it live):

let A = /*@__PURE__*/ (() => class A {
	static foo = 1 // will not be lowered
})()

@robpalme
Copy link

robpalme commented Feb 5, 2023

Static fields are always lowered to avoid safari TDZ issue in bundle mode.

@hyrious Wow - I am very curious. Where can I learn more about the Safari TDZ issue?

My understand was that TDZ is consistently specified behavior and therefore I'm surprised to hear it might be acting differently in one browser.

Edit: Ok I looked into the backstory and think there might be a more general issue.

@71
Copy link

71 commented Feb 9, 2023

We really need to be able to turn this off. Outside of producing pretty unreadable code (that's still visible when doing toString(), despite setting name; granted it's rarely useful but still a bummer), this breaks static {} blocks with mutually dependent classes (I'm guessing accessing this is a common thing to do in static blocks). I would happily take reduced performance over a lowering from this to undefined.

@71
Copy link

71 commented Jun 17, 2023

Looks like 0.18.4 no longer rewrites classes as described in this issue. I haven't tried it in my own project (since I rewrote it in a way that doesn't use static blocks, though it's less clean than it used to be), but hopefully that fixes it if I switch back to it in the future. Using the playground, I also found out that 0.18.2 fixed additional issues.

Thank you for the fix (and for making the fantastic esbuild in the first place)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants