fix(stream): continue on iterator error values#2980
Closed
yaacovCR wants to merge 7 commits intographql:asynciteratablefrom
Closed
fix(stream): continue on iterator error values#2980yaacovCR wants to merge 7 commits intographql:asynciteratablefrom
yaacovCR wants to merge 7 commits intographql:asynciteratablefrom
Conversation
Co-authored-by: Ivan Goncharov <[email protected]>
# Conflicts: # src/execution/execute.js
… resovlers # Conflicts: # src/execution/execute.js
Contributor
Author
|
@robrichard @IvanGoncharov this is more of a question than a PR -- what is the desired behavior? |
Contributor
|
I don't think it's expected to receive more values after an async iterable throws an error. How is this handled with subscriptions? |
Contributor
Author
|
That’s just it, this code is not about the iterator returning an error, that’s further down. This code section is an error encountered in completion of a list item. With iterables, the next item is allowed to be processed, shouldn’t it be the same with async iterables? |
Contributor
Author
|
Added tests to explain |
Contributor
Contributor
Author
|
Yes! Will rework when I have a moment to target that branch |
yaacovCR
force-pushed
the
continue-on-async-iterator-error-value
branch
from
8ee3876 to
cf0f829
Compare
yaacovCR
force-pushed
the
continue-on-async-iterator-error-value
branch
3 times, most recently
from
aec6fd9 to
ac0de32
Compare
if the iterator errors when attempting to get the next value, we can assume subsequent calls to next will also error, and abort, but if we successfully get the next value, but it ends up triggering an error, we can continue, optimistic that the next value will not do so.
yaacovCR
force-pushed
the
continue-on-async-iterator-error-value
branch
from
ac0de32 to
4b1547d
Compare
Contributor
Author
|
ready for review |
yaacovCR
added a commit
to ardatan/graphql-tools
that referenced
this pull request
May 1, 2021
robrichard
force-pushed
the
asynciteratable
branch
2 times, most recently
from
f1ddb83 to
10f12a1
Compare
Contributor
Author
|
Bump, would this be accepted if rebased? |
IvanGoncharov
added
the
stream/defer
Contributor
|
@yaacovCR I pulled this in while rebasing. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
if the iterator errors when attempting to get the next value, we can assume subsequent calls to next will also error, and abort, but if we successfully get the next value, but it ends up triggering an error, we can continue, optimistic that the next value will not do so.