Fix NodePath.referencesImport for JSXMemberExpression #14403
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51599/ |
swandir
force-pushed
the
referencesImport-JSXMemberExpression
branch
from
43a1eb2
to
e072ba9
Compare
swandir
changed the title
| this: NodePath<t.Expression>, | ||
| this: NodePath, |
There was a problem hiding this comment.
Because JSXMemberExpression is not an Expression
There was a problem hiding this comment.
We can mark it as NodePath<t.Expression | t.JSXMemberExpression>
There was a problem hiding this comment.
Sure, done
Though I wonder what's the point of narrowing the type of this here? all the other methods accept NodePath 🤔
There was a problem hiding this comment.
Uhm yes that's a good observation. This method always work, it just return false for other node types.
You are right, NodePath is good enough here. Sorry!
There was a problem hiding this comment.
Yeah, I had a thought that making this type narrower doesn't make it more convenient for the TypeScript user, since they either would already be in a context where the type matches (e.g in a visitor) and won't notice, or would be forced to preclude the call with a manual check, which is performed inside the method anyway.
Sure, done
Reverted 😃
There was a problem hiding this comment.
Although...
function f(path: NodePath) {
path.referencesImport("moduleSource", "importName");
}this does not produce a TS error now 🤔
There was a problem hiding this comment.
Yeah that's correct, right? It can return false for every unsupported node type, but it always knows what to return.
There was a problem hiding this comment.
I mean, if a function is typed to accept only a certain type as this, I'd expect TypeScript to prevent me from calling it on anything incompatible
There was a problem hiding this comment.
Ah! I forgot that definitions currently are published separately, and they differ. So this function signature does not affect third party usage. Mystery solved 😅
| (this.isMemberExpression() || this.isOptionalMemberExpression()) && | ||
| (this.node.computed | ||
| ? isStringLiteral(this.node.property, { value: importName }) | ||
| : (this.node.property as t.Identifier).name === importName) | ||
| (this.isJSXMemberExpression() && | ||
| (this.node.property as t.JSXIdentifier).name === importName) || | ||
| ((this.isMemberExpression() || this.isOptionalMemberExpression()) && | ||
| (this.node.computed | ||
| ? isStringLiteral(this.node.property, { value: importName }) | ||
| : (this.node.property as t.Identifier).name === importName)) |
There was a problem hiding this comment.
JSXMemberExpression is never computed
nicolo-ribaudo
added
pkg: traverse
area: jsx
PR: Fixes failing main
PR: Bug Fix 🐛
swandir
force-pushed
the
referencesImport-JSXMemberExpression
branch
from
49885fa
to
1cc2a3e
Compare
github-actions
bot
added
the
outdated
As described in the issue,
NodePath.referencesIssueis only missing a check forJSXMemberExpressionto support it.