Skip to content
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

feat: infer result type correctly with TypeScript #142

Merged

Conversation

tomoto
Copy link
Contributor

@tomoto tomoto commented Sep 28, 2021

Greetings, it looks like some people (#126 for example) want the type definition to support the non-string results, and here is my contribution.

Basically I added the minimal fix to the type definition to infer the result type based on the output option, and the TypeScript test cases to validate the type checks successfully pass as intended. As long as the output option is statically known to the compiler (which hopefully would be the case in the typical use cases), the result type can be uniquely determined; otherwise, the applications are still responsible for asserting the expected type but they no longer need to insert the dirty as unknown at least.

I also removed the mention to DefinitelyTyped in the doc comment and the type definition generaiton script in package.json since they looked obsolete.

Hope this helps. Thank you very much.

@avoidwork
Copy link
Owner

Hi, thanks for preparing this; I missed the notification. Will get it merged shortly.

@avoidwork avoidwork merged commit 072d6f5 into avoidwork:master Oct 24, 2021
@avoidwork
Copy link
Owner

Unfortunately I need to force push the merge out; typescript/tsc is no longer usable from npx.

Screenshot 2021-10-24 091347

avoidwork added a commit that referenced this pull request Oct 24, 2021
…th-typescript"

This reverts commit 072d6f5, reversing
changes made to afd78c2.
@tomoto
Copy link
Contributor Author

tomoto commented Oct 27, 2021

Thank you for looking into this. OK, I will change package.json not to use npx. Actually I am not a huge fan of npx and did not know it could cause such an issue. Talk to you later. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants