Improve directive and fragment removal logic #6322
Conversation
jpvajda
added
🏓 awaiting-contributor-response
There was a problem hiding this comment.
Hey @mtsmfm 👋! Thanks for your patience! I've noticed your 2 tests seem to conflict in logic with each other. I'd like to see if we can keep the behavior as consistent as possible. I see this is related to an issue that worked in v2 but not in v3. Can you confirm this is still an issue in the latest versions of Apollo (v3.7.1 at the time of this writing)?
Feel free to close or amend this PR as need be. Thanks!
🦋 Changeset detectedLatest commit: e394739 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -218,10 +218,6 @@ describe('Basic resolver capabilities', () => { | |||
| const serverQuery = gql` | |||
| fragment Foo on Foo { | |||
| bar | |||
| ...Foo2 | |||
There was a problem hiding this comment.
This test was added recently and has a __typename-only fragment which would have been previously removed from the document. It can simply be omitted here.
Consider the following schema on the server:
```graphql
interface Character {
appearsIn: [Episode]!
}
type Human implements Character {
appearsIn: [Episode]!
height: Int
}
```
Then the following addition on the client:
```graphql
type Droid implements Character {
appearsIn: [Episode]!
primaryFunction: String
}
```
With the following query:
```graphql
query HeroForEpisode($ep: Episode!) {
hero(episode: $ep) {
... on Human {
height
}
... on Droid @client {
primaryFunction
}
}
}
```
The server will complain that the fragment spread has no type overlap
with parent, as the server does not have knowledge of the `Droid` type.
This commit strips out inline fragments which use a client directive,
allowing them to be resolved via local state.
|
Doing some tidying before merging this: I cherry-picked the commit from #7488 since the PRs are somewhat related (and both needed minor updates based on when they were opened), and just removed the The only outstanding item here is I'd like to move the second call to |
|
After discussing with @benjamn, I've done some light refactoring here (there are more improvements we can make in the future, but for now I was able to remove one call to This PR is ready for review 😄 |
alessbell
changed the title
alessbell
added
🐞 bug
🥚 backwards-compatible
|
@benjamn after our last chat about this I added some tests where the fragment appears before the query, and the one failing test is for I've kept the test with a FIXME comment pointing to #10539 and |
|
Closing in favor of #10560. |
Fixes #6311 and incorporates changes from #7488.