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 spec text for KnownOperationType #947

Closed
wants to merge 2 commits into from

Conversation

yaacovCR
Copy link
Contributor

@netlify
Copy link

netlify bot commented May 20, 2022

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 12b3a1e
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/6287349871c9d50008520c8b
😎 Deploy Preview https://deploy-preview-947--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 settings.

@yaacovCR
Copy link
Contributor Author

See graphql-js PR: graphql/graphql-js#3601
Spec enhancement suggested by @benjaminjkraft

@benjaminjkraft
Copy link

Neat. Not sure what the style in the spec is -- would it make sense to refer to this from 3.3.1 which describes this more informally ("if [the mutation root type] is not provided, the service does not support mutations")?

@benjie
Copy link
Member

benjie commented May 23, 2022

The text as currently proposed will conflict with https://github.com/graphql/graphql-spec/pull/825/files#diff-607ee7e6b71821eecadde7d92451b978e8a75e23d596150950799dc5f8afa43e which is currently RFC2. Either #825 needs to be refactored to not use mutation in the examples, or this should use a separate example schema for this validation rule. May I suggest we follow the latter approach as it allows our example schema to grow over time.

@yaacovCR
Copy link
Contributor Author

Neat. Not sure what the style in the spec is -- would it make sense to refer to this from 3.3.1 which describes this more informally ("if [the mutation root type] is not provided, the service does not support mutations")?

There seem to be a few references to Validation Rules, namely:

https://github.com/graphql/graphql-spec/blame/main/spec/Section%202%20--%20Language.md#L1134-L1135

We could try that here for sure, in my opinion.

May I suggest something => perhaps it makes sense for you to "take over" this PR, considering you will be leading the discussion at the WG and this rule was your idea to begin with. I sometimes wish GitHub had a UI for transferring ownership of PRs => I think it would be relatively easy for you to reopen a new one, and -- if you do -- I will close this one in favor of yours.

@yaacovCR
Copy link
Contributor Author

The text as currently proposed will conflict with https://github.com/graphql/graphql-spec/pull/825/files#diff-607ee7e6b71821eecadde7d92451b978e8a75e23d596150950799dc5f8afa43e which is currently RFC2. Either #825 needs to be refactored to not use mutation in the examples, or this should use a separate example schema for this validation rule. May I suggest we follow the latter approach as it allows our example schema to grow over time.

If @benjaminjkraft takes this over (as I hope he will) he might want to adopt this suggestion, otherwise I will try to work on it. Good catch!

@benjaminjkraft
Copy link

Sure, I can take over the PR and add a separate schema.

@benjaminjkraft
Copy link

Replaced by #955.

@yaacovCR yaacovCR closed this Jun 1, 2022
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.

3 participants