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

3.13 Directive validation edits #1089

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jbellenger
Copy link

@jbellenger jbellenger commented Apr 2, 2024

The Validation section for Directive types is a little out of step with the validation sections for the other types. In particular, while the grammar definition of Directive requires one or more directive locations, this requirement isn't mentioned in the validation section.

Compare this to the validation sections for other types with "one or more" requirements:

  • 1. An Object type must define one or more fields.
  • 1. A Union type must include one or more member types.
  • 1. An Enum type must define one or more unique enum values.
  • 1. An Input Object type must define one or more input fields.
  • 1. An Interface type must define one or more fields.

This PR updates Directive validation to be more consistent with the validation sections of other types:

  1. Add a "one or more" requirement as the first validation rule
  2. Use "Directive" instead of "directive"
  3. Rename the section from "Validation" to "Type Validation"

While this PR just clarifies the existing spec, I suspect it might be common for implementations to not check for non-empty directive locations.
I've updated graphql-java and can do the same for graphql-js if I get a tentative OK on this PR.

- add requirement that DirectiveLocations is non empty
- Validation -> TypeValidation
- s/directive/Directive in most places
Copy link

linux-foundation-easycla bot commented Apr 2, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: benjie / name: Benjie (c8ca2c1)
  • ✅ login: jbellenger / name: James Bellenger (71a76ed, cb28e5e)

Copy link

netlify bot commented Apr 2, 2024

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit c8ca2c1
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/6643359ba7e18800088d3a03
😎 Deploy Preview https://deploy-preview-1089--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.


1. A directive definition must not contain the use of a directive which
1. A Directive definition must include at least one DirectiveLocation.
Copy link
Author

@jbellenger jbellenger Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this worded differently from the other "one or more" constraints?
If this were worded in the same style as Objects, it would read "A Directive must define one or more DirectiveLocations"

To be pedantic, while this sentence would attempt to use a plural form of the DirectiveLocation token, it could also be interpreted as a plural form of the DirectiveLocations token, which is given a distinct definition in Appendix B.4 and does not match the Directive grammar.

I thought that a phrasing that avoided pluralizing DirectiveLocation would be less ambiguous, though recognize there's also value in using consistent language. I'm open to feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're also generally okay with must include one or more DirectiveLocation.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 This seems like an editorial change and so does not need to go through the RFC process.

@graphql/tsc Can we get another +1?

spec/Section 3 -- Type System.md Outdated Show resolved Hide resolved
@benjie benjie added the ✏️ Editorial PR is non-normative or does not influence implementation label May 14, 2024
@benjie
Copy link
Member

benjie commented May 14, 2024

I've graphql-java/graphql-java#3552 and can do the same for graphql-js if I get a tentative OK on this PR.

@jbellenger Please go ahead and make the changes to GraphQL.js 🙌

@dondonz
Copy link
Contributor

dondonz commented May 14, 2024

Nice! Great to see the spec clarified

@benjie
Copy link
Member

benjie commented Jul 1, 2024

GraphQL.js backport into v16 open here: graphql/graphql-js#4137

@benjie
Copy link
Member

benjie commented Jul 15, 2024

Scheduled for discussion in Thursday's WG: https://github.com/graphql/graphql-wg/blob/main/agendas/2024/07-Jul/18-wg-primary.md

@benjie
Copy link
Member

benjie commented Jul 19, 2024

cc @graphql/tsc: TL;DR: merging this in one week unless anyone raises concerns.

This was discussed at last nights WG (replay; agenda; notes) and it was agreed this is an editorial fix and good to merge. It already has two TSC approvals but I'll leave it open another week or so and then go ahead and merge it. Essentially, we see this as a bugfix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editorial PR is non-normative or does not influence implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants