Unnecessary reassignment of exported class when also used in static class property

[aleclarson]

https://sucrase.io/#code=%0Aexport%20class%20B%20%7B%7D%0A%0Aexport%20class%20A%20%7B%0A%20%20static%20B%20%3D%20B%0A%7D

This is the line I'm referring to:

static __initStatic() {this.B = exports.B = B}

[aleclarson]

I'm also curious why __initStatic is necessary in the first place? Babel leaves the class property untouched.

Maybe you could add an option to emit code like Babel does in this case?

[alangpierce]

Hey @aleclarson, thanks for reporting! Looks like this specific bug is an issue with shadow/declaration detection and the implementation of live bindings in the imports transform. Normally if you have code like this:

export class Foo {}
Foo = "some string";

the second assignment actually updates both the variable and export Foo. Sucrase implements those sorts of export re-assignments as:

 class Foo {} exports.Foo = Foo;
Foo = exports.Foo = "some string";

But in this case, Sucrase is incorrectly classifying the static B = B as an export re-assignment. Classes are a bit of a nuanced case because they don't create a new scope (e.g. you can't do static x = 1; static y = x + 1;) but assignments should be treated differently from regular assignments. In the Sucrase code, the parser sets IdentifierRole for each identifier, and I bet this could be fixed by updating class field parsing to set that correctly. My gut reaction is that class field declarations should just be their own identifier role, but maybe there's an existing one that would make sense. It looks like the bug also affects regular class fields, not just static fields. I can try to come up with a fix, or if you'd like to try coming up with a PR, let me know.

One workaround is to disable the imports transform and target ESM, though I imagine that probably isn't a reasonable option.

I'm also curious why __initStatic is necessary in the first place?

This is due to Sucrase's behavior of transpiling class fields (including static fields) by default. You can actually disable this by passing in the option disableESTransforms when invoking transform, though unfortunately it's not hooked up well to the various integrations. I've been meaning to make that easier. I've also been meaning to do a 4.0 release that fully drops support for all of these ES transforms (including optional chaining) since they're not nearly as relevant these days. I'd love to hear that would cause issues for you or if you'd be in favor.

Unfortunately, disableESTransforms: true doesn't actually fix the issue here. The output in that case looks like:

class A {
  static B = exports.B = B
} exports.A = A;

So it's really a bug in identifier parsing and the imports transform, not related to the __initStatic details.