Skip to content

Enable 'schema' keyword to be provided without root operations#1166

Closed
benjie wants to merge 1 commit into
mainfrom
allow-naked-schema-main
Closed

Enable 'schema' keyword to be provided without root operations#1166
benjie wants to merge 1 commit into
mainfrom
allow-naked-schema-main

Conversation

@benjie

@benjie benjie commented May 1, 2025

Copy link
Copy Markdown
Member

This PR is motivated by:

In the above PR we want to be able to indicate the error propagation behavior of a schema. However, I find it displeasing that to do so we would need to also explicitly detail the operations supported by the schema when they would normally be auto-detected based on their names:

+schema @behavior(onError: NO_PROPAGATE) {
+  query: Query
+  mutation: Mutation
+  subscription: Subscription
+}
+
 type Query {
   q: Int
 }

 type Mutation {
   m: Int
 }

 type Subscription {
   s: Int
 }

This PR makes it so that you can use a schema keyword to add directives and/or description to the schema without needing to also specify the root operation types if they can be inferred by the default naming. The above diff would thus become:

+schema @behavior(onError: NO_PROPAGATE)
+
 type Query {
   q: Int
 }

 type Mutation {
   m: Int
 }

 type Subscription {
   s: Int
 }

Much nicer! ✨

This is also useful without the onError feature, since it allows you to apply a description to a schema without having to detail the operations:

+"""Just an example"""
+schema
+
 type Query {
   q: Int
 }

 type Mutation {
   m: Int
 }

 type Subscription {
   s: Int
 }

PR #1164 applies these changes onto #1163; but this PR is mergeable into the spec as-is.

@netlify

netlify Bot commented May 1, 2025

Copy link
Copy Markdown

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 470358c
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/681344fa07b2300008ab1b36
😎 Deploy Preview https://deploy-preview-1166--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.

@martinbonnin martinbonnin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚢

@fotoetienne

Copy link
Copy Markdown
Contributor

What about instead allowing the block to be empty?

i.e.:

schema @behavior(onError: NO_PROPAGATE) {}

This is perhaps less likely to break existing parsers

@benjie

benjie commented May 1, 2025

Copy link
Copy Markdown
Member Author

This is perhaps less likely to break existing parsers.

It's not.

import { parse } from 'graphql';

const ast = parse(`schema {}`);
console.dir(ast);
/home/benjie/Dev/graphql/graphql-js/npmDist/error/syntaxError.js:13
  return new _GraphQLError.GraphQLError(`Syntax Error: ${description}`, {
         ^

GraphQLError: Syntax Error: Expected Name, found "}".
    at syntaxError (/home/benjie/Dev/graphql/graphql-js/npmDist/error/syntaxError.js:13:10)
    at Parser.expectToken (/home/benjie/Dev/graphql/graphql-js/npmDist/language/parser.js:1337:40)
    at Parser.parseOperationType (/home/benjie/Dev/graphql/graphql-js/npmDist/language/parser.js:246:33)
    at Parser.parseOperationTypeDefinition (/home/benjie/Dev/graphql/graphql-js/npmDist/language/parser.js:724:28)
    at Parser.many (/home/benjie/Dev/graphql/graphql-js/npmDist/language/parser.js:1440:26)
    at Parser.parseSchemaDefinition (/home/benjie/Dev/graphql/graphql-js/npmDist/language/parser.js:706:33)
    at Parser.parseDefinition (/home/benjie/Dev/graphql/graphql-js/npmDist/language/parser.js:171:23)
    at Parser.many (/home/benjie/Dev/graphql/graphql-js/npmDist/language/parser.js:1440:26)
    at Parser.parseDocument (/home/benjie/Dev/graphql/graphql-js/npmDist/language/parser.js:127:25)
    at parse (/home/benjie/Dev/graphql/graphql-js/npmDist/language/parser.js:28:27) {
  path: undefined,
  locations: [ { line: 1, column: 9 } ],
  extensions: [Object: null prototype] {}
}

Node.js v22.14.0

What about instead allowing the block to be empty?

To me, an empty block would explicitly say there are no operation types, whereas a missing block says to use the defaults.

@benjie

benjie commented May 19, 2025

Copy link
Copy Markdown
Member Author

This was discussed at the WG at the beginning of this month, Lee had some pushback:

https://youtu.be/Lo0OhLoMBII?t=2754

If you have a strong use case for this, please do comment.

@benjie

benjie commented May 19, 2025

Copy link
Copy Markdown
Member Author

Extracted the bugfix part to #1167

@benjie

benjie commented May 22, 2025

Copy link
Copy Markdown
Member Author

I think I'm generally in agreement with Lee's pushback that:

"""
This is an example schema
"""
schema

type Query {
  a: Int
}

is less clear than

"""
This is an example schema
"""
schema {
  query: Query
}

type Query {
  a: Int
}

And since a) I have a lot of open PRs, and b) we don't need this for disabling error propagation (because we've moved that to "service" instead of "schema") I'm going to close it temporarily. I think if schema directives become common in future I would like to revisit it because I prefer:

+schema @newBehavior
 
 type Query {
   a: Int
 }

over

+schema @newBehavior {
+  query: Query
+}
 
 type Query {
   a: Int
 }

But we'll cross that bridge when we get to it.

@benjie benjie closed this May 22, 2025
@benjie benjie added 🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md) and removed 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) labels May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants