Avoid reusing AST nodes #4309
Avoid reusing AST nodes #4309
Conversation
|
Awesome! Thanks for working on this. Would it be possible to add a regression test for #3856. It should be as simple as adding a file to https://github.com/facebook/relay/tree/main/packages/babel-plugin-relay/__tests__/fixtures |
|
Also, babel-check-duplicated-nodes looks pretty simple and pretty stable. Could we just vendor that one function instead of taking on the additional dependency? |
eoin
force-pushed
the
3856
branch
2 times, most recently
from
f14f6fc
to
f41b61e
Compare
|
@captbaritone thanks for the review. I hadn't spotted that there's also @babel/helper-check-duplicate-nodes which appears to have changed more over time, has test coverage and is used by babel itself. I'm inclined to depend on that, instead of vendoring. What do you think? Regarding a regression test; the fixture + snapshot approach is tricky, in that for the issue to manifest we need to add something to the transform chain that relies on AST node uniqueness. That's doable, but before I go down that path, is it worth it? |
|
Makes sense. I'll import this as is. Thanks again! |
|
@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@captbaritone merged this pull request in 39ebc1c. |
Reusing AST nodes can result in subtle bugs, for example this issue is caused by AST node reuse breaking memoization in the
@babel/plugin-transform-modules-commonjsplugin.The first commit here introduces @babel/helper-check-duplicated-nodes which breaks a number of tests. The second commit fixes those tests.
Fixes #3856.