-
-
Notifications
You must be signed in to change notification settings - Fork 54
types: make model types support sync AND async methods #347
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
Conversation
z1haze
commented
Jun 2, 2025
Thanks for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot wasn't able to review any files in this pull request.
So I have an opinion here, this isn't really about runtime compatibility (which is fine), since the lib treats every model function as async, but about whether the type system should match what the documentation states.
I think really there are 2 possible solutions:
-
Update the types to string | Promise to match the doc's promise of sync-or-async. (Which is what this CR is doing).
-
Update the docs to clarify: "You can write synchronous logic, but model methods must return a Promise."
To me option 2 seems cleaner — the current typings enforce consistent async shape and avoid noisy unions. Since async functions support returning sync values anyway, for example, just wrap it in async? That will always return a promise.
(async () => 'foo')(); // returns a resolved Promise
What do yall think?
I agree with @HappyZombies - people can mark their function async and they are compatible with the typing system and the method is still technically sync (but I admit is returning a promise). Generally I'd prefer a narrower well defined interface in TS than one that covers all the technical possibilities of the implementation (a problem our types currently have with the bespoke Falsey type - something I've thought about trying to remove).
If the OP of the discussion doesn't like sticking to the type declaration, then use JS, if they want to use TS they they'll need to adhere to the typings because those are the intended (rather than possible) interfaces
@HappyZombies @dhensby thank you for your input. Since I'm no TypeScript pro I can't really add valuable information. Feel free to close, if you think this is out of scope.
Summary
This is proof of concept for further discussion, related to #344
Please add your opinion and review.
Linked issue(s)
#344
Involved parts of the project
Types
Added tests?
OAuth2 standard
Reproduction
Custom IDE related