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

Add test for multiple where directives #834

Merged
merged 13 commits into from
Jul 20, 2020
Merged

Conversation

cwhitby
Copy link
Contributor

@cwhitby cwhitby commented Jun 16, 2019

  • Added or updated tests
  • Added Docs for all relevant versions
  • Updated the changelog

Related Issue/Intent

References #831

Changes

No code changes, only adding a test for applying multiple @where directives on the same field

Breaking changes

None

@spawnia
Copy link
Collaborator

spawnia commented Jun 17, 2019

@cwhitby I realized this problem is twofold. While i adapted the handling of BuilderDirective's to allow for multiple builders, the BaseDirective does not take into account that there may be more than one directive of the same name.

Thanks for the PR, i am working on a generalized fix.

@spawnia
Copy link
Collaborator

spawnia commented Jun 19, 2019

Actually, the GraphQL specification declares that directives must be unique per location. There is an ongoing RFC to allow repeatable directives: graphql/graphql-spec#472

@spawnia spawnia added the 5.x Related to the 5.x release series label May 27, 2020
@Jofairden
Copy link

@spawnia you should review this again, as afaik repeatable directives are now officially supported by the spec: https://github.com/graphql/graphql-wg/blob/master/notes/2019-10-10.md#repeatable-directives-ivan-10m

@spawnia
Copy link
Collaborator

spawnia commented Jul 14, 2020

@Jofairden thanks, I am aware. Added support in graphql-php for it myself and plan to enable repeatable directives in Lighthouse v5.

@spawnia
Copy link
Collaborator

spawnia commented Jul 20, 2020

@cwhitby the test is working now, since we changed some logic around that makes it function correctly. Also tried your original test 9a70046 (#834) and got a count of 0, should be correct?

Merging this for now.

@spawnia spawnia merged commit 3ab416c into nuwave:master Jul 20, 2020
DDynamic pushed a commit to DDynamic/lighthouse that referenced this pull request Nov 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.x Related to the 5.x release series
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants