Skip to content

Make NodePath<T | U> distributive - #16439

Merged
nicolo-ribaudo merged 1 commit into
babel:mainfrom
nicolo-ribaudo:ts-nodepath-distributive
Apr 21, 2024
Merged

nicolo-ribaudo merged 1 commit into
babel:mainfrom
nicolo-ribaudo:ts-nodepath-distributive

Conversation

@nicolo-ribaudo

Copy link
Copy Markdown
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
@nicolo-ribaudo
nicolo-ribaudo force-pushed the ts-nodepath-distributive branch from 0fe41a6 to c62c6d6 Compare April 19, 2024 17:49
@babel-bot

Copy link
Copy Markdown
Collaborator

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

}

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

Copy link
Copy Markdown
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
Copy Markdown
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.

@liuxingbaoyu liuxingbaoyu left a comment

Copy link
Copy Markdown
Member

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. :)

@JLHwung JLHwung left a comment

Copy link
Copy Markdown
Contributor

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
@nicolo-ribaudo
nicolo-ribaudo deleted the ts-nodepath-distributive branch April 21, 2024 13:48
@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 Jul 22, 2024
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue 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.

4 participants