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

fix: skip include logic handling multiple paths to same node #197

Merged
merged 4 commits into from
Mar 30, 2023

Conversation

boopathi
Copy link
Member

@boopathi boopathi commented Mar 13, 2023

TL;DR

To use the new behaviour, use the new CompilerOption -

compileQuery(
  schema, ast, operationName,
  {
    useExperimentalPathBasedSkipInclude: true
  }
)

Description

When there are multiple paths to the same field Node (fragments), there can be multiple skip/include requirements for the same node. This breaks our assumption in the existing implementation where we compute the skip/include for a particular field node and use the same for all paths.

For example,

query {
  paths {
    one {
      ...frag @skip(if: true)
    }
    two {
      ...frag @skip(if: false)
    }
  }
}
fragment frag on Item {
  target
}

Here, in this query, the field target has two paths - paths.one.target, and paths.two.target. But, in the current implementation we compute __internalShouldInclude for a particular field node - so this gets reused for all paths to the field Node - so is the reason for the bug.

This PR fixes it by remodelling the internal representation. This PR uses a path specific skip/include logic and while generating code, it uses the path specific logic. We represent this as an object - __internalShouldIncludePaths[path.in.dot.notation] = SkipIncludeCode for that path.

@boopathi boopathi marked this pull request as draft March 13, 2023 17:27
@boopathi boopathi marked this pull request as ready for review March 15, 2023 12:20
@boopathi
Copy link
Member Author

👍

1 similar comment
@ayruslore
Copy link

👍

@Joneser
Copy link

Joneser commented Mar 29, 2023

Do you think it would be possible to merge this in order to unblock #177 ?

src/ast.ts Outdated Show resolved Hide resolved
@boopathi
Copy link
Member Author

👍

1 similar comment
@oporkka
Copy link
Member

oporkka commented Mar 30, 2023

👍

@oporkka oporkka merged commit 6a3a36a into main Mar 30, 2023
@oporkka oporkka deleted the skip-fragment-spread-1 branch March 30, 2023 12:29
@boopathi boopathi mentioned this pull request Mar 30, 2023
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.

misbehaving spreading fragments
4 participants