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

Allow fields with arguments in @requires #2120

Merged
merged 3 commits into from
Sep 13, 2022

Conversation

pcmanus
Copy link
Contributor

@pcmanus pcmanus commented Aug 31, 2022

This PR allow the fields argument of a @requires to require fields that are defined with arguments. If some of those argument are required, then those argument needs to be passed a value in the @requires, while non-required argument can be passed a value but don't have to (and if no value is provided, they will be queried with their default value for the @requires, exactly the same way that they would if they were queried directly).

I'll note that this PR maintains the rejection of fields with arguments in @key and @provides: we can lift the restriction for those directives later in separate PRs. In particular, that may be subjective, but allowing passing argument in @requires feels a bit easier to reason about/understand to me than with @key and @provides, so it may be worth taking a bit more time to think about the implications of the later ones.

For @requires, the code was mostly working out of the box, so the first commit mainly just 1) remove the validation that was rejecting it and 2) adds test coverage. However, adding it makes it necessary to re-validate the @requires post-merge to prevent a few corner cases from silently merging but failing later, and that added validation is part of that first commit too (with ample comments).

The 2nd commit of this PR is new validation to ensure that we reject the use of aliases in the fields argument of @requires, @provides and @key, and this is because this change made me realised that alias were not rejected but were not working properly (they would silently make a @provides not work for instance if used). This validation is really more of an oversight and make sense regardless of the rest of this PR, but I think it make sense to include it here because "fields with arguments" is probably the main reason why someone might to use aliases in the first place, and so this ticket makes it more important to be clear that those are not supported. To be clear, we could later add proper support for aliases, but this is probably somewhat involved and, afaict, has never be requested by any user, so not too urgent.

@netlify
Copy link

netlify bot commented Aug 31, 2022

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit ee44520

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 31, 2022

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.

Sylvain Lebresne added 3 commits September 13, 2022 11:58
If such field with arguments is used in a @requires, then that @requires
must provide a value for any required argument and may provide a value
for any non-required one (like in any normal graphQL selection set).

Fixes apollographql#1987
Not because aliases cannot ever make sense in those directives, but
rather because it currently does not work (it breaks at runtime) but
wasn't rejected properly.
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.

None yet

2 participants