-
-
Notifications
You must be signed in to change notification settings - Fork 583
fix: support async generators as response resolvers #2108
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
Merged
Merged
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
991a7b4
test: add generator resolver tests
kettanaito f324d33
fix: support async generator response resolvers
kettanaito baf112e
fix: accept AsyncGenerator as response resolver type
kettanaito a6a29bc
Merge branch 'main' into fix/generator-resolver
kettanaito 11d3719
fix(RequestHandler): mark as used early, opt-out in generators
kettanaito aac734a
fix(RequestHandler): unwrap AsyncGenerator generic from MaybePromise
kettanaito 8e26859
test: add return type tests for generators
kettanaito 180b722
chore: revert MaybeAsync to AsyncGenerator generic
kettanaito 8053895
Merge branch 'main' into fix/generator-resolver
kettanaito f815d58
Merge branch 'main' into fix/generator-resolver
kettanaito 3d6d083
chore: print tsconfig used in type tests
kettanaito 93da9ec
Merge branch 'main' into fix/generator-resolver
kettanaito 63fdd4b
fix: use iterables instead of generators (#2213)
jakebailey 9f75946
test: add type test for resolver return type
kettanaito 5c76326
test(generator): simplify fetch assertions
kettanaito 3325ae1
test(types): import from build, not source
kettanaito File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| import { invariant } from 'outvariant' | ||
| import { getCallFrame } from '../utils/internal/getCallFrame' | ||
| import { isIterable } from '../utils/internal/isIterable' | ||
| import type { ResponseResolutionContext } from '../utils/executeHandlers' | ||
|
|
@@ -57,6 +56,11 @@ export type AsyncResponseResolverReturnType< | |
| MaybeAsyncResponseResolverReturnType<ResponseBodyType>, | ||
| MaybeAsyncResponseResolverReturnType<ResponseBodyType> | ||
| > | ||
| | AsyncGenerator< | ||
| MaybeAsyncResponseResolverReturnType<ResponseBodyType>, | ||
| MaybeAsyncResponseResolverReturnType<ResponseBodyType>, | ||
| MaybeAsyncResponseResolverReturnType<ResponseBodyType> | ||
| > | ||
| > | ||
|
|
||
| export type ResponseResolverInfo< | ||
|
|
@@ -117,11 +121,17 @@ export abstract class RequestHandler< | |
| public isUsed: boolean | ||
|
|
||
| protected resolver: ResponseResolver<ResolverExtras, any, any> | ||
| private resolverGenerator?: Generator< | ||
| MaybeAsyncResponseResolverReturnType<any>, | ||
| MaybeAsyncResponseResolverReturnType<any>, | ||
| MaybeAsyncResponseResolverReturnType<any> | ||
| > | ||
| private resolverGenerator?: | ||
| | Generator< | ||
| MaybeAsyncResponseResolverReturnType<any>, | ||
| MaybeAsyncResponseResolverReturnType<any>, | ||
| MaybeAsyncResponseResolverReturnType<any> | ||
| > | ||
| | AsyncGenerator< | ||
| MaybeAsyncResponseResolverReturnType<any>, | ||
| MaybeAsyncResponseResolverReturnType<any>, | ||
| MaybeAsyncResponseResolverReturnType<any> | ||
| > | ||
| private resolverGeneratorResult?: Response | StrictResponse<any> | ||
| private options?: HandlerOptions | ||
|
|
||
|
|
@@ -256,6 +266,9 @@ export abstract class RequestHandler< | |
| return null | ||
| } | ||
|
|
||
| // Preemptively mark the handler as used. | ||
| // Generators will undo this because only when the resolver reaches the | ||
| // "done" state of the generator that it considers the handler used. | ||
| this.isUsed = true | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's important to mark the handler as used immediately, even if using generators then opts-out from this behavior. This is what we promise right now, so let's keep that promise. |
||
|
|
||
| // Create a response extraction wrapper around the resolver | ||
|
|
@@ -304,39 +317,28 @@ export abstract class RequestHandler< | |
| const result = this.resolverGenerator || (await resolver(info)) | ||
|
|
||
| if (isIterable<AsyncResponseResolverReturnType<any>>(result)) { | ||
| // Immediately mark this handler as unused. | ||
| // Only when the generator is done, the handler will be | ||
| // considered used. | ||
| // Opt-out from marking this handler as used. | ||
| this.isUsed = false | ||
|
|
||
| const { value, done } = result[Symbol.iterator]().next() | ||
| const nextResponse = await value | ||
|
|
||
| if (done) { | ||
| this.isUsed = true | ||
| } | ||
|
|
||
| // If the generator is done and there is no next value, | ||
| // return the previous generator's value. | ||
| if (!nextResponse && done) { | ||
| invariant( | ||
| this.resolverGeneratorResult, | ||
| 'Failed to returned a previously stored generator response: the value is not a valid Response.', | ||
| ) | ||
|
|
||
| // Clone the previously stored response from the generator | ||
| // so that it could be read again. | ||
| return this.resolverGeneratorResult.clone() as StrictResponse<any> | ||
| } | ||
|
|
||
| if (!this.resolverGenerator) { | ||
| this.resolverGenerator = result | ||
| } | ||
|
|
||
| const { done, value } = await this.resolverGenerator.next() | ||
| const nextResponse = await value | ||
|
|
||
| if (nextResponse) { | ||
| // Also clone the response before storing it | ||
| // so it could be read again. | ||
| this.resolverGeneratorResult = nextResponse?.clone() | ||
| this.resolverGeneratorResult = nextResponse.clone() | ||
| } | ||
|
|
||
| if (done) { | ||
| // A one-time generator resolver stops affecting the network | ||
| // only after it's been completely exhausted. | ||
| this.isUsed = true | ||
|
|
||
| // Clone the previously stored response so it can be read | ||
| // when receiving it repeatedly from the "done" generator. | ||
| return this.resolverGeneratorResult?.clone() | ||
| } | ||
|
|
||
| return nextResponse | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| /** | ||
| * @vitest-environment node | ||
| */ | ||
| import { http, HttpResponse, delay } from 'msw' | ||
| import { setupServer } from 'msw/node' | ||
|
|
||
| const server = setupServer() | ||
| const toJson = (response: Response) => response.json() | ||
|
|
||
| beforeAll(() => { | ||
| server.listen() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| server.resetHandlers() | ||
| }) | ||
|
|
||
| afterAll(() => { | ||
| server.close() | ||
| }) | ||
|
|
||
| it('supports generator function as response resolver', async () => { | ||
| server.use( | ||
| http.get('https://example.com/weather', function* () { | ||
| let degree = 10 | ||
|
|
||
| while (degree < 13) { | ||
| degree++ | ||
| yield HttpResponse.json(degree) | ||
| } | ||
|
|
||
| degree++ | ||
| return HttpResponse.json(degree) | ||
| }), | ||
| ) | ||
|
|
||
| // Must respond with yielded responses. | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(11) | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(12) | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(13) | ||
| // Must respond with the final "done" response. | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(14) | ||
| // Must keep responding with the final "done" response. | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(14) | ||
| }) | ||
|
|
||
| it('supports async generator function as response resolver', async () => { | ||
| server.use( | ||
| http.get('https://example.com/weather', async function* () { | ||
| await delay(20) | ||
|
|
||
| let degree = 10 | ||
|
|
||
| while (degree < 13) { | ||
| degree++ | ||
| yield HttpResponse.json(degree) | ||
| } | ||
|
|
||
| degree++ | ||
| return HttpResponse.json(degree) | ||
| }), | ||
| ) | ||
|
|
||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(11) | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(12) | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(13) | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(14) | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(14) | ||
| }) | ||
|
|
||
| it('supports generator function as one-time response resolver', async () => { | ||
| server.use( | ||
| http.get( | ||
| 'https://example.com/weather', | ||
| function* () { | ||
| let degree = 10 | ||
|
|
||
| while (degree < 13) { | ||
| degree++ | ||
| yield HttpResponse.json(degree) | ||
| } | ||
|
|
||
| degree++ | ||
| return HttpResponse.json(degree) | ||
| }, | ||
| { once: true }, | ||
| ), | ||
| http.get('*', () => { | ||
| return HttpResponse.json('fallback') | ||
| }), | ||
| ) | ||
|
|
||
| // Must respond with the yielded incrementing responses. | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(11) | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(12) | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(13) | ||
| // Must respond with the "done" final response from the iterator. | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual(14) | ||
| // Must respond with the other handler since the generator one is used. | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual( | ||
| 'fallback', | ||
| ) | ||
| expect(await fetch('https://example.com/weather').then(toJson)).toEqual( | ||
| 'fallback', | ||
| ) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { it } from 'vitest' | ||
| import { http, HttpResponse } from 'msw' | ||
|
|
||
| it('supports generator function as response resolver', () => { | ||
| http.get<never, never, { value: number }>('/', function* () { | ||
| yield HttpResponse.json({ value: 1 }) | ||
| yield HttpResponse.json({ value: 2 }) | ||
| return HttpResponse.json({ value: 3 }) | ||
| }) | ||
|
|
||
| http.get<never, never, { value: string }>('/', function* () { | ||
| yield HttpResponse.json({ value: 'one' }) | ||
| yield HttpResponse.json({ | ||
| // @ts-expect-error Expected string, got number. | ||
| value: 2, | ||
| }) | ||
| return HttpResponse.json({ value: 'three' }) | ||
| }) | ||
| }) | ||
|
|
||
| it('supports async generator function as response resolver', () => { | ||
| http.get<never, never, { value: number }>('/', async function* () { | ||
| yield HttpResponse.json({ value: 1 }) | ||
| yield HttpResponse.json({ value: 2 }) | ||
| return HttpResponse.json({ value: 3 }) | ||
| }) | ||
|
|
||
| http.get<never, never, { value: string }>('/', async function* () { | ||
| yield HttpResponse.json({ value: 'one' }) | ||
| yield HttpResponse.json({ | ||
| // @ts-expect-error Expected string, got number. | ||
| value: 2, | ||
| }) | ||
| return HttpResponse.json({ value: 'three' }) | ||
| }) | ||
| }) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Older versions of TypeScript (4.7 - 5.2) have trouble digesting this type:
Reproduction steps
pnpm test:tsThere 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.
Hi, @Andarist! Sorry for troubling you once again. If you have a minute, could you please lend me a hand with this type issue?
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 would need to see a slimmed-down repro. The one that I tried put together in a rush errors in the current TS version too: TS playground
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.
Thanks for giving it a try, @Andarist. Here's a minimal repro:
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.
That is not exactly minimal ;p it still depends on everything in the MSW's codebase. I could really use a self-contained TS playground here.
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.
Yes, I can do that.
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.
FWIW I did compile and test locally, and it all seemed to work, though I did not acutally test the full range of TS versions (but I can't imagine it wouldn't work, maybe I'm jinxing myself)
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.
Happy to give this a try once it's here 👀 Installing different
typescriptdependency versions triggers thetest:tsinto using that version for testing. The CI will also cover all the ranges for us. So excited we may actually ship 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.
Sent #2213.
FWIW if you're looking for multi-version testing and aren't too tied to vitest, https://github.com/tstyche/tstyche is one option I've heard of which automatically runs multiple versions of TS without having to install them explicitly.
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.
That's nice! Something to consider in the setup.