Fix repeatable custom directives - #2136
Conversation
Getting a false positive hint when a repeatable custom directive is composed saying that it's not repeatable. This happens because we addDirectivesShallow, but the directive isn't fully created when this check is run. Solved by switching the merge order between building directives and types.
✅ Deploy Preview for apollo-federation-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
No issues with the tests, but I'd like to run this against the federation-harness to make sure there are no issues before merging. |
pcmanus
left a comment
pcmanus
left a comment
There was a problem hiding this comment.
I don't think that this quite solve the problem in all cases.
The underlying issue here is that mergeAppliedDirective relies on directive definition having been merged, so we need to make sure it's never called before mergeDirectiveDefinition.
Moving the merging of type definitions after that of directive definitions does fixes it for directive applications "within type definitions", but that's not the only place where directive applications can appear.
In particular, we can have directive applications both in "schema definition" and on the arguments or directive definition themselves. For the former, moving the merging of directive definitions one more step up (before the call the mergeSchemaDefinition) would probably fix it, but the later problem is a bit more tricky.
So my suggestion for solving this would be to split the merging of directive applications separately (for all elements: schema definition, types and directive definitions), and have that done last during merging. It's a bit more code change (though hardly a big change), but it feels like this is easier to reason about and not have break later.
As an aside, I'll note that:
- the added test does not pass because the directive in the test is not actually marked repeatable (I assume due to copy-pasta from the github issue).
- there is a change to (the auto-generated)
hints.mdrelated to fixing the typo toINCONSISTEN_NON_REPEATABLE_DIRECTIVE_ARGUMENTS(missingT) but the corresponding code change is not in the commit.
Nit: may I suggest taking the habit of checking CI before marking issues ready for review?
Sorry about that, for some reason I pushed without saving the files. |
| import { CompositionResult, composeServices } from '../compose'; | ||
| import gql from 'graphql-tag'; | ||
| import './matchers'; | ||
| import { print } from 'graphql'; |
There was a problem hiding this comment.
Note to reviewers: I noticed that compose.test.ts tests were getting run multiple times because the helper functions were being imported from other files, so I split those out into a separate file.
| // and this test will have to updated. | ||
| expect(secondCall[0]!.compositionId).toEqual( | ||
| 'cc95112b64179c4e549de788b051f44010a02877f568649a42caeeee6a135601', | ||
| 'a7e83d2e958dc8e0f08326443113802c900a4d977a26df6cfda599f138d9bb2f', |
There was a problem hiding this comment.
Note to reviewers: This is just a reordering of applied directives in the SDL (see the diff in the .snap file for a similar case).
We sometimes have to call the method merging directives on a merged value that we end up removing later, and this because we need @inaccessible to be merged to decide if the value should be kept (this is case of enum value, but I think input field are also the same). But since apollographql#2136, most directives (@inaccessible being the one exception) are not merged right away, but we simply "record" that they should be merged and merge them later. The result however is cases where when we later try the merging, the element is not attached to the schema anymore, and an assertion was triggered. This fixes this cases.
We sometimes have to call the method merging directives on a merged value that we end up removing later, and this because we need @inaccessible to be merged to decide if the value should be kept (this is case of enum value, but I think input field are also the same). But since #2136, most directives (@inaccessible being the one exception) are not merged right away, but we simply "record" that they should be merged and merge them later. The result however is cases where when we later try the merging, the element is not attached to the schema anymore, and an assertion was triggered. This fixes this cases.
2 participants
Getting a false positive hint when a repeatable custom directive is composed saying that it's not repeatable. This happens because we addDirectivesShallow, but the directive isn't fully created when this check is run. Solved by switching the merge order between building directives and types.
Fixes #2134