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

fix: export "getResponse" for batched GraphQL queries #1982

Merged
merged 12 commits into from
Jan 21, 2024

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito commented Jan 20, 2024

Provides two integration test examples of how to intercept and mock a batched GraphQL query that's created by Apollo and batched-execute.

Roadmap

  • Provide the way to call handleRequest so it doesn't require tapping into server.emitter and server.resolvedOptions internals.
  • Consider if getResponse is a good public API. Rename it to resolveResponse? Usage-wise, it's good, just wondering about the naming.
    • Renamed to executeHandlers. It's precisely what this function does.
  • Add tests that combine some mocked and some bypassed queries within a single batched query response.
  • Add the "Batched GraphQL queries" recipe to the documentation.
  • Change getResponse signature to (handlers, request). No need for an object, its contract is unlikely to change (its intention is simple).
  • Add unit tests for getResponse.
  • Rename this pull request as a fix that gets getResponse

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
EndBug Federico Grandi
@kettanaito
Copy link
Member Author

@mattcosta7, may I ask your opinion on exporting the executeHandlers function as public now?

@mattcosta7
Copy link
Contributor

@mattcosta7, may I ask your opinion on exporting the executeHandlers function as public now?

Oh nice, I like this. It does a nice job of encapsulating these operations and just returning the results! Makes sense to me!

@kettanaito
Copy link
Member Author

@mattcosta7, I have a few thoughts on that API:

  • I somewhat dislike that parsedResult is present in the execution result object. This looks like an internal detail that should not leak to the user. But once this is public, we won't be able to remove it nicely.
  • The whole executeHandlers brings me to our discussion around redesigning how request lookup works internally. I'd like it to become sort of a class instance with a queue where you push requests and get them resolved. It doesn't stop us from refactoring internally and wrapping the new API in executeHandlers for backward-compatibility so it's relatively a low priority concern.

@mattcosta7
Copy link
Contributor

@mattcosta7, I have a few thoughts on that API:

  • I somewhat dislike that parsedResult is present in the execution result object. This looks like an internal detail that should not leak to the user. But once this is public, we won't be able to remove it nicely.
  • The whole executeHandlers brings me to our discussion around redesigning how request lookup works internally. I'd like it to become sort of a class instance with a queue where you push requests and get them resolved. It doesn't stop us from refactoring internally and wrapping the new API in executeHandlers for backward-compatibility so it's relatively a low priority concern.

Yeah that makes sense. As far as users should know just requests and responses exist and the rest is detail (maybe though, matching is also something transparent?)

@kettanaito
Copy link
Member Author

I'm thinking whether it'd make more sense to make executeHandlers as a new function, with the input only needed for public use and the return picking the handler and response (or even just response) and returning it. Basically, a wrapped around our internal executeHandlers. Easier to maintain and we don't change the function that works right for internal needs because it makes a bit lesser sense when used publicly.

@kettanaito
Copy link
Member Author

I think my latest commit addressed all the issues I have with the function! It's now getResponse, and it returns a mocked Response instance only.

@kettanaito kettanaito marked this pull request as ready for review January 20, 2024 20:14
@kettanaito kettanaito changed the title test: add batched queries tests fix: export "getResponse" for batched GraphQL queries Jan 21, 2024
@kettanaito kettanaito merged commit 42f1473 into main Jan 21, 2024
11 checks passed
@kettanaito kettanaito deleted the feat/graphql-batched-queries branch January 21, 2024 11:59
@kettanaito
Copy link
Member Author

Released: v2.1.3 🎉

This has been released in v2.1.3!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

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

Successfully merging this pull request may close these issues.

Support batched GraphQL queries
2 participants