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

Make NodePath<T | U> distributive #16439

Merged
merged 1 commit into from Apr 21, 2024

Conversation

nicolo-ribaudo
Copy link
Member

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

This pull request makes it so that the types NodePath<t.Identifier | t.StringLiteral> and NodePath<t.Identifier> | NodePath<t.StringLiteral> are equivalent. This makes our path.is* able to better perform type narrowing:

if (path.isIdentifier()) {
  // path is NodePath<t.Identifier> -- this already worked
} else {
  // path is NodePath<t.StringLiteral> -- this works thanks to this PR
}

It also makes #16430 slightly faster, because it reduces the number of possible NodePath<...> types (because NodePath<t.Identifier> | NodePath<t.StringLiteral> and NodePath<t.Identifier | t.StringLiteral> are now the same type).

Unfortunately it makes it more common for .ensureBlock() to not work, since asserts return types do not work on unions (microsoft/TypeScript#44212). I consider this acceptable, since path.is* are much more common.

@nicolo-ribaudo nicolo-ribaudo added PR: Internal 🏠 A type of pull request used for our changelog categories area: typescript labels Apr 19, 2024
@babel-bot
Copy link
Collaborator

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

@@ -43,7 +43,7 @@ class NodePath<T extends t.Node = t.Node> {
this.scope = null;
}

declare parent: t.ParentMaps[T["type"]];
declare parent: t.Node;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does parent path type narrowing still work in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because it's properly defined further down in the NodePath interface.

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.

Amazing!
I've tried something similar before and failed. :)

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.

Awesome work.

@nicolo-ribaudo nicolo-ribaudo merged commit 4d8b2d0 into babel:main Apr 21, 2024
50 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the ts-nodepath-distributive branch April 21, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: typescript PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants