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

Change 'original' to 'previous' to clarify multiple extensions #1123

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Oct 31, 2024

@benhead described this well in #1116

Essentially the issue boils down to:

1  union MyUnion = Foo
2  extend union MyUnion = Bar | Baz
3  extend union MyUnion = Bar | Qux
  • On line 1 we define a union.
  • On line 2 we extend the "original" union (defined on line 1).
    • The rule "All member types of a Union type extension must not already be a member of the original Union type." passes, since neither Bar nor Baz exist in the original union (Foo)
  • On line 3 we extend the "original" union. AMBIGUITY: is this "original" the true original, defined on line 1, or is it the previous version of the union which is the combination of line 1 and line 2?
    • If we go with the "true original" interpretation, this type extension passes the rule since neither Bar nor Qux are referenced in the union on line 1.
    • If we go with the "previous" interpretation, this type extensions fails the rule (as we would expect?) because Bar is already included in the composite union Foo | Bar | Baz.

I believe we actually mean the previous (i.e. combined) version. This is certainly what the reference implementation seems to do:

import { buildSchema, printSchema, validateSchema } from 'graphql';

const schema = buildSchema(/* GraphQL */ `
  union MyUnion = Foo
  extend union MyUnion = Bar | Baz
  extend union MyUnion = Bar | Qux

  type Query { u: MyUnion }
  type Foo { foo: Int }
  type Bar { bar: Int }
  type Baz { baz: Int }
  type Qux { qux: Int }
`);

const errors = validateSchema(schema);
if (errors.length > 0) {
  console.dir(errors);
  process.exit(1);
}

console.dir(printSchema(schema));

yields:

GraphQLError: Union type MyUnion can only include type Bar once.

Aside: I actually think the original intention was to extend a type once in a derived schema as implied by the Type System Extensions opening paragraph; however that ship has long sailed and multiple extensions are common - in fact extending types defined in the same schema (as shown in the document below) is common practice.

I attempted to add a non-normative note to clarify that "original here means the previous version, which may itself be an extended type..." or something but couldn't get happy with the wording. Then I realised that simply changing original to previous would convey the intent well, and by sheer coincidence they happen to have the same number of characters!

There is one caveat to this solution, and that is that original type definitions are "hoisted", so though the order of type extensions in the document is significant, the original type (actual original!) can occur anywhere, so the use of "previous" is arguably dubious in this case (though I'd argue that named type definitions always precede type extensions in order of processing, so this is still fine):

extend union MyUnion = Bar | Baz
extend union MyUnion = Qux

type Query { u: MyUnion }
type Foo { foo: Int }
type Bar { bar: Int }
type Baz { baz: Int }
type Qux { qux: Int }

union MyUnion = Foo

Also, I can no longer read the word union without seeing it as onion, so that's a fun malapropism that's going to haunt me now 🙄

@benjie benjie added the ✏️ Editorial PR is non-normative or does not influence implementation label Oct 31, 2024
Copy link

netlify bot commented Oct 31, 2024

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 3a52ce2
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/67235948bbf2db00089795f8
😎 Deploy Preview https://deploy-preview-1123--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.

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.

1 participant