Fix types for path.get("body.${index}")#17829
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/61070 |
| it("should support template literal expressions - number", () => { | ||
| const path = {} as NodePath<t.BlockStatement>; | ||
| const index: number = 0; | ||
| const statement = path.get(`body.${index}`); |
There was a problem hiding this comment.
I really hope that get only returns NodePath and not NodePath[].
Because technically path.get("x.${y}") could also return NodePath[], but that's rare and more complicated, and I'm hesitant to support it.
|
commit: |
JLHwung
left a comment
There was a problem hiding this comment.
Marked this PR as internal because it improves the typings. The path.get function is not affected.
|
This is more likely a fix in the next major release, because until then it would return |
path.get("body.${index}")path.get("body.${index}")
| : TravD<Node[K] & {}, R> | ||
| : never | ||
| : null | ||
| : `${any}` extends K |
There was a problem hiding this comment.
Is this any different from just string extends K?
There was a problem hiding this comment.
${any} can allow ${number}
There was a problem hiding this comment.
${any}can allownumber
It can allow any number casted to a string, right? Doesn't string also allow anything casted to a string?
I'm finding this very confusing overall: https://www.typescriptlang.org/play/?#code/DYUwLgBAhgXBAGASA3lAdgTwL7wgXggCJDoBnBAMwHsqVSwAnASzQHMcBuAKFEgCM4SVJhz4iJKOSH1mbTj3AQAxoJTpsuAsTIQZLVtyA
What about only supporting the ${number} case here, and users that have string-based template need to type them like
path.get("body.${foo as "body" | "somethingElse"})to get proper type? That would probably then also give them NodePath[] when needed.
There was a problem hiding this comment.
There's a string type test above, and it passed. :)
I don't know why TypeScript is complaining about assignment, but the following code works.
type a = `${any}` extends `${string}` ? true : false;
This code would be false, but we've already split it beforehand.
type c = `${any}` extends `x.${string}` ? true : false;
There was a problem hiding this comment.
There's a string type test above, and it passed. :)
I mean, something like
({} as t.FunctionDeclaration).get(`body.${"" as "body"}`)that, thanks to the explicit cast, returns NodePath[].
There was a problem hiding this comment.
This seems to work, and I added a test.
| it("should support template literal expressions - number", () => { | ||
| const path = {} as NodePath<t.BlockStatement>; | ||
| const index: number = 0; | ||
| const statement = path.get(`body.${index}`); |
There was a problem hiding this comment.
In this case, what is the result of Split<...>? Is it not ["body", number], same as it would be for Split<"body.0">?
There was a problem hiding this comment.
My bad, they should be the same.
ffe67b1 to
f007757
Compare
f007757 to
276667d
Compare
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
Thanks, this looks good to me now.
Fixes #1, Fixes #2I discovered this while working on #17828.