Skip to content

Conversation

@robrichard
Copy link
Contributor

Implementing what was discussed in graphql/defer-stream-wg#45

@netlify
Copy link

netlify bot commented Aug 31, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit e0a539c
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/631b2bb4f150260008fcaed9
😎 Deploy Preview https://deploy-preview-3720--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@robrichard robrichard requested a review from benjie August 31, 2022 19:27
@github-actions
Copy link

Hi @robrichard, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@robrichard robrichard added the stream/defer Issues/PRs related to experimental steam/defer support label Aug 31, 2022
@benjie
Copy link
Member

benjie commented Sep 1, 2022

I don't think this is the approach I would have taken. Instead:

  1. When a nullable position becomes null because of an error that has bubbled (no need to handle when the field itself throws an error), add this path to a new set on exeContext (let's call the new set errorNulledPaths since it's a list of the paths that were nulled out due to an error caught by a nullable type)
  2. In filterSubsequentPayloads, find and cancel the subsequentPayloads whose path starts with or matches one of the paths in errorNulledPaths - no need to traverse the response data

Assuming that the length of paths is negligible, I think this basic approach would scale at O(N*M) where N is the number of errors and M is the number of subsequent payloads, but I'm sure there are clever ways to optimize it better.

@robrichard robrichard force-pushed the robrichard/null-parallel branch from 591a71a to 4205d65 Compare September 1, 2022 22:49
@robrichard
Copy link
Contributor Author

@benjie I significantly rewrote this. Now, as discussed in the WG today, I've added the logic to filter subsequent payloads at the point where the bubbled null is ultimately set. This no longer requires inspecting the payload response and should not add any overhead in the non-error case.

@robrichard robrichard requested a review from yaacovCR September 7, 2022 21:57
@benjie
Copy link
Member

benjie commented Sep 8, 2022

Definitely looks better; thanks @robrichard!

@robrichard robrichard changed the title strip subsequent payloads when parent field is null Filter subsequent payloads when parent field is null Sep 9, 2022
@robrichard robrichard force-pushed the robrichard/null-parallel branch from 4205d65 to e0a539c Compare September 9, 2022 12:04
@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Sep 14, 2022
@IvanGoncharov IvanGoncharov merged commit b9a2695 into graphql:main Sep 14, 2022
@robrichard robrichard deleted the robrichard/null-parallel branch September 14, 2022 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: bug fix 🐞 requires increase of "patch" version number stream/defer Issues/PRs related to experimental steam/defer support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants