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
Disallow duplicated AST nodes #11807
Conversation
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 1e796b3:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/25897/ |
for (const bareSuper of bareSupers) { | ||
bareSuper.insertAfter(nodes); | ||
if (isFirst) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test legacy-class-prototype-properties/child-classes-properties
is failing if we change these to bareSuper.insertAfter(nodes.map(n => t.cloneNode(n)))
unconditionally. The current approach happens to make our test cases happy and also slightly faster because we get rid of redundant clone.
However, I think it reveals an issue which is not caught in our current test cases.
I suggest we address that in a separate PR or even let it stay in the backlog since it is related to legacy decorators only.
I am not familiar with decorators part, suggestions are definitely welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably failing because in the decorators plugin we use a WARNING_CALLS
WeakSet to track warnings: if we clone the node we cannot find it anymore in the set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some code style suggestions, but overall it looks good 👍
for (const bareSuper of bareSupers) { | ||
bareSuper.insertAfter(nodes); | ||
if (isFirst) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably failing because in the decorators plugin we use a WARNING_CALLS
WeakSet to track warnings: if we clone the node we cannot find it anymore in the set.
78ff568
to
1e796b3
Compare
As a follow-up to #7149, this PR enables duplicated node checks against all testcases and then fixes all duplicate AST errors.
I didn't add a regression test of #11506 because it is covered in this test case. I realized that the duplicated node checks are not effective when I am investigating #11506, which should have been captured by our test runner.