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

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

Open
aleclarson opened this issue Jun 27, 2022 · 2 comments

Comments

@aleclarson
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Owner

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.

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

No branches or pull requests

2 participants