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

Provide better parentPath typings #14739

Merged
merged 6 commits into from Jul 9, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jul 7, 2022

Q                       A
License MIT

In this PR we provide better NodePath#parentPath and NodePath#parent typings using a map from node types to its parent AST node types, generated from our AST definitions. There are no runtime overhead shipped to our end users because they are done in typings.

The new typings catches two babel types error because we used the parentPath info in certain plugins but forget to add babel types support.

This PR includes commits from #14737. I will rebase once that PR is merged.

// todo: provide a better parent type for Placeholder, currently it acts
// as a catch-all parent type for an abstract NodePath, s.t NodePath.parent must
// be a Node if type has not been specified
registerParentMaps("Node", ["Placeholder"]);
Copy link
Contributor Author

@JLHwung JLHwung Jul 7, 2022

Choose a reason for hiding this comment

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

Caveat: the Placeholder has a unique role here.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 7, 2022

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

if (
initializerPath.isIdentifier() &&
initializerPath.isReferencedIdentifier()
) {
ReferencedIdentifier(initializerPath, {
Copy link
Contributor Author

@JLHwung JLHwung Jul 7, 2022

Choose a reason for hiding this comment

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

isReferencedIdentifier ensures that this is either an Identifier or a JSXIdentifier, but ReferencedIdentifier can't handle JSXIdentifier.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jul 8, 2022

Choose a reason for hiding this comment

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

TS already know that initializerPath is a t.Expression, and t.Expression does not include t.JSXIdentifier.

Could we type isReferencedIdentifier like this, so that TS can automatically infer with initializerPath.isReferencedIdentifier() that initializerPath is a NodePath<t.Identifier>?

  isReferencedIdentifier<T extends t.Node>(
    this: NodePath<T>,
    opts?: object,
  ): this is NodePath<T & VirtualTypeAliases["ReferencedIdentifier"]>;

Copy link
Contributor Author

@JLHwung JLHwung Jul 8, 2022

Choose a reason for hiding this comment

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

In this way the isReferencedIdentifier query can not differentiate a NodePath union, i.e. NodePath<A> | NodePath<B> inferred from path.isA() || path.isB(), see also the typing errors in recent CI. I tried typings like

isReferencedIdentifier<T extends t.Node, T2 extends t.Node>(
    this: NodePath<T> | NodePath<T2>,
    opts?: object,
  ): this is NodePath<(T | T2) & VirtualTypeAliases["ReferencedIdentifier"]>;

but TS can't pick it up. Help is welcome. I think the intersection behaviour is more useful, so from now on I will ignore such typing errors.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jul 8, 2022

Choose a reason for hiding this comment

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

I wonder why our NodePath has the wrong variance and NodePath<A | B> is not inferred as the same as NodePath<A> | NodePath<B> 🤔

@JLHwung JLHwung force-pushed the refine-parent-path-typings branch 2 times, most recently from 6f18348 to 32dfc31 Compare Jul 8, 2022
Super:
| ArrayExpression
| ArrowFunctionExpression
| AssignmentExpression
| AssignmentPattern
| AwaitExpression
Copy link
Contributor Author

@JLHwung JLHwung Jul 8, 2022

Choose a reason for hiding this comment

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

This is incorrect. A Super's parent must be either a CallExpression or a MemberExpression. We should consider remove Super from Expression in Babel 8.

@JLHwung JLHwung force-pushed the refine-parent-path-typings branch from f7a4c4f to 59bd8e0 Compare Jul 8, 2022
nicolo-ribaudo
nicolo-ribaudo previously approved these changes Jul 8, 2022
@nicolo-ribaudo nicolo-ribaudo dismissed their stale review Jul 8, 2022

I didn't notice CI is failing

@liuxingbaoyu liuxingbaoyu added PR: Internal 🏠 A type of pull request used for our changelog categories area: typescript labels Jul 9, 2022
@JLHwung JLHwung merged commit 41582ee into babel:main Jul 9, 2022
35 checks passed
@JLHwung JLHwung deleted the refine-parent-path-typings branch Jul 9, 2022
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