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

incremental: lazily create DeferredFragments #4153

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jul 25, 2024

depends on #4152

goal:

avoid creating or passing around the deferMap

methodology:

  • each DeferredFragmentRecord will be unique for a given DeferUsage and creation Path.
  • we annotate the DeferUsage with a depth property representing the path depth in the response for wherever this DeferUsage is used.
  • from a given ExecutionGroup, we can derive the path for its DeferredFragmentRecord from the DeferUsage and the ExecutionGroup path by "rewinding" the ExecutionGroup path to the depth annotated on the corresponding DeferUsage.

note:

for performance, we now save the depth of the path on the path elements themselves. this could always be replaced by a call to pathToArray as path.depth === pathToArray(path).length)

@yaacovCR yaacovCR requested a review from a team as a code owner July 25, 2024 13:44
Copy link

netlify bot commented Jul 25, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 60f92c0
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/66c624c4f84c460008fa4075
😎 Deploy Preview https://deploy-preview-4153--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 configuration.

Copy link

Hi @yaacovCR, 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

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jul 25, 2024

Comparing to #4150, we now annotate the DeferUsage with the actual depth in the response rather than the field depth in the operation. This is simpler and results in no change No Actual Change to the path interface (although technically we still change it by annotating it with the depth, this could always be derived later).

@yaacovCR yaacovCR marked this pull request as draft July 25, 2024 13:49
@yaacovCR yaacovCR requested a review from robrichard July 25, 2024 16:41
yaacovCR added a commit to yaacovCR/graphql-spec that referenced this pull request Jul 25, 2024
@yaacovCR
Copy link
Contributor Author

tentative spec changes to go along with this draft implementation PR are at yaacovCR/graphql-spec#3

@yaacovCR yaacovCR marked this pull request as ready for review July 26, 2024 05:27
@yaacovCR
Copy link
Contributor Author

once we no longer create the deferMap, collectFields also does not need to return the newDeferUsages.

we do still return a boolean if defers were encountered to update the context so that buildExecutionPlan will be called; this allows us to still skip buildExecutionPlan in the wholly non-incremental case, somewhat narrowing the scope of that optimization

@yaacovCR yaacovCR changed the title experiment(incremental): lazily create DeferredFragments incremental: lazily create DeferredFragments Jul 26, 2024
@yaacovCR yaacovCR force-pushed the still-lazy-create branch 2 times, most recently from 3c6b5c1 to 66b9dc0 Compare August 11, 2024 04:35
@yaacovCR

This comment has been minimized.

Copy link

@github-actions publish-pr-on-npm

@yaacovCR Something went wrong, please check log.

Copy link

@github-actions publish-pr-on-npm

@yaacovCR The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.5.canary.pr.4153.4ff43175428332c954563050819fcb612e19ca41
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-4153

@yaacovCR yaacovCR force-pushed the still-lazy-create branch 2 times, most recently from 3757ea7 to 78e9df7 Compare August 13, 2024 23:24
@yaacovCR

This comment has been minimized.

Copy link

@github-actions publish-pr-on-npm

@yaacovCR The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.5.canary.pr.4153.d5c18bebb93273daf40fce67daa1babc430a2ce2
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-4153

goal:

avoid creating or passing around the deferMap

methodology:

each DeferredFragmentRecord will be unique for a given deferUsage and creationPath
- we annotate the deferUsage with a "depth" property representing the path length in the response for wherever this defer is delivered.
- from a given execution group path, we can derive the path for the deferredFragment for a given deferUsage by "rewinding" the execution group path to the depth annotated on the given deferUsage
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.

2 participants