feat: @defer support #898

igrlk · 2022-10-25T23:00:20Z

Hi everyone ✋ As some of you may have noticed, apollo and graphql-js added support for @defer directive, so it's time to do the same in Mercurius!

🤔 What is @defer and why do we need that?

@defer is a directive that allows you to receive query data incrementally. There is a bunch of information about it but a decent source is Apollo Docs @defer.

⌛ Current state of things

graphql-spec for @defer is not yet finished, although looks like we can rely on it, because 👇
graphql-js implemented @defer and made it available in [email protected]
apollo-client and apollo-server added support for @defer.
- Apollo-Server changes are not yet in main branch, but in the next major version branch

📦 What this PR does

It uses the new GraphQL incremental api (if it's available! otherwise we use old api for backwards-compatibility) to execute queries.
The incremental api itself does all the magic for us and returns either the classic response (body.data.myField) or the incremental one with stream (see more in the api type definitions)

Client should send us the following header accept: multipart/mixed; deferSpec=20220824 in order to get a stream response back. Otherwise, the error is thrown that doesn't allow client to get the data.

The PR also moves to a new signature of GraphQLError since the old one is deprecated and its support is removed in graphql@17

Additionally, it bumps the graphql-js to 17.0.0-alpha.2 (latest available right now). Once PR has all the tests passed and feedback incorporated, it's going to be cut off to a separate release branch, instead of getting merged to master directly because of graphql@17 which is still in alpha

🧰 Local development / testing

If you wish to see how it's going to look like in a real app, I've put together an Apollo Client/Server + Mercurius setup that allows you to run a query with @defer against either:

This Mercurius branch, to see what we've got
Apollo Server, to see how it works there

⚠️ Things that aren't yet done

graphql-jit is not adopted to use graphql@17 yet and I asked code owners if they want to make an upgrade GraphQL 17 support? zalando-incubator/graphql-jit#184

Please take into account that all the things I mentioned in the PR are my suggestions based on what I've read and heard. I am open to the ideas you might have, especially because it's my first PR to this repository.

I want to give a huge shoutout to the Mercurius team for their amazing hard work maintaining this project and to Apollo team who were the first in @defer implementation, and whose examples I've used to implement the changes in this repo.

Cheers ✌️

…20824 accept header

mcollina

This is a amazing on multiple level! Deferred resolver is massive new feature coming in graphql v17!

(Given that this PR is also made possible by Readable.from() and node stream iterator support... I'm even more in awe. I championed and implemented those features).

I'm a bit concerned how we could land this and let people experiment about it for now.

It's better if we put @defer under an option. This will allow to throw if jit is enabled or it's missing graphql-jit.

package.json

test/directives.js

mcollina · 2022-10-26T06:32:17Z

test/directives.js

+      'multipart/mixed; deferSpec=12345'
+    ]
+
+    for (const accept of wrongAcceptValues) {


move this for loop outside of the test definition

test/directives.js

mcollina · 2022-10-26T06:34:06Z

test/directives.js

+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


Can you also add a test using undici.fetch()? It should work and support multipart responses.

index.js

test/directives.js

index.js

…removed unnecessary status code in the error

mcollina · 2022-10-26T09:28:29Z

index.js

+      ) {
+        // The client ran an operation that would yield multiple parts, but didn't
+        // specify `accept: multipart/mixed`. We return an error.
+        throw new Error(


Please create an error in https://github.com/mercurius-js/mercurius/blob/master/lib/errors.js and use it here

lib/errors.js

mcollina · 2022-10-26T09:29:23Z

test/defer.js

+    directive @defer(
+      if: Boolean! = true
+      label: String
+    ) on FRAGMENT_SPREAD | INLINE_FRAGMENT


Shouldn't this directive added automatically by mercurius?

I would do this behind a defer: true option, set to false by default.

Hey @mcollina I've added opts.defer, but I'm not quite sure if we really need that, since we decided to cut this off to a new release branch where we assume @defer will be available to everyone.

I can remove opts.defer and just leave its directive definition so that we always have this functionality with graphql@17.

What do you think?

mcollina · 2022-10-26T10:45:52Z

How could I ship this today and keep the current dependency at graphql@16?

igrlk · 2022-10-26T10:51:30Z

How could I ship this today and keep the current dependency at graphql@16?

I didn't update any gql-related dependencies in package.json and the whole change is not breaking, since the new functionality only works if graphql@17 is installed.

So I think it's safe to ship this once I incorporate the feedback and CI is green. Appreciate if you can help with the things you mentioned - otherwise I'll do this later today 🙂

mcollina · 2022-10-26T10:53:46Z

package.json

@@ -64,6 +64,7 @@
    "graphql": "^16.0.0",


I think we are not testing this behavior in CI, right? We should. The peerDependency might not be correct.

I could try adding CI tests that install graphql@17 if you can point me to the right direction - how and where you add them?

Also, what do you mean by peerDependency not correct? It didn't change in my PR, right?

How would an end user use defer? As you can see we depend upon graphql.

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

I'm happy to cut a next branch for you, so we can land this with the alpha version of graphql-js.

wdyt?

sounds good to me! so i finish the feedback first and then you create a next release branch?

I incorporated the feedback and going to see what's going on there with JIT compiler when I have some time

igrlk · 2022-10-26T20:12:39Z

test/defer.js

+    t.teardown(async () => {
+      await reader.releaseLock()
+      app.close()
+      process.exit()


Not sure if I wasn't iterating through the stream correctly, but the test never stops - added process.exit() to make it passing.
@mcollina can you please help here, maybe you see a problem in my code?

igrlk · 2022-10-27T09:15:18Z

@mcollina I believe you saw my issue in graphql-jit - looking forward to hearing back from them re graphql 17 support. Should we keep this PR open until they give us more details?

mcollina · 2022-10-27T09:41:45Z

It's likely better to wait until we hear back, I might be wrong but there is no urgency to ship given graphl@17 is still in alpha.

glasser · 2022-10-28T21:15:43Z

Apollo-Server changes are not yet in main branch, but in the next major version branch

Just to clarify: we have released support for defer/stream in Apollo Server 4 (which is now the current major version), but we are considering it experimental for now for various reasons including that it requires unreleased graphql-js.

Exciting to see Mercurius implementing it as well! Happy to see you're also requiring the client to opt in with deferSpec so that the whole ecosystem can cleanly adjust to any final changes that may show up before the GraphQL spec receives the final version.

mcollina

I've rebased this on top of the next branch.
We can land as soon as the tests pass

mcollina · 2022-10-26T21:15:28Z

test/defer.js

+const mercurius = require('../index')
+const { canUseIncrementalExecution } = require('../lib/util')
+
+if (canUseIncrementalExecution) {


This if should not be needed anymore.

mcollina · 2022-10-26T21:16:19Z

index.js

@@ -187,6 +189,20 @@ const plugin = fp(async function (app, opts) {
    })
  }

+  if (opts.defer) {


Could you throw if jit is enabled with defer?

I've added an error throwing when both jit and defer are enabled & updated the docs to have defer option there and in typescript definitions

mcollina · 2022-10-26T21:17:04Z

lib/util.js

+
+// istanbul ignore next
+function executeGraphql (args) {
+  if (canUseIncrementalExecution) {


This should happen only if defer is enabled.

mcollina · 2022-10-26T21:17:34Z

lib/util.js

@@ -23,7 +26,26 @@ function hasExtensionDirective (node) {
  }
 }

+const canUseIncrementalExecution = !!require('graphql/execution').experimentalExecuteIncrementally


not needed, this is always true now.

* Add space between merged SDLs to fix merging errors * Add unit tests for schema merging

…-js#901)

* feat: add types for object in graphiql configuration * feat: add test to types

…s-js#900) * Prevent parsing schema extensions * Add unit test

Signed-off-by: Matteo Collina <[email protected]>

…into defer-directive-support

igrlk · 2022-11-02T20:55:16Z

I've rebased this on top of the next branch.
We can land as soon as the tests pass

Thanks! I've made more changes that you requested.

I'm going to check if we can get the response from graphql-jit team or I can maybe make the necessary changes in their project, since we're blocked by that.

igrlk · 2022-11-14T20:42:56Z

I didn't get any response from graphql-jit so decided to make a PR to migrate it to graphql@17 - hopefully someone will respond on it.
zalando-incubator/graphql-jit#185

BorisTB · 2023-08-10T23:58:17Z

Hi! What needs to be done here to merge this? I’d really like to use this feature so I’ll gladly help.

mcollina · 2023-08-11T08:18:07Z

I think a public release of GraphQL v17?

added @defer support for requests with multipart/mixed; deferSpec=202…

cee50b7

…20824 accept header

igrlk mentioned this pull request Oct 25, 2022

feat: Implement @live, @stream, @defer #342

Open

mcollina requested changes Oct 26, 2022

View reviewed changes

igrlk added 2 commits October 26, 2022 10:02

reverted mercurius version bump, moved @defer tests to their own file

4ad38b4

remove 'Mercurius' from the error message about wrong accept header, …

08ee6d2

…removed unnecessary status code in the error

mcollina reviewed Oct 26, 2022

View reviewed changes

lib/errors.js Show resolved Hide resolved

mcollina reviewed Oct 26, 2022

View reviewed changes

igrlk added 3 commits October 26, 2022 19:31

bump graphql to 17.0.0-alpha.2

35fdb02

moved missing multipart accept header error into errors.js

d41fff6

added opts.defer: boolean to enable @defer directive

274c5bc

igrlk force-pushed the defer-directive-support branch from 92f04ad to 274c5bc Compare October 26, 2022 18:51

igrlk added 2 commits October 26, 2022 20:02

use @fastify/accepts instead of Negotiator package

3e3ca77

add @defer test with undici.fetch

2b3c25a

igrlk commented Oct 26, 2022

View reviewed changes

igrlk mentioned this pull request Oct 26, 2022

GraphQL 17 support? zalando-incubator/graphql-jit#184

Closed

igrlk mentioned this pull request Oct 30, 2022

Explicitly say in the docs that JIT is disabled by default #901

Merged

mcollina changed the base branch from master to next November 2, 2022 11:07

mcollina changed the base branch from next to master November 2, 2022 11:07

mcollina changed the base branch from master to next November 2, 2022 11:10

mcollina reviewed Nov 2, 2022

View reviewed changes

Igloczek and others added 3 commits November 2, 2022 12:11

Add space between merged SDLs to fix merging errors (mercurius-js#899)

369457e

* Add space between merged SDLs to fix merging errors * Add unit tests for schema merging

Explicitly say in the docs that JIT is disabled by default (mercurius…

780c668

…-js#901)

feat: add types for object in graphiql configuration (mercurius-js#907)

b4d70fc

* feat: add types for object in graphiql configuration * feat: add test to types

Igloczek and others added 9 commits November 2, 2022 15:46

Prevent parsing schema exceptions when importing directives (mercuriu…

e27e5b0

…s-js#900) * Prevent parsing schema extensions * Add unit test

Bumped v11.3.0

6933420

Signed-off-by: Matteo Collina <[email protected]>

Removed canUseIncrementalExecution check

d1d8485

Merge branch 'mercurius-js:master' into defer-directive-support

2d599a7

Merge branch 'defer-directive-support' of github.com:igrlk/mercurius …

ef73394

…into defer-directive-support

Throw an error if JIT is used together with defer, update the docs

640300f

Throw an error if JIT is used together with defer, update the docs

8b7e9b2

Merge branch 'defer-directive-support' of github.com:igrlk/mercurius …

7a84f17

…into defer-directive-support

Merge branch 'next' into defer-directive-support

093d0df

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: @defer support #898

feat: @defer support #898

igrlk commented Oct 25, 2022 •

edited

Loading

mcollina left a comment

mcollina Oct 26, 2022

mcollina Oct 26, 2022

mcollina Oct 26, 2022

mcollina Oct 26, 2022

mcollina Oct 26, 2022

igrlk Oct 26, 2022

mcollina commented Oct 26, 2022

igrlk commented Oct 26, 2022

mcollina Oct 26, 2022

igrlk Oct 26, 2022

mcollina Oct 26, 2022

igrlk Oct 26, 2022 •

edited

Loading

mcollina Oct 26, 2022

igrlk Oct 26, 2022

mcollina Oct 26, 2022

igrlk Oct 26, 2022

igrlk Oct 26, 2022

igrlk commented Oct 27, 2022

mcollina commented Oct 27, 2022

glasser commented Oct 28, 2022

mcollina left a comment

mcollina Oct 26, 2022

mcollina Oct 26, 2022

igrlk Nov 2, 2022

mcollina Oct 26, 2022

mcollina Oct 26, 2022

igrlk commented Nov 2, 2022

igrlk commented Nov 14, 2022

BorisTB commented Aug 10, 2023

mcollina commented Aug 11, 2023

feat: @defer support #898

Are you sure you want to change the base?

feat: @defer support #898

Conversation

igrlk commented Oct 25, 2022 • edited Loading

🤔 What is @defer and why do we need that?

⌛ Current state of things

📦 What this PR does

🧰 Local development / testing

⚠️ Things that aren't yet done

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Oct 26, 2022

igrlk commented Oct 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrlk Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrlk commented Oct 27, 2022

mcollina commented Oct 27, 2022

glasser commented Oct 28, 2022

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igrlk commented Nov 2, 2022

igrlk commented Nov 14, 2022

BorisTB commented Aug 10, 2023

mcollina commented Aug 11, 2023

igrlk commented Oct 25, 2022 •

edited

Loading

igrlk Oct 26, 2022 •

edited

Loading