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

Simplify helper dependencies #15771

Merged
merged 6 commits into from Jul 12, 2023

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jul 11, 2023

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

In this PR we simplify the helper dependencies by replacing imports from @babel/types, @babel/template and @babel/traverse to a peer dependent @babel/core. It probably also fixes #15765 but we will need confirmation once the proposed changes are published.

Possible story on #15765:

Given node_modules layouts as followed. Here dotted link means the start point has declared dependency on the end point, and solid link means the start point has a folder named the end point.

graph TD;
    A[node_modules]-->B["@babel/traverse@7.16"];
    A-->C["@babel/types@7.16"];
    A-->F["@babel/core@7.16"];
    F -.-> C
    F -.-> B
    A-->ECH["ember-composable-helpers@5"]
    ECH-.->D
    ECH-.-> F
    A-->D["ember-cli-babel@7"];
    D-.-> F
    D-.-> I
    D-->E[node_modules];
    E-->G["@babel/traverse@7.22.8"];
    E-->H["@babel/types@7.22"];
    E-->I["@babel/preset-env@7.22"]
    I -.-> BTC
    E-->BTC["@babel/plugin-transform-classes@7.22"]
    BTC-.->BRS
    E-->BRS["@babel/helper-replace-supers@7.22"]
    BRS-.-> G
    G -.-> H

The helper-replace-supers calls traverse.visitors.merge from @babel/traverse to merge the environment visitor with a visitor hooking on the Scopeable alias .

const unshadowSuperBindingVisitor = traverse.visitors.merge<{
refName: string;
}>([
environmentVisitor,
{
Scopable(path, { refName }) {
// https://github.com/Zzzen/babel/pull/1#pullrequestreview-564833183
const binding = path.scope.getOwnBinding(refName);
if (binding && binding.identifier.name === refName) {
path.scope.rename(refName);
}
},
},
]);

After #15702 is merged in @babel/traverse@7.22.8, visitors.merge will explode the alias into node types. Here Scopeable is expanded to ClassAccessorProperty and other AST nodes according to the latest @babel/types definition. At this time the visitor is not executed.

Now when the visitor is eventually invoked by ember-cli-babel, the root @babel/traverse is backed by an older version of @babel/types, which does not recognize ClassAccessorProperty and therefore an error is thrown.

So in this PR we replace all traverse usage with peer dependencies of @babel/core. It should at least alleviate the situation of duplicated libraries.

For end users, the best solution is still to deduplicate @babel/types instances in node_modules: It is more memory efficient and less prone to compatibility issues like this.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 11, 2023

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

@liuxingbaoyu
Copy link
Member

CI errors seem to be related?

@JLHwung
Copy link
Contributor Author

JLHwung commented Jul 12, 2023

Yes, CI error is related, I will investigate tomorrow.
CI is green again, 🤟

@JLHwung JLHwung force-pushed the simplify-helper-dependencies branch from ff09744 to 3e1e170 Compare July 12, 2023 13:43
@nicolo-ribaudo
Copy link
Member

When I tried to this in the past, I had problems with ESM and cycles.

i.e. if you have these files:

// @babel/core
export { x } from "@babel/helper-1";
export { y } from "@babel/helper-2";
// @babel/helper-1
import { y } from "@babel/core";
export let x = 1 + x;
// @babel/helper-2
export let y = 2;

it fails because y is in TDZ when evaluating @babel/helper-1, even if @babel/helper-2 is a transitive dependency of @babel/helper-1. I talked about this problem with some TC39 delegates that worked on the original ESM design, but it's very unlikely that the evaluation order will ever be changed.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Note: I'm also exploring a fix that doesn't rely on the shape of the dependencies tree.

@nicolo-ribaudo
Copy link
Member

Releasing this, hoping to fix the regression.

@nicolo-ribaudo nicolo-ribaudo merged commit 476f633 into babel:main Jul 12, 2023
56 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the simplify-helper-dependencies branch July 12, 2023 15:01
@JLHwung JLHwung added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jul 12, 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 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 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.

You gave us a visitor for the node type ClassAccessorProperty but it's not a valid type
4 participants