-
Notifications
You must be signed in to change notification settings - Fork 254
feat: @defer support #898
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
base: next
Are you sure you want to change the base?
feat: @defer support #898
Changes from 3 commits
cee50b7
4ad38b4
08ee6d2
35fdb02
d41fff6
274c5bc
3e3ca77
2b3c25a
369457e
780c668
b4d70fc
e27e5b0
6933420
d1d8485
2d599a7
ef73394
640300f
8b7e9b2
7a84f17
093d0df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| 'use strict' | ||
|
|
||
| const Negotiator = require('negotiator') | ||
| const fp = require('fastify-plugin') | ||
| let LRU = require('tiny-lru') | ||
| const routes = require('./lib/routes') | ||
|
|
@@ -18,10 +19,10 @@ const { | |
| extendSchema, | ||
| validate, | ||
| validateSchema, | ||
| specifiedRules, | ||
| execute | ||
| specifiedRules | ||
| } = require('graphql') | ||
| const { buildExecutionContext } = require('graphql/execution/execute') | ||
| const { Readable } = require('stream') | ||
| const queryDepth = require('./lib/queryDepth') | ||
| const buildFederationSchema = require('./lib/federation') | ||
| const { initGateway } = require('./lib/gateway') | ||
|
|
@@ -47,6 +48,7 @@ const { | |
| preExecutionHandler, | ||
| onResolutionHandler | ||
| } = require('./lib/handlers') | ||
| const { executeGraphql, MEDIA_TYPES } = require('./lib/util') | ||
|
|
||
| // Required for module bundlers | ||
| // istanbul ignore next | ||
|
|
@@ -546,7 +548,7 @@ const plugin = fp(async function (app, opts) { | |
| return maybeFormatErrors(execution, context) | ||
| } | ||
|
|
||
| const execution = await execute({ | ||
| const execution = await executeGraphql({ | ||
| schema: modifiedSchema || fastifyGraphQl.schema, | ||
| document: modifiedDocument || document, | ||
| rootValue: root, | ||
|
|
@@ -555,9 +557,93 @@ const plugin = fp(async function (app, opts) { | |
| operationName | ||
| }) | ||
|
|
||
| /* istanbul ignore next */ | ||
| if (execution.initialResult) { | ||
| const acceptHeader = reply.request.raw.headers.accept | ||
|
|
||
| if ( | ||
| !( | ||
| acceptHeader && | ||
| new Negotiator({ | ||
| headers: { accept: acceptHeader } | ||
| }).mediaType([ | ||
| // mediaType() will return the first one that matches, so if the client | ||
| // doesn't include the deferSpec parameter it will match this one here, | ||
| // which isn't good enough. | ||
| MEDIA_TYPES.MULTIPART_MIXED_NO_DEFER_SPEC, | ||
| MEDIA_TYPES.MULTIPART_MIXED_EXPERIMENTAL | ||
| ]) === MEDIA_TYPES.MULTIPART_MIXED_EXPERIMENTAL | ||
| ) | ||
| ) { | ||
| // The client ran an operation that would yield multiple parts, but didn't | ||
| // specify `accept: multipart/mixed`. We return an error. | ||
| throw new Error( | ||
|
Collaborator
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. Please create an error in https://github.com/mercurius-js/mercurius/blob/master/lib/errors.js and use it here |
||
| 'Server received an operation that uses incremental delivery ' + | ||
| '(@defer or @stream), but the client does not accept multipart/mixed ' + | ||
| 'HTTP responses. To enable incremental delivery support, add the HTTP ' + | ||
| "header 'Accept: multipart/mixed; deferSpec=20220824'." | ||
| ) | ||
| } | ||
|
mcollina marked this conversation as resolved.
|
||
|
|
||
| reply.header('content-type', 'multipart/mixed; boundary="-"; deferSpec=20220824') | ||
|
|
||
| return Readable.from( | ||
|
mcollina marked this conversation as resolved.
|
||
| writeMultipartBody( | ||
| execution.initialResult, | ||
| execution.subsequentResults | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| return maybeFormatErrors(execution, context) | ||
| } | ||
|
|
||
| /* istanbul ignore next */ | ||
| async function * writeMultipartBody (initialResult, subsequentResults) { | ||
| yield `\r\n---\r\ncontent-type: application/json; charset=utf-8\r\n\r\n${JSON.stringify( | ||
| orderInitialIncrementalExecutionResultFields(initialResult) | ||
| )}\r\n---${initialResult.hasNext ? '' : '--'}\r\n` | ||
|
|
||
| for await (const result of subsequentResults) { | ||
| yield `content-type: application/json; charset=utf-8\r\n\r\n${JSON.stringify( | ||
| orderSubsequentIncrementalExecutionResultFields(result) | ||
| )}\r\n---${result.hasNext ? '' : '--'}\r\n` | ||
| } | ||
| } | ||
|
|
||
| /* istanbul ignore next */ | ||
| function orderInitialIncrementalExecutionResultFields (result) { | ||
| return { | ||
| hasNext: result.hasNext, | ||
| errors: result.errors, | ||
| data: result.data, | ||
| incremental: orderIncrementalResultFields(result.incremental), | ||
| extensions: result.extensions | ||
| } | ||
| } | ||
|
|
||
| /* istanbul ignore next */ | ||
| function orderSubsequentIncrementalExecutionResultFields (result) { | ||
| return { | ||
| hasNext: result.hasNext, | ||
| incremental: orderIncrementalResultFields(result.incremental), | ||
| extensions: result.extensions | ||
| } | ||
| } | ||
|
|
||
| /* istanbul ignore next */ | ||
| function orderIncrementalResultFields (incremental) { | ||
| return incremental?.map((i) => ({ | ||
| hasNext: i.hasNext, | ||
| errors: i.errors, | ||
| path: i.path, | ||
| label: i.label, | ||
| data: i.data, | ||
| items: i.items, | ||
| extensions: i.extensions | ||
| })) | ||
| } | ||
|
|
||
| async function maybeFormatErrors (execution, context) { | ||
| execution = addErrorsToExecutionResult(execution, context.errors) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| 'use strict' | ||
|
|
||
| const { execute } = require('graphql') | ||
| const { experimentalExecuteIncrementally } = require('graphql/execution') | ||
|
|
||
| function hasDirective (directiveName, node) { | ||
| if (!node.directives || node.directives.length < 1) { | ||
| return false | ||
|
|
@@ -23,7 +26,26 @@ function hasExtensionDirective (node) { | |
| } | ||
| } | ||
|
|
||
| const canUseIncrementalExecution = !!require('graphql/execution').experimentalExecuteIncrementally | ||
|
Collaborator
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. not needed, this is always true now. |
||
|
|
||
| // istanbul ignore next | ||
| function executeGraphql (args) { | ||
| if (canUseIncrementalExecution) { | ||
|
Collaborator
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. This should happen only if defer is enabled. |
||
| return experimentalExecuteIncrementally(args) | ||
| } | ||
|
|
||
| return execute(args) | ||
| } | ||
|
|
||
| const MEDIA_TYPES = { | ||
| MULTIPART_MIXED_NO_DEFER_SPEC: 'multipart/mixed', | ||
| MULTIPART_MIXED_EXPERIMENTAL: 'multipart/mixed; deferSpec=20220824' | ||
| } | ||
|
|
||
| module.exports = { | ||
| hasDirective, | ||
| hasExtensionDirective | ||
| hasExtensionDirective, | ||
| canUseIncrementalExecution, | ||
| executeGraphql, | ||
| MEDIA_TYPES | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,7 @@ | |
| "graphql": "^16.0.0", | ||
|
Collaborator
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. I think we are not testing this behavior in CI, right? We should. The
Contributor
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. I could try adding CI tests that install Also, what do you mean by
Collaborator
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. How would an end user use defer? As you can see we depend upon
Contributor
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. I see. So the Apollo Server, as a reference, didn't update graphql version and they will only do that when graphql@17 is released. Until that, it's the change that is implemented and tested on CI, but not available to the end users since it's still in alpha That being said, the idea of mine was to do the same here - start supporting it but not allow users to use it until graphql@17 is officially released In the future, when graphql@17 is live, we will update the mercurius dependency and everyone will be able to use defer
Collaborator
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. I'm happy to cut a next branch for you, so we can land this with the alpha version of graphql-js. wdyt?
Contributor
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. sounds good to me! so i finish the feedback first and then you create a next release branch?
Collaborator
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. +1
Contributor
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. I incorporated the feedback and going to see what's going on there with JIT compiler when I have some time |
||
| "graphql-jit": "^0.7.3", | ||
| "mqemitter": "^5.0.0", | ||
| "negotiator": "^0.6.3", | ||
| "p-map": "^4.0.0", | ||
| "readable-stream": "^4.0.0", | ||
| "safe-stable-stringify": "^2.3.0", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| const { test } = require('tap') | ||
| const Fastify = require('fastify') | ||
| const mercurius = require('../index') | ||
| const { canUseIncrementalExecution } = require('../lib/util') | ||
|
|
||
| if (canUseIncrementalExecution) { | ||
|
Collaborator
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. This if should not be needed anymore. |
||
| const schema = ` | ||
| directive @defer( | ||
| if: Boolean! = true | ||
| label: String | ||
| ) on FRAGMENT_SPREAD | INLINE_FRAGMENT | ||
|
Collaborator
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. Shouldn't this directive added automatically by mercurius?
Collaborator
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. I would do this behind a
Contributor
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. Hey @mcollina I've added I can remove What do you think? |
||
|
|
||
| type Query { | ||
| allProducts: [Product!]! | ||
| } | ||
|
|
||
| type Product { | ||
| delivery: DeliveryEstimates! | ||
| sku: String! | ||
| id: ID! | ||
| } | ||
|
|
||
| type DeliveryEstimates { | ||
| estimatedDelivery: String! | ||
| fastestDelivery: String! | ||
| }` | ||
|
|
||
| const allProducts = new Array(1).fill(0).map((_, index) => ({ | ||
| id: `${index}`, | ||
| sku: 'sku' | ||
| })) | ||
|
|
||
| const resolvers = { | ||
| Query: { | ||
| allProducts: () => { | ||
| return allProducts | ||
| } | ||
| }, | ||
| Product: { | ||
| delivery: () => { | ||
| return { | ||
| estimatedDelivery: '25.01.2000', | ||
| fastestDelivery: '25.01.2000' | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const query = ` | ||
| query deferVariation { | ||
| allProducts { | ||
| delivery { | ||
| ...MyFragment @defer | ||
| } | ||
| sku | ||
| id | ||
| } | ||
| } | ||
|
|
||
| fragment MyFragment on DeliveryEstimates { | ||
| estimatedDelivery | ||
| fastestDelivery | ||
| } | ||
| ` | ||
|
|
||
| const wrongAcceptValues = [ | ||
| '', | ||
| 'application/json', | ||
| 'multipart/mixed', | ||
| 'multipart/mixed; deferSpec=12345' | ||
| ] | ||
|
|
||
| for (const accept of wrongAcceptValues) { | ||
| test('errors with @defer when used with wrong "accept" header', async t => { | ||
| const app = Fastify() | ||
| await app.register(mercurius, { schema, resolvers, graphiql: true }) | ||
|
|
||
| const res = await app.inject({ | ||
| method: 'POST', | ||
| url: '/graphql', | ||
| headers: { | ||
| 'content-type': 'application/json', | ||
| accept | ||
| }, | ||
| body: JSON.stringify({ query }) | ||
| }) | ||
|
|
||
| t.match(res, { | ||
| statusCode: 200, | ||
| body: JSON.stringify({ | ||
| data: null, | ||
| errors: [{ | ||
| message: "Server received an operation that uses incremental delivery (@defer or @stream), but the client does not accept multipart/mixed HTTP responses. To enable incremental delivery support, add the HTTP header 'Accept: multipart/mixed; deferSpec=20220824'." | ||
| }] | ||
| }) | ||
| }) | ||
|
|
||
| await app.close() | ||
| t.end() | ||
| }) | ||
| } | ||
|
|
||
| const correctAcceptValues = [ | ||
| 'multipart/mixed; deferSpec=20220824', | ||
| 'multipart/mixed; deferSpec=20220824, application/json', | ||
| 'application/json, multipart/mixed; deferSpec=20220824' | ||
| ] | ||
|
|
||
| for (const accept of correctAcceptValues) { | ||
| test('works with @defer when used with correct "accept" header', async t => { | ||
| const app = Fastify() | ||
| await app.register(mercurius, { schema, resolvers, graphiql: true }) | ||
|
|
||
| const res = await app.inject({ | ||
| method: 'POST', | ||
| url: '/graphql', | ||
| headers: { | ||
| 'content-type': 'application/json', | ||
| accept | ||
| }, | ||
| body: JSON.stringify({ query }) | ||
| }) | ||
|
|
||
| t.match(res, { | ||
| statusCode: 200, | ||
| headers: { | ||
| 'content-type': 'multipart/mixed; boundary="-"; deferSpec=20220824' | ||
| }, | ||
| body: `\r | ||
| ---\r | ||
| content-type: application/json; charset=utf-8\r | ||
| \r | ||
| {"hasNext":true,"data":{"allProducts":[{"delivery":{},"sku":"sku","id":"0"}]}}\r | ||
| ---\r | ||
| content-type: application/json; charset=utf-8\r | ||
| \r | ||
| {"hasNext":false,"incremental":[{"path":["allProducts",0,"delivery"],"data":{"estimatedDelivery":"25.01.2000","fastestDelivery":"25.01.2000"}}]}\r | ||
| -----\r | ||
| ` | ||
| }) | ||
|
|
||
| await app.close() | ||
| t.end() | ||
| }) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.