-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Adds @defer
support
#10018
Adds @defer
support
#10018
Conversation
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.
Some initial thoughts. Overall this is looking very promising — thanks @alessbell!
116a37e
to
f035994
Compare
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 |
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.
🙏🏻 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?
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.
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.
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.
If I exposed a version that avoided them, would the idea be entertained to be used here?
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.
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!
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.
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 👍🏻
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.
👋 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.
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.
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.
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.
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 exploringmeros
(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 theresponse-iterator
library inline to help keep bundle sizes down, but have absolutely attributed the author on all of those code blocks, and have includedresponse-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.
24b994a
to
51fa990
Compare
51fa990
to
3a4fbde
Compare
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.
Another round of suggestions / nits / food for thought. This is looking great @alessbell!
#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.
05a0694
to
fd830d6
Compare
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
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.
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.
@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.
Closes #8184 and closes #10019.
This PR adds support for the
@defer
directive. It allows the client to parse themultipart/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
Checklist: