-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add support for @stream
#12918
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: jerel/new-incremental
Are you sure you want to change the base?
Add support for @stream
#12918
Conversation
🦋 Changeset detectedLatest commit: b05df03 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 |
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: d7e01a4f6a371b1e587d26b1 |
commit: |
This reverts commit dba0d09.
707aa69
to
411b132
Compare
{ __typename: "Friend", id: "3" }, | ||
], | ||
}), | ||
dataState: "streaming", |
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.
This may sound controversial, but this test makes me wonder if streaming
is the wrong value here (note: only for @stream
, not for @defer
). While the array merging behavior is fixed in #12923 (which actually removes the partial results here), it would still be possible to define a merge function that keeps the partial results in this list, which means that streaming
is incorrect here and partial
should actually be the value.
With @stream
, we don't know the total length of the array, nor are there "missing" fields in the result like you have with @defer
, so even at a type level, streaming
doesn't really change TData
when used with @stream
. complete
or partial
feels a bit more correct here and why I think relying more on the networkStatus: NetworkStatus.streaming
to determine if the list is still streaming in makes sense.
Would love to talk through this to get thoughts on this.
096db02
to
b05df03
Compare
Closes #12864
Add support for the
@stream
directive for both the[email protected]
and[email protected]
formats.This has a missing component to it which we'll add in a followup PR which allows you to determine how to merge the arrays. Currently the arrays are merged from cache by deep merging the array items at the same indexes. This however means that if a streamed list returns fewer items than the cached list, the items at the end of the cached list are retained. This would not be the case if the
@stream
directive were used.For that we plan to add the ability to specify the merge strategy, but this will be done in a followup PR. These changes should be good enough to work with a base implementation of stream.