-
-
Notifications
You must be signed in to change notification settings - Fork 810
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
batch-execute enhancements #3588
Conversation
🦋 Changeset detectedLatest commit: 7229033 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/theguild/graphql-tools/8i7BBqoyptioGuce3ceA68k5PND5 |
It all looks great to me. Very important to highlight that batch execution as implemented here is based almost entirely on the amazing work at Gatsby by @vladar see gatsbyjs/gatsby#22347 |
I added that Gatsby PR link into the documentation page to give due credit. Rah rah, open source! |
…to gmac/batch-execute
Calling this installment ready to go, @ardatan. I'll have a followup to this that fixes some bugs with fragments... those will probably be best reviewed on their own. |
|
||
if (executionVariableNames.length > 0) { | ||
if (executionVariableNames.length > 0 || hasFragmentDefinitions) { |
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.
@yaacovCR – have a look at 44ba4cb. I've fixed a couple bugs related to fragments here:
- Fragments weren't getting prefixed unless there were also variables.
- Inlined root fragments were allowed to leave an invalid orphaned fragment definition. This now filters out fragment definitions unless they have a surviving spread implementation.
Thanks for the feedback, gents. Looks tighter now and fixes more bugs. |
import { DocumentNode, DefinitionNode, OperationTypeNode, OperationDefinitionNode, Kind } from 'graphql'; | ||
|
||
export function operationTypeFromDocument(document: DocumentNode): OperationTypeNode { | ||
for (const def of document.definitions) { |
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.
Maybe getOperationAST from graphql-js. But in case of multiple operations given in the same document, we will need operationName as well.
Thank you @gmac <3 |
Description
Batch execution is a really good feature. Unfortunately, it's a hidden gem within the mono-repo at the moment with little attention given outside of the built-in schema stitching option. This PR works on changing that by making batch execution friendlier for general use. There are some possible points of contention here, so let's discuss...
BUG FIX: See 44ba4cb. The current implementation has a couple bugs involving fragment definitions:
Within
splitResults
, errors without a path are now returned to all requests in the batch. This is really important for general usability: right now, batch execution swallows unpathed errors such as schema validations and complexity limits. These errors (particularly schema validations) will rarely be encountered by schema stitching, but are commonplace when batching hand-written queries. The downside of returning all unpathed errors is that all results in the batch will report the same validation error even though it may only apply to one request in the batch, but over-reporting feels better than under-reporting.I've reduced the
graphqlTools0_
prefix down to just_0_
... heavily inspired by Bramble. Given that batching rewrites all root scope aliases, we're not really at risk of conflicting with anything, and thegraphqlTools
prefix divulges too much information about the underlying implementation, IMO. The simpler prefix is more generic and doesn't encourage analytics or instrumentation based on weak signals. It also lends itself to being documented as a generic capability in higher-level product features.Adds a local test suite for the batch-execute package.
Lastly, I've added a page of documentation about this fantastic package that is both remarkably powerful and quite easy to use. With documentation circulating, we may see some renewed interest in this hidden gem of GraphQL Tools.
Curious of your thoughts here, @yaacovCR.
Type of change
Please delete options that are not relevant.
Checklist:
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...