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

Code feedback #44

Open
kettanaito opened this issue Jun 19, 2024 · 4 comments
Open

Code feedback #44

kettanaito opened this issue Jun 19, 2024 · 4 comments

Comments

@kettanaito
Copy link

Hey! So excited to finally see this library out!

I will provide a few code review and suggestions below. Feel free to convert this to a discussion or whichever format is more suitable for you.

// @ts-expect-error FIXME: mismatch on the return type between HttpResponse
// with a stream vs. json
>(async ({ query, variables, operationName }) => {

This isn't something MSW can solve with the current version range of TS we support, unfortunately. You can learn more about why here: mswjs/msw#1691.

Solution: Provide a union of expected response types to the handler. I'm not sure if graphql.*() functions support that right now though. It looks like they infer the response resolver (and its return type) from the query. Let me know if this turns problematic, we can work on a solution for it.


const randomDelay = Math.random() * (delayMax - delayMin) + delayMin;
await wait(randomDelay);

It would be great to let the folks use the built-in delay values too, like infinite or random server response time. Looks like a change here.


For debugging reasons, it would be nice to see what schema was used when a handler was run. You can achieve that by creating a custom request handler instead of wrapping graphql.operation(). You can then define its log() method to include the schema when debugging handlers.

I think that can be handy given you can swap the schema mid-test. You also get access to internal caching API which you can use to cache any computation you need (like gql(query) or AST parsing, if that makes sense).


If you have any questions about the WebSocket mocking, please let me know.

@alessbell
Copy link
Contributor

Hey @kettanaito 👋

Thanks so much for taking the time to dig in and for your comments! This is great feedback - I'll keep this open as I work through these items. I'm working on WS mocking right now and appreciate the offer to field any questions - I'll let you know, and will otherwise link to the PR once it's up :)

@alessbell
Copy link
Contributor

One update on using the built-in delay values, something I recently shipped. It's definitely nice to be able to reuse that :)

I noticed flakiness in Jest tests with concurrent React autobatching in a scenario with GraphQL queries with @defer and multipart responses. With a 5ms delay between chunks, they were sometimes being batched into a single render.

I want the default behavior in Node processes to be "each chunk causes a re-render" so I upped it to 20ms which seems to be firmly on the "not autobatched" side of the chasm: https://github.com/apollographql/graphql-testing-library/blob/main/src/handlers.ts#L41-L47 though I still need to test this with React 19 RCs.

Finally I'm continuing work on subscriptions, so far so good!

@kettanaito
Copy link
Author

So excited to hear how the subscriptions will turn out!

A bit updates from my end, I'm planning on releasing the WebSocket support in MSW later this month. @alessbell, would I be able to hear your feedback before that? I don't mind postponing, it's really about hearing back and shipping the best API possible.

@alessbell
Copy link
Contributor

@kettanaito that's very exciting! So far I haven't run into any issues - I'm working out some details around reconnection attempts, but I'm far enough along that I'd say it's all clear on my end :)

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

No branches or pull requests

2 participants