-
Notifications
You must be signed in to change notification settings - Fork 169
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
Route handler return types #1009
Conversation
This makes sense to me. Would you mind adding some type tests that fail without this change and pass with it? Thanks! |
@IanVS I added some tests though I'm not 100% sure I did them right. Couldn't get them to fail even though vscode showed errors when I didn't have the new types. |
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.
I think the tests are good. I reverted out your first commit to run the tests without your changes, and they do indeed fail with errors like:
ERROR: 26:20 expect TypeScript@4.0 compile error:
Argument of type '() => number' is not assignable to parameter of type 'RouteHandler<AnyRegistry>'.
Type 'number' is not assignable to type 'MaybePromise<object | Response | { id?: string | undefined; attrs: {}; modelName: string; save(): void; update<K extends never>(key: K, value: {}[K]): void; update(changes: Partial<{}>): void; destroy(): void; reload(): void; }>'.
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.
Looks good, thanks for the PR!
Thanks for your help/input! |
How and when would this get released? Just not sure what the process/timeframe is |
I'm trying to see if there are any other PRs I can get merged in, and then I'll try cutting a release today. |
Turns out I don't have publishing rights to npm, so we need to wait for @samselikoff to do that part. |
Happy to add you & don’t want to block! What’s your npm username |
@samselikoff thanks. I'm |
Done!
… On Dec 2, 2021, at 12:16 PM, Ian VanSchooten ***@***.***> wrote:
@samselikoff thanks. I'm ianvs on npm as well.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
Thanks, it looks like I'm added to |
Sorry was confused by another thread! Should have it now
… On Dec 2, 2021, at 7:47 PM, Ian VanSchooten ***@***.***> wrote:
Thanks, it looks like I'm added to ember-cli-mirage, but not miragejs. I don't use ember, so you may as well remove me from that one.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
@pleek91, 0.1.43 has been released with your changes. Thanks again, everyone. |
This updates the types for
RouteHandler
to allow for any valid JSON response. Addsnumber | string | boolean | null
as well as arrays ofobject | number | string | boolean | null
.This is necessary because endpoint can return more than just an object. For instance the project I'm currently working on adding mirage too has an endpoint that returns just a number. Which is totally valid however using the current types without this change causes typescript to emit an error.