-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Conversation
// 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"]); |
There was a problem hiding this comment.
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.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52470/ |
if ( | ||
initializerPath.isIdentifier() && | ||
initializerPath.isReferencedIdentifier() | ||
) { | ||
ReferencedIdentifier(initializerPath, { |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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"]>;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
🤔
6f18348
to
32dfc31
Compare
Super: | ||
| ArrayExpression | ||
| ArrowFunctionExpression | ||
| AssignmentExpression | ||
| AssignmentPattern | ||
| AwaitExpression |
There was a problem hiding this comment.
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.
f7a4c4f
to
59bd8e0
Compare
In this PR we provide better
NodePath#parentPath
andNodePath#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.