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

Added type-level mapping between aliases and nodes that have that alias. #9110

Merged

Conversation

cameron-martin
Copy link
Contributor

@cameron-martin cameron-martin commented Dec 1, 2018

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

Problem

The TypeScript types for Visitor in @babel/traverse do not allow node aliases as properties. These cannot be added with the current contents of the auto-generated TypeScript definitions for @babel/types.

Solution

If @babel/types had a type-level mapping between aliases and the node types that have those aliases, e.g:

export interface Aliases {
  Function: Function;
  Expression: Expression;
  LVal: LVal;
  // ...etc
}

Then the Visitor type in the TypeScript definitions can be intersected with the following type:

{ [K in keyof Aliases]?: VisitNode<S, Aliases[K]> }

This would then allow using aliases as Visitor properties when using TypeScript.

This PR adds such an Aliases type.

Concerns

  • There are no tests for this, because it seems like the infrastructure for testing d.ts files is not set up. If tests are required then I can set this infrastructure up.
  • This does not add equivalent functionality to the flow types. I believe that if the same thing was added, then it could be used in the same way as the TypeScript types by using $ObjMap. Can somebody confirm what the situation with flow definitions for @babel/traverse are and whether this would be a useful addition to the flow types?

@babel-bot
Copy link
Collaborator

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

Copy link
Member

@danez danez left a comment

Choose a reason for hiding this comment

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

Fine with me

@danez danez added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Dec 3, 2018
@cameron-martin
Copy link
Contributor Author

What's the process for merging PRs? Specifically, how come this has been approved but not yet merged?

@nicolo-ribaudo nicolo-ribaudo merged commit 9e95da4 into babel:master Dec 19, 2018
@cameron-martin cameron-martin deleted the feature/babel-types-aliases-map branch December 19, 2018 21:44
cameron-martin added a commit to cameron-martin/DefinitelyTyped that referenced this pull request Dec 19, 2018
This is made possible by babel/babel#9110.

When the next version after v7.2.2 is released, the package.json can be updated and the build will no longer fail.
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
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 pkg: types PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[@babel/types]: Export type-level mapping between aliases and the types of nodes that have that alias.
5 participants