Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
jankapunkt wants to merge 2 commits into master from types/sync-async-model-types

Conversation

@jankapunkt
Copy link
Member

@jankapunkt jankapunkt commented May 20, 2025

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

Copy link

z1haze commented Jun 2, 2025

Thanks for this

This comment was marked as outdated.

Copy link

Copilot AI left a 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.

Copy link
Member

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:

  1. Update the types to string | Promise to match the doc's promise of sync-or-async. (Which is what this CR is doing).

  2. 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?

dhensby reacted with thumbs up emoji

Copy link
Contributor

dhensby commented Jul 9, 2025

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

Copy link
Member Author

@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.

dhensby and HappyZombies reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@dhensby dhensby Awaiting requested review from dhensby

@HappyZombies HappyZombies Awaiting requested review from HappyZombies

@jorenvandeweyer jorenvandeweyer Awaiting requested review from jorenvandeweyer

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /