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

Use NodePath#hub as part of the paths cache key #15725

Merged
merged 5 commits into from Jul 5, 2023

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 27, 2023

Q                       A
Fixed Issues? Fixes #6437
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This fixes #6437, implementing option 2. I solved the problem of "we call traverse with no hub" by adding support for traversing a node visiting also the top-level node itself, so that we can directly traverse the Program NodePath that already has the hub.

We might want to, at some point, expose that top-level traversal as an official API, such as NodePath#traverseSelf, but I'm intentionally keeping it under the radar now (even if technically the new capability is accessible from the third-party code).

I don't know if this fixes or not #12570, because it looks like that issue has already been fixed on main?

cc @robhogan this fixes the underlying issue of facebook/metro#906

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jun 27, 2023
@babel-bot
Copy link
Collaborator

babel-bot commented Jun 27, 2023

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/54782/

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Jun 27, 2023

I'm very confused by the "old Babel" e2e failure. That test is using @babel/core and @babel/traverse 7.0.0, so I do not see how this patch can affect it 🤔

EDIT: The unit test was actually using both old and new at the same time, and there are some problems with mixing traverse versions. Fixed by #14794.

@nicolo-ribaudo nicolo-ribaudo changed the title Use NodePath#hub as part of the cache key Use NodePath#hub as part of the paths cache key Jun 29, 2023
Copy link
Member

@liuxingbaoyu liuxingbaoyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better for us to use || instead of ?? in this PR?

@nicolo-ribaudo nicolo-ribaudo merged commit abb5a7c into babel:main Jul 5, 2023
53 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the fix-hub-cach branch July 5, 2023 09:21
@Tofandel
Copy link

Tofandel commented Jul 6, 2023

Posting this here to save some headaches to others

If anyone gets the error

Cannot read properties of undefined (reading 'addHelper')
    at Scope.toArray (@babel/traverse/lib/scope/index.js:464:36)
    at getSpreadLiteral (@babel/plugin-transform-spread/lib/index.js:21:20)

When building after upgrading babel from 7.22.6 to 7.22.7

Make sure to switch from @babel/plugin-proposal-object-rest-spread to @babel/plugin-transform-object-rest-spread in your package json

nicolo-ribaudo added a commit that referenced this pull request Jul 6, 2023
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out an approach to NodePath 'hub' object tracking with better cache behavior
5 participants