feat: add ReturnType parser#2317
Conversation
domoritz
left a comment
There was a problem hiding this comment.
Thank you. Looks good overall but the code feels more complex than it ought to be.
| } else { | ||
| // Case: ReturnType<SomeType["methodName"]> or other complex types | ||
| // Get the type directly from TypeScript's type system | ||
| const argType = this.checker.getTypeAtLocation(typeArg); |
There was a problem hiding this comment.
I really want to a lid the type checker if possible. What cases fail when we remove this branch?
There was a problem hiding this comment.
When removing the else branch completely:
test/valid-data/type-return-type-complex/main.ts(15,28): error TSJ - 100: Unknown node of kind "TypeReference"
Error: Unknown node of kind "TypeReference"
at ReturnTypeNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/ReturnTypeNodeParser.ts:98:19)
at ChainNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ChainNodeParser.ts:37:49)
at TypeAliasNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/TypeAliasNodeParser.ts:40:43)
at AnnotatedNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/AnnotatedNodeParser.ts:34:47)
at ExposeNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ExposeNodeParser.ts:23:45)
at CircularReferenceNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/CircularReferenceNodeParser.ts:24:43)
at ChainNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ChainNodeParser.ts:37:49)
at TopRefNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/TopRefNodeParser.ts:14:47)
at <anonymous> (/home/rockerboo/code/others/ts-json-schema-generator/src/SchemaGenerator.ts:31:39)
at Array.map (<anonymous>)
There was a problem hiding this comment.
Yeah, but what line of the example causes this?
There was a problem hiding this comment.
test/valid-data/type-return-type-complex/main.ts(15,28)
Specifically the node ReturnType
If you give me some more context of what to look at, I can take a deeper look to finding a better solution.
|
|
||
| throw new UnknownNodeError(node); | ||
| } catch (error) { | ||
| throw error; |
| } | ||
|
|
||
| // Fallback to type checking method | ||
| const type = this.checker.getTypeOfSymbolAtLocation(symbol, typeArg); |
There was a problem hiding this comment.
Same as below. In what cases do we need the fallback? Why can't we use the function parser instead of implementing a lot of custom logic here?
There was a problem hiding this comment.
Removing this fallback causes issues with implicit function return types. I added a test for this case.
Error: /home/rockerboo/code/others/ts-json-schema-generator/test-no-explicit-return.ts(5,34): error TSJ - 100: Unknown node of kind "TypeReference"
Error: Unknown node of kind "TypeReference"
at ReturnTypeNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/ReturnTypeNodeParser.ts:110:15)
at ChainNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ChainNodeParser.ts:37:49)
at TypeAliasNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/TypeAliasNodeParser.ts:40:43)
at AnnotatedNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/NodeParser/AnnotatedNodeParser.ts:34:47)
at ExposeNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ExposeNodeParser.ts:23:45)
at CircularReferenceNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/CircularReferenceNodeParser.ts:24:43)
at ChainNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/ChainNodeParser.ts:37:49)
at TopRefNodeParser.createType (/home/rockerboo/code/others/ts-json-schema-generator/src/TopRefNodeParser.ts:14:47)
at <anonymous> (/home/rockerboo/code/others/ts-json-schema-generator/src/SchemaGenerator.ts:31:39)
at Array.map (<anonymous>)
There was a problem hiding this comment.
Can we reuse some of the existing function parsing logic here?
There was a problem hiding this comment.
I tried to poke at it in terms of utilizing the FunctionNodeParser but since it's like an abstraction on top of functions I couldn't find a simple way of doing that. I am not fully aware of how this all works, so I do not know how to make it work well enough like this. Also we have it separated as "functions" but this is more of a data structure format (object, number, boolean, string) because of the ReturnType unless it returns a function (which might need to be further tested).
I can try to make it better but I am just not sure how to connect them.
I added more tests for the different function types though, and also tested methods.
|
I went a little further into return type of functions returning functions, but got more into a gray area and make this PR more complex. But it wasn't too bad to implement. const x = () => () => 1;
type X = ReturnType<typeof x>;Generics with return type of functions returning functions or methods returning functions was a a bit more complex. const x = <T extends string>() => <T>(): T => "hello";
type X = ReturnType<typeof x>; |
domoritz
left a comment
There was a problem hiding this comment.
@arthurfiorette could you help review this?
| try { | ||
| const typeName = ts.isIdentifier(node.typeName) ? node.typeName.text : node.typeName.getText(); | ||
| return typeName === "ReturnType" && node.typeArguments?.length === 1; | ||
| } catch { |
| return false; | ||
| } | ||
|
|
||
| // Check if it's a ReturnType reference |
There was a problem hiding this comment.
Let's remove unnecessary comments that just explain what happens
|
|
||
| // Handle different types of type arguments | ||
| if (ts.isTypeQueryNode(typeArg)) { | ||
| // Case: ReturnType<typeof functionName> |
There was a problem hiding this comment.
Please remove all the unnecessary comments. We don't need comments explaining what happens but instead should only have comments that explain why we do something the way we do.
| return false; | ||
| } | ||
|
|
||
| const typeName = ts.isIdentifier(node.typeName) ? node.typeName.text : node.typeName.getText(); |
There was a problem hiding this comment.
getText might throw in sourceless cenarios, where node.typeName.pos === -1
Handles
ReturnType<typeof functionName>ReturnType<SomeType['method']>This was to support more complex types from Redux toolkit store.
This was written mostly by AI which I guided along to get it to know how to get the return type. So some issues might be in how it is being parsed. I can look through the feedback to improve it.
Context:
https://github.com/microsoft/TypeScript/blob/f14b5c8a2f0be503ac455054a91573c63f0e5088/src/services/services.ts#L974-L976
https://github.com/microsoft/TypeScript/blob/f14b5c8a2f0be503ac455054a91573c63f0e5088/src/services/codefixes/fixAwaitInSyncFunction.ts#L56-L67
Possibly related issues:
#1155
#1847
#1887
#1910