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

feat: fragmentifyDocument utility #4025

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

samuelAndalon
Copy link

@samuelAndalon samuelAndalon commented Mar 7, 2024

This PR introduces a utility function fragmentifyDocument that significantly optimize GraphQL ASTs by programmatically converting InlineFragments to FragmentDefinitions and replace these inlineFragments with FragmentSpreads. This transformation aims to enhance the efficiency of operation parsing, validation and execution, especially for large and complex queries.

GraphQL operations can become quite large and complex, leading to increased parsing time and execution latency. This is particularly evident with extensive use of InlineFragments, which, while flexible and powerful, can add significant overhead to query processing. By extracting these inline fragments into separate FragmentDefinitions and referencing them with FragmentSpreads, we can significantly reduce the complexity and size of the initial query sent to the server.

The optimization would consist of:

  1. Traverse the AST.
  2. Search for InlineFragment nodes in SelectionSet nodes.
  3. Create a FragmentDefinition of the SelectionSet of that InlineFragments.
  4. Append that FragmentDefinition to the document AST.
  5. replace the InlineFragment in the SelectionSet with a FragmentSpread.

This optimization is scoped for operations that heavily rely in polymorphism. It doesn't need a graphQL schema at all, only needs the AST.

I have a test suite pending to add, (already have the tests), but we (ExpediaGroup) tried this in production giving us a 100x smaller operations.

This PR was originally raised against Apollo Federation, however, this utility function applies to ASTs and to any GraphQL implementation specification, so graphql-js sound like the best place to have this.

Copy link

linux-foundation-easycla bot commented Mar 7, 2024

CLA Missing ID CLA Not Signed

Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 3b0c035
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/65e93dd0f8882b0008799657
😎 Deploy Preview https://deploy-preview-4025--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.

@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 1, 2024

@samuelAndalon this looks extremely useful! I think it might fit very well into graphql-tools as that is more of a general toolbox, but it could make sense within the reference implementation if the WG ever formalizes something in the spec with regard to canonical documents of some kind.

What do you think?

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