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

create new deferred grouped field set records lazily #3998

Closed
wants to merge 1 commit into from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Dec 19, 2023

Before #3984, we could not do this because of problems with empty defers. Now, it's a safe optimization.

Consider:

      query {
        alwaysThrows
        ... @defer(name: "WillBeEmpty") {
          someField
        }
      }

Before the current change, when alwaysThrows throws, the deferred grouped field set for someField would be found to be in the "blast radius", and deferred fragment WillBeEmpty would be filtered. This would yield:

    {
      "data": {
        "alwaysThrows": null,
      },
      "errors": [ ... ]
    }

Prior to #3984, if we had made the current change and created the deferred grouped field set for someField lazily, someField would not be found to be in the "blast radius" -- because it would not have been created yet -- and WillBeEmpty would not have been filtered.

However, with #3984, WillBeEmpty is not sent because all empty deferred fragments are never sent and do not require filtering. This is now a safe change.

@yaacovCR yaacovCR added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Dec 19, 2023
Copy link

netlify bot commented Dec 19, 2023

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 464af2b
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/65816f461c53bd0008139940
😎 Deploy Preview https://deploy-preview-3998--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

Note that there is a tweak/fix for #3984 in #3993 for subsequent results.

@yaacovCR
Copy link
Contributor Author

Actually, wait => this is still not safe!

What if a deferred fragment that should be filtered is never filtered, but it wasn't otherwise empty?

Consider:

      query {
        someRootField {
          alwaysThrows
        }
        ... @defer(name: "WillNotBeEmpty") {
          someRootField {
            someNestedField
          }
          someOtherRootField
        }
      }

By the time alwaysThrows throws, a deferred grouped field set could be created for someOtherRootField (if someRootField itself is async, for example) and so deferred fragment WillNotBeEmpty will not be empty. But it cannot be delivered, because it should be found to be in the blast radius if alwaysThrows by someRootField.

@yaacovCR yaacovCR closed this Dec 19, 2023
@yaacovCR
Copy link
Contributor Author

TODO: add test for above

@yaacovCR yaacovCR deleted the lazy branch September 5, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant