Skip to content
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

Alternative spec implementation of deduplicated incremental delivery #1077

Closed
wants to merge 39 commits into from

Conversation

yaacovCR
Copy link
Contributor

This is an alternative to my most recent PR #1052 with an eye to describing the algorithm in a bit less detail to decrease the size of the spec edits. We also introduce the idea of an observable stream to which events can be pushed using a callback.

This is an alternative as well to @benjie's most recent proposal at #1074.

The main differences as I see them within the given proposal are as follows.

This PR:

  1. will kick off incremental entries shared between sibling defers and unique to sibling defers in parallel rather than waiting for the former to complete.
  2. generates a path-independent field plan for each field within the operation; implementations can use this to memoize/deduplicate if using defer on list items; as well as persisting this across operations.
  3. provides a single-line difference in implementation between early and delayed execution; the algorithm is otherwise the same.

This is also of course an alternative to #742 -- the original PR without deduplication.

Copy link

netlify bot commented Feb 12, 2024

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 3ef628c
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/66eb131235e7a90008df92e2
😎 Deploy Preview https://deploy-preview-1077--graphql-spec-draft.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 configuration.

@yaacovCR yaacovCR marked this pull request as draft February 12, 2024 15:04
@yaacovCR yaacovCR force-pushed the deduplicate5 branch 2 times, most recently from f8d3cf3 to f20ce04 Compare February 13, 2024 20:09
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Feb 13, 2024

UPDATED 2024-02-22:

I've separated out stream from this PR for the sake of simplicity; the commit adding defer comes at the tail end of this PR, with some restructuring and moving happening in the initial commits.

The diff for the defer commit is +322 and -51, on net an addition of 271 lines.

For comparison, proposal at #1074 has a diff of +328 and −45, on an addition of 283 lines. (That is the total for the entirety of #1074, as that PR is set to be pulled into a branch already containing essentially the same restructuring work.)

So this proposal by length is on par with #1074, but we also get the additional features mentioned above: (1) parallel execution of all incremental entries (including where sibling defers have overlapping and unique fields), (2) a demonstration of the path-independence of field collection and grouped field set partition, and (3) explicit handling of early execution via 1-line if statements.

To make this magic happen, we do have to expand a bit our spec language. In particular, we add the ability to pass around an event emitter to the data execution layer that can be used to defer execution of deferred fragments until the incremental result stream signals that it has released a fragment as pending. We otherwise retain the simple functional style of the remainder of the spec => and we don't perform any mutations.

I will be updating this PR, probably via rebasing. If something significant changes, I will try to highlight it in the comments here, but basically the plan is just to further simplify, cut down on verbiage, etc.

@yaacovCR yaacovCR force-pushed the deduplicate5 branch 8 times, most recently from d1a5495 to c904162 Compare February 21, 2024 18:09
@yaacovCR
Copy link
Contributor Author

Here are some initial visualizations of what the graph in YieldIncrementalResults might look like

For operation:

{
  someFieldA
  ... @defer(label: "IntermediateComponent") {
    someFieldB
    ... @defer(label: "SlowComponent") {
      someFieldC
    }
  }
}
graph TD
    A1((IntermediateComponent)) --> B1{"{ someFieldB }"}
    A1 --> A2((SlowComponent))
    A2 --> B2{"{ someFieldC }"}
Loading

For operation:

{
  someFieldA
  ... @defer(label: "Sibling1") {
    someFieldB
    someFieldX
  }
  ... @defer(label: "Sibling2") {
    someFieldB
    someFieldY
  }
}
graph TD
    A1((Sibling1)) --> B1{"{ someFieldX }"}
    A1 --> B2{"{ someFieldB }"}
    A2((Sibling2)) --> B2
    A2 --> B3{"{ someFieldY }"}
Loading

yaacovCR and others added 25 commits September 18, 2024 20:42
instead of sometimes fieldGroup, for consistency and so as to remove another "Group" term
"node" should be "field" within CreateSourceEventStream

Co-authored-by: Rob Richard <robrichard87@gmail.com>
accompanying note is a WIP, open to further suggestions as to how to clarify
* Add Response Section for defer/stream

* review fixes

* fix
* Add defer and stream directives to type system

* Add defer/stream validation rules
@yaacovCR
Copy link
Contributor Author

Closing in favor of #1110 !!!

@yaacovCR yaacovCR closed this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants