Adds `@defer` support #10018

alessbell · 2022-08-18T18:16:36Z

Closes #8184 and closes #10019.

This PR adds support for the @defer directive. It allows the client to parse the multipart/mixed responses returned by servers that support @defer.

Testing

This branch can be built and run against apollographql/supergraph-demo-fed2#212 via https://github.com/jpvajda/ac-defer-example.

Code coverage

File	% Stmts	% Branch	% Funcs	% Lines
src/link/http/responseIterator.ts	95	100	100	93.33
src/link/http/iterators/async.ts	80	100	66.66	80
src/link/http/iterators/nodeStream.ts	82	69.23	63.63	84.78
src/link/http/iterators/promise.ts	92.85	100	80	92.85
src/link/http/iterators/reader.ts	87.5	100	66.66	87.5
src/utilities/common/responseIterator.ts	100	100	100	100

Checklist:

If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests

benjamn

Some initial thoughts. Overall this is looking very promising — thanks @alessbell!

src/link/core/types.ts

src/link/http/responseIterator.ts

src/link/http/iterators/nodeStream.ts

src/link/http/iterators/async.ts

src/utilities/common/responseIterator.ts

src/core/QueryInfo.ts

maraisr · 2022-08-31T01:59:29Z

src/link/http/parseAndCheckHttpResponse.ts

-        throw parseError;
+  const decoder = new TextDecoder("utf-8");
+  const ctype = getContentTypeHeaders(response);
+  // Adapted from meros https://github.com/maraisr/meros/blob/main/src/node.ts


🙏🏻 thanks for mentioning me here. Was wondering if you dont mind me asking, if there was any shortcomings of meros to not just be used directly?

Hi @maraisr 👋 Really appreciate your work on meros! I opted to avoid async function* syntax which would often require transpilation by our end users, and thus slightly larger bundle sizes.

If I exposed a version that avoided them, would the idea be entertained to be used here?

No, I don't think so - in the end we took a different approach to stream normalization altogether, that was just the initial reason :) Thanks again for your interest!

The code to me looks next to identical, you just stripped the generator parts. 🤔 would have loved to have worked with you, be sure to include meros' license when your release in anycase 👍🏻

👋 I've re-opened this discussion so we can continue to engage in the dialog about this section of code. in this commit we've refactored the approach based on the concerns raised by @maraisr.

I guess renaming some variables and moving code up or down constitute as new code? I called it current you called it message, I called it next you called it buffer. And if you scroll through the history of these commits, you can see it was all derivative work.

The entire block was all mine you adapted to remove the generates and preamble aspects, and remove the performance improvement about multi payload chunks.

Just have some compassion here, I spent weeks of my life crafting that library, learning/understanding the RFC, reasoning with folk about getting it to behave differently. Got tests to pass, worked with other server folk and clients to get it to where it is today. Lots of effort and time, for you guys to now come in, and claim it as your own. I know I cannot reason with plagiarists, so cut my losses and that is that.

All the best.

Hi @maraisr - sorry to hear this has upset you. This was definitely not our intent. Here's a quick timeline of events explaining how we got to this point:

When we started working on @defer support, a teammate spiked out various approaches that could work. This included exploring meros (which you know as you were in a few discussions about it) and he ultimately landed on something that was a combination of ideas, but very much his own code.

That teammates work sat for a bit while we focused on other things, but was picked back up in earnest about a month ago (by others - the original author is no longer on the project). As part of picking it back up, we blew the dust off things a bit, and wanted to make sure the planned approach still made sense. To this end we re-explored a few options, one of which was using out of the box meros.

meros is great, but there are a few things keeping us from using it (generators, we need to keep bundle sizes down as much as possible, etc.). We decided to continue with our own code where we could, and have definitely attributed other authors where it makes sense to do so. For example, we've leveraged parts of the response-iterator library inline to help keep bundle sizes down, but have absolutely attributed the author on all of those code blocks, and have included response-iterator as a dependency so the author gets download counts.

We originally had a very small block of code from meros in place, which we attributed, but have since refactored things so the attribution was dropped.

The original author of this work was definitely looking at how other libraries were handling things like boundary parsing, and meros was one of those libraries. We've reviewed the code multiple times and don't see signs of plagiarism, but also don't want to upset any community members. Could you share more specific details about which parts of the code you're upset about? We'll review those blocks further and add proper attribution if it's missing. We definitely understand the hard work that goes into building OSS libraries and tooling, and the importance of making sure everyone is properly attributed for their efforts.

benjamn

Another round of suggestions / nits / food for thought. This is looking great @alessbell!

src/link/http/createHttpLink.ts

src/utilities/common/responseIterator.ts

src/utilities/common/canUse.ts

src/core/QueryInfo.ts

src/core/QueryManager.ts

src/core/QueryInfo.ts

src/link/http/iterators/async.ts

src/link/http/__tests__/responseIteratorNoAsyncIterator.ts

#10018 (comment)

#10018 (comment) Note: this does change the name and meaning of subscribeAndCount's generic type parameter from TData to TResult, which is typically either an ApolloQueryResult<TData> or FetchResult<TData>, though the exact type is now fully generic and typically inferred from the argument types passed to subscribeAndCount. Anyone who has been importing subscribeAndCount from `@apollo/client/testing/core` and explicitly providing the `subscribeAndCount<TData>(...)` type parameter (rather than letting it be inferred from the arguments) may need to adjust their code accordingly. While this is technically a breaking change at the level of the type system, it seems unlikely to be very disruptive, since TypeScript should catch any inconsitencies at build time, and subscribeAndCount is an optional testing utility, and thus not likely to be used in production.

The label property seems to belong to the incremental chunks, rather than to the execution result that contains the incremental array: https://deploy-preview-742--graphql-spec-draft.netlify.app/draft/#sec-Label

https://deploy-preview-742--graphql-spec-draft.netlify.app/draft/#sec-Path

Since queryInfo.lastDiff is essentially a cache for queryInfo.getDiff(), I think we should call the public getDiff method instead of assuming lastDiff is defined.

Since tests are still passing without setting these properties to undefined, I'd prefer to keep the markResult method from modifying its input (other than updating result.data).

This allows us to reuse mutable objects created by previous merges without copying them again.

benjamn

@alessbell I'm happy with the current state of this PR, though I will leave it to you to review my recent commits and merge into release-3.7 whenever you're ready.

alessbell added 3 commits August 16, 2022 15:57

chore: update gitignore

8411bbe

chore: fix could not find TS at this path VSC error

9183c85

feat: add response-iterator dependency

b96a9d4

alessbell mentioned this pull request Aug 18, 2022

Add @defer support to Apollo Client #9598

Closed

benjamn reviewed Aug 18, 2022

View reviewed changes

This was referenced Aug 18, 2022

@defer support apollo-elements/apollo-elements#255

Closed

Align @defer v1 implementation with latest proposed spec updates #10019

Closed

alessbell marked this pull request as ready for review August 22, 2022 12:42

benjamn reviewed Aug 22, 2022

View reviewed changes

src/utilities/common/responseIterator.ts Outdated Show resolved Hide resolved

src/core/QueryInfo.ts Outdated Show resolved Hide resolved

alessbell added 15 commits August 24, 2022 10:23

feat: add response iterator handlers

75b8bee

chore: copy over QueryInfo diff

b2b86d8

chore: bump bundlesize to 31.3kb (was 31.23kb)

bba1d1b

feat: add ExecutionPatchResult type

eab2116

feat: add multipart handling in createHttpLink

789cea1

chore: update MockedSubscriptionResult to include patch result type

5a3d05b

chore(tests): add HttpLink and useQuery multipart tests

fc10386

chore(deps): add @types/node-fetch and web-streams-polyfill dev deps

af97ea7

chore(tests): remove irrelevant tests

80937c3

chore(refactor): call observer.complete inside read functions

6e9fbf5

chore: use Promise returned by iterator.next over for await

7542b0e

chore: remove comments from createHttpLink

8a1bc63

fix: decode value

40ea9be

fix: avoid redefining inherited fields on ExecutionPatchResult

71ad7a5

fix: avoid final cast to AsyncIterableIterator in async.ts

ba26eae

alessbell force-pushed the issue-8184-defer-support branch from 116a37e to f035994 Compare August 24, 2022 14:31

jpvajda mentioned this pull request Aug 30, 2022

@defer documentation #10057

Closed

3 tasks

maraisr reviewed Aug 31, 2022

View reviewed changes

jpvajda added the 🛫 in-progress label Sep 1, 2022

jpvajda assigned alessbell Sep 1, 2022

jpvajda mentioned this pull request Sep 2, 2022

Improve the @defer client side demo in supegraph fed 2 demo #10067

Closed

alessbell added 5 commits September 7, 2022 11:32

fix: bug in incremental result merge

307f747

fix: silence console error on final hasNext: false chunk

1644b94

chore: refactor boundary parsing

04626f0

fix: error handling with errorPolicy: all and add tests

d26a1a6

fix: add tests and update error handling

ac86fc4

alessbell force-pushed the issue-8184-defer-support branch from 24b994a to 51fa990 Compare September 7, 2022 15:34

chore: bump bundlesize to 31.4kb (was 31.3kb)

3a4fbde

alessbell force-pushed the issue-8184-defer-support branch from 51fa990 to 3a4fbde Compare September 7, 2022 15:59

alessbell added 2 commits September 7, 2022 15:40

chore: finalize TS types and fix single tsc error

9cbcde8

chore(tests): fix 2 of 3 remaining responseIterator tests

91c347a

benjamn reviewed Sep 7, 2022

View reviewed changes

alessbell and others added 4 commits September 8, 2022 08:31

fix: PR review comments

6f073ed

chore: remove unnecessary cast and clone readonly errors

ecbd8b5

Optimize hasDirectives to avoid linear indexOf for every DirectiveNode.

6d3ad04

#10018 (comment)

benjamn force-pushed the issue-8184-defer-support branch from 05a0694 to fd830d6 Compare September 8, 2022 17:39

benjamn added 5 commits September 8, 2022 17:36

Move label to ExecutionPatchIncrementalResult.incremental chunks.

fd1e61a

The label property seems to belong to the incremental chunks, rather than to the execution result that contains the incremental array: https://deploy-preview-742--graphql-spec-draft.netlify.app/draft/#sec-Label

Use key type (string | number) to choose array or object.

b404782

https://deploy-preview-742--graphql-spec-draft.netlify.app/draft/#sec-Path

Call queryInfo.getDiff() instead of using .lastDiff directly.

b7854e2

Since queryInfo.lastDiff is essentially a cache for queryInfo.getDiff(), I think we should call the public getDiff method instead of assuming lastDiff is defined.

Avoid removing properties from result passed to QueryInfo#markResult.

a5ac881

Since tests are still passing without setting these properties to undefined, I'd prefer to keep the markResult method from modifying its input (other than updating result.data).

Reuse DeepMerger instance within each result.incremental batch.

b191ddf

This allows us to reuse mutable objects created by previous merges without copying them again.

benjamn approved these changes Sep 8, 2022

View reviewed changes

alessbell merged commit f2bb099 into release-3.7 Sep 8, 2022

benjamn mentioned this pull request Sep 8, 2022

Improvements to the hasDirectives utility function #10082

Merged

benjamn added a commit that referenced this pull request Sep 8, 2022

Mention PR #10018 in CHANGELOG.md.

818d822

benjamn deleted the issue-8184-defer-support branch September 15, 2022 18:56

igrlk mentioned this pull request Oct 25, 2022

feat: @defer support mercurius-js/mercurius#898

Open

1 task

alessbell removed the 🛫 in-progress label Nov 9, 2022

alessbell changed the title ~~Adds @defer support~~ Adds @defer support Jan 19, 2023

github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `@defer` support #10018

Adds `@defer` support #10018

alessbell commented Aug 18, 2022 •

edited

benjamn left a comment

maraisr Aug 31, 2022

alessbell Sep 1, 2022

maraisr Sep 1, 2022

alessbell Sep 1, 2022

maraisr Sep 1, 2022

jpvajda Sep 1, 2022

maraisr Sep 1, 2022 •

edited

hwillson Sep 2, 2022

benjamn left a comment

benjamn left a comment

Adds @defer support #10018

Adds @defer support #10018

Conversation

alessbell commented Aug 18, 2022 • edited

Testing

Code coverage

Checklist:

benjamn left a comment

Choose a reason for hiding this comment

maraisr Aug 31, 2022

Choose a reason for hiding this comment

alessbell Sep 1, 2022

Choose a reason for hiding this comment

maraisr Sep 1, 2022

Choose a reason for hiding this comment

alessbell Sep 1, 2022

Choose a reason for hiding this comment

maraisr Sep 1, 2022

Choose a reason for hiding this comment

jpvajda Sep 1, 2022

Choose a reason for hiding this comment

maraisr Sep 1, 2022 • edited

Choose a reason for hiding this comment

hwillson Sep 2, 2022

Choose a reason for hiding this comment

benjamn left a comment

Choose a reason for hiding this comment

benjamn left a comment

Choose a reason for hiding this comment

Adds `@defer` support #10018

Adds `@defer` support #10018

alessbell commented Aug 18, 2022 •

edited

maraisr Sep 1, 2022 •

edited