Fix JSXIdentifier handling in isReferencedIdentifier
#17503
Conversation
JLHwung
added
the
PR: Bug Fix 🐛
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/59905 |
| if (path.parentPath.isUpdateExpression()) continue; | ||
| // It will be handled after transforming the loop | ||
| if (path.parentPath.isFor({ left: path.node })) continue; | ||
| if (path.parentPath.isForXStatement({ left: path.node })) continue; |
There was a problem hiding this comment.
This is caught by the new typing because left does not present in ForStatement node. Here we can narrow isFor to isForXStatement.
| this: NodePath, | ||
| opts?: Opts<VirtualTypeAliases["Flow"]>, | ||
| ): this is NodePath<t.Flow>; | ||
| isExpression(this: NodePath): this is NodePath<t.Expression>; |
There was a problem hiding this comment.
The opts parameter is removed to align with the implementation.
| } | ||
|
|
||
| // check if node is referenced | ||
| return nodeIsReferenced(node, parent, this.parentPath.parent); |
There was a problem hiding this comment.
Previously this branch also handles the JSXIdentifier-in-JSXMemberExpression case, however at this point the opts are no longer checked against the JSXIdentifier node, so NodePath(<A.B/>).get(path to A).isReferencedIdentifier({ name: "NotA" }) would incorrectly return true. This is also fixed in the new implementation.
There was a problem hiding this comment.
Could you update the PR title so that it properly describes this in the changelog?
|
commit: |
JLHwung
changed the title
is* and assert* helpers typingJSXIdentifier handling in isReferencedIdentifier
| opts?: Options<VirtualTypeAliases["ReferencedIdentifier"]>, | ||
| ): boolean { | ||
| const { node, parent } = this; | ||
| if (!isIdentifier(node, opts) && !isJSXMemberExpression(parent, opts)) { |
There was a problem hiding this comment.
Previously we passed opts to isJSXMemberExpression, which will always return false when opts is e.g. { name: "foo" } because the JSXMemberExpression does not have a name property. This is fixed in the new implementation.
|
|
||
| for (const type of [...t.TYPES].sort()) { | ||
| output += `is${type}(this: NodePath, opts?: Opts<t.${type}>): this is NodePath<t.${type}>;`; | ||
| output += `is${type}(this: NodePath): this is NodePath<t.${type}>;`; |
There was a problem hiding this comment.
Here we provided an overload signature, because otherwise TS will throw
TS2684: The 'this' context of type 'NodePath | NodePath | NodePath | NodePath<...> | NodePath<...> | NodePath<...>' is not assignable to method's 'this' of type 'NodePath'.
Type 'NodePath' is not assignable to type 'NodePath'.
Property 'expression' is missing in type 'ClassMethod' but required in type 'ArrowFunctionExpression'.
in line 1114
babel/packages/babel-traverse/src/scope/index.ts
Lines 1103 to 1114 in 092d2e2
I think we may have to do it for the t.is* helpers as well.
| // Signature overload to avoid issues like https://github.com/babel/babel/pull/17503#discussion_r2325598609 | ||
| `export function is${type}(node: t.Node | null | undefined): ${result};`, | ||
| `export function is${type}<Opts extends Options<t.${type}>>(node: t.Node | null | undefined, opts?: Opts | null): ${resultWithOpts};`, | ||
| `export function is${type}<Opts extends Options<t.${type}>>(node: t.Node | null | undefined, opts?: Opts | null): ${resultWithOpts} { |
There was a problem hiding this comment.
This last one can probably just return boolean now, since its type is not actually exposed externally but only used to check the return type.
github-actions
bot
added
the
outdated
4 participants
(NodePath("<div.A>").get(path to div).isReferencedIdentifier({ name: "div" })returnsfalsewhileNodePath("<div.A>").get(path to div).isReferencedIdentifier()returnstrue.In this PR we improved the
is*andassert*helper typings: The assertion type is now narrowed by the providedoptionskey, given that theoptionskey is a partial AST match. We also excluded thetypeproperty in theoptionskey since it makes no sense to providetypein the isHelper: e.g.t.isFor(node, { type: "ForStatement" })should have beent.isForStatement(node).The improved typings yields better helper usage in the codebase and we can remove a few
@ts-expect-error.Also fixed a
isReferencedIdentifierimplementation bug where we passedoptsto the parent AST helper. Previously it will yield very confusing behaviour as mentioned above and I think it should be fixed in a patch release.