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

[babel 8] Reland "Use NodePath#hub as part of the paths cache key" #15759

Merged
merged 4 commits into from
Aug 2, 2023

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jul 6, 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 is just a revert of the revert of #15725, it's not fixed yet.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 6, 2023

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

@nicolo-ribaudo
Copy link
Member Author

The angular error is caused by the caching changes, and not by the changes in babel-core to traverse the Program node with the existing parent path.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Jul 6, 2023

Ohh :/

The problem is caused by using an old version of @babel/core with newer versions of @babel/traverse, since older versions of Babel core obviously do not have the changes in packages/babel-core/src/transformation/index.ts. And angular pins @babel/core to 7.22.5.

@alan-agius4 regardless of this bug on our side, what is the reason for pinning @babel/core, given that all its internal dependencies can get updated? In theory it should not cause problems because we aim at having every new version compatible with all the combinations of older versions of other packages, but in practice allowing @babel/core to upgrade together with the other Babel packages makes it less likely to find compatibility problems.

@nicolo-ribaudo
Copy link
Member Author

This should fix the compat problem, tested locally. Let's see if it fixes the angular-cli integration test.

The fix/check is very bad, but it's self-contained and designed to run as less as possible (it only runs when calling into @babel/traverse through one of the methods used by @babel/core, and only when those methods are not called recursively via @babel/traverse).

@alan-agius4
Copy link

alan-agius4 commented Jul 7, 2023

In general we pin all of our direct dependencies to have a more stable releases. By pinning all of our dependencies we reduce compatibility issues, bugs and other breaky behaviour as some packages don’t follow semver. Over the years this did prove to make the Angular CLI more stable especially for LTS versions.

However, in this case though pinning did cause a problem.

@nicolo-ribaudo
Copy link
Member Author

This is ready for review, but the code is incredibly ugly.

Performance wise it's ok: the new Error().stack check to see if our caller is @babel/core only happens once (per compiled file) when calling @babel/traverse from old versions of @babel/core or from programs using @babel/traverse directly.

The angular-cli (breaking) test is failing because the workaround only applies to Babel 7, but it will start passing once @angular/cli updates their @babel/core version.

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review July 7, 2023 09:41
@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jul 7, 2023
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

🤦

@liuxingbaoyu
Copy link
Member

Could we possibly make it only land in babel8?
The method based on stack detection doesn't seem very elegant, especially if someone disabled the stack.
This bug has been around for ages, and it should be acceptable to defer to babel8. Even I worry that someone is relying on the wrong behavior. 🤦‍♂️

@nicolo-ribaudo nicolo-ribaudo changed the title Reland "Use NodePath#hub as part of the paths cache key" [babel 8] Reland "Use NodePath#hub as part of the paths cache key" Jul 18, 2023
@nicolo-ribaudo
Copy link
Member Author

I updated the PR to only land the breaking change in Babel 8.

@nicolo-ribaudo nicolo-ribaudo added babel 8 and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Aug 2, 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.

Thanks!

@nicolo-ribaudo nicolo-ribaudo merged commit 65a6bca into babel:main Aug 2, 2023
56 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the reland-hub-fix branch August 2, 2023 11:53
@nicolo-ribaudo nicolo-ribaudo added babel 8 PR: Bug Fix (next major) 🐛 A type of pull request used for our changelog categories for next major release and removed babel 8 labels Aug 8, 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 Nov 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 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 (next major) 🐛 A type of pull request used for our changelog categories for next major release
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