Support multiple static blocks#12738
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/44285/ |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e002f53:
|
ddbc630 to
0c990d4
Compare
0c990d4 to
d661b37
Compare
JLHwung
left a comment
There was a problem hiding this comment.
It's been a while and I complete forget the context of this PR.
This PR looks good to me except that we should avoid shadowing upper private identifiers.
|
|
||
| export type ParseClassMemberState = {| | ||
| hadConstructor: boolean, | ||
| hadStaticBlock: boolean, |
There was a problem hiding this comment.
The parser types are not exported, so we are free to refactor here.
| return; | ||
| for (const path of body) { | ||
| if (!path.isStaticBlock()) continue; | ||
| const staticBlockPrivateId = generateUid(scope, privateNames); |
There was a problem hiding this comment.
The inserted unique staticBlockPrivateId may accidentally shadow private id defined on upper levels:
class C {
static #_;
constructor() {
class D {
static {
C.#_ = 42;
}
}
}
}The injected #_ = AIIFE(static block) will shadow #_ defined on C. Consider reuse the privateNameVisitorFactory in @babel/helper-create-class-features-plugin.
There was a problem hiding this comment.
Why don't we just use the filename + source location to generate a private name?
There was a problem hiding this comment.
That does not really solve the mentioned issue, just makes it way less likely to happen. I don't think this is a blocker and we can address that in another PR.
There was a problem hiding this comment.
We should probably consider tracking private identifiers in @babel/traverse's generateUid.
There was a problem hiding this comment.
Or we provide a new generateUniquePrivateKeyAPI since plain identifier does not conflict with private identifiers.
|
Let's do #12738 (comment) in a separate PR, since it happens regardless of multiple static blocks. |
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
Can you add an input/output test with multiple static blocks?
d661b37 to
e002f53
Compare
I will not mark this PR as ready until the upstream PR is merged.
The integration test about
new.targetis disabled because of #12737. Yet the test ofstatic-blocksis still valid because we transform static blocks to static private field initializers.