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

separateOperations: stop unused fragments from being included #2853

Closed
wants to merge 1 commit into from

Conversation

filipstefansson
Copy link

The separateOperations method sometimes include unused fragments if you name an operation the same thing as a fragment:

fragment One on T {
  oneField
}

fragment Two on T {
  twoField
}

query One {
  ...Two
}

query Ones {
  ...One
}

When running separateOperations on the above, and then looking at .Ones of the result, we get:

fragment One on T {
  oneField
}

## NOT SUPPOSED TO BE HERE ##
fragment Two on T {
  twoField
}

query Ones {
  ...One
}

This happens because in the dependency graph, there's nothing differentiating an operation from a fragment. They are just added by name. So in the scenario above the fragment One will add the query One as a dependecy of Ones, and then the query One has the fragment Two as a dependency, so that will be added too.

I fixed this by adding a prefix to the names in the dependecy graph, so operations can't be added a dependencies anymore, only fragments.

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Nov 24, 2020
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Nov 24, 2020
@IvanGoncharov
Copy link
Member

@filipstefansson Thanks for taking the time to report it and provide a solution.
Adding prefixes is a valid approach but I decided to take this opportunity and refactor this function in #2859.

saihaj pushed a commit to saihaj/graphql-js that referenced this pull request Jan 12, 2021
yaacovCR pushed a commit to yaacovCR/graphql-executor that referenced this pull request Apr 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants