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

Fix ambiguity around when schema definition may be omitted #3839

Merged
merged 5 commits into from
Feb 9, 2023

Conversation

benjie
Copy link
Member

@benjie benjie commented Feb 3, 2023

This PR implements the tests and fixes necessary for Spec RFC #987.

The main changes

This PR is made of three main commits:

  1. Test printing a schema that has Query, Mutation and Virus types, but only supports query operations (via the Query type) and does not support mutation operations.
  2. Test parsing this same schema text, and assert that the schema does not have a mutation type.
  3. Fix the printing of the schema.

Unnecessary test?

The fourth commit addresses the following failing test that I didn't know what to do about:

it('builds a schema without the query type', () => {
const sdl = dedent`
type Query {
foo: String
}
`;
const schema = buildSchema(sdl);
const introspection = introspectionFromSchema(schema);
// @ts-expect-error
delete introspection.__schema.queryType;
const clientSchema = buildClientSchema(introspection);
expect(clientSchema.getQueryType()).to.equal(null);
expect(printSchema(clientSchema)).to.equal(sdl);
});

I don't understand the reasoning behind this test. It seems to me that the schema output from this should instead be:

schema {}

type Query {
  foo: String
}

since the schema has no operation types (but uses the default names); however this is unparseable (because {} is unparseable). Perhaps @IvanGoncharov or @leebyron can shed some light on the reasoning behind this test? I've worked around it by special casing it, but I'd prefer to remove the special case code and also remove the test case.

@benjie benjie added the spec RFC Implementation of a proposed change to the GraphQL specification label Feb 3, 2023
@netlify
Copy link

netlify bot commented Feb 3, 2023

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 8a266f3
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/63e450f54ed6810008a942cc
😎 Deploy Preview https://deploy-preview-3839--compassionate-pike-271cb3.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.

@github-actions
Copy link

github-actions bot commented Feb 3, 2023

Hi @benjie, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@benjie
Copy link
Member Author

benjie commented Feb 3, 2023

cc @graphql/graphql-js-reviewers

@benjie
Copy link
Member Author

benjie commented Feb 3, 2023

Going to close/reopen to see if Netlify retries 🤷‍♂️

@spawnia
Copy link
Member

spawnia commented Feb 4, 2023

Works nicely when ported to PHP: webonyx/graphql-php#1303.

@leebyron leebyron force-pushed the test-schema-definition-ambiguity branch from 0ce831a to 917f970 Compare February 9, 2023 01:47
@leebyron leebyron force-pushed the test-schema-definition-ambiguity branch from 917f970 to 8a266f3 Compare February 9, 2023 01:48
@leebyron leebyron merged commit f201681 into main Feb 9, 2023
@leebyron leebyron deleted the test-schema-definition-ambiguity branch February 9, 2023 01:51
@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number spec RFC Implementation of a proposed change to the GraphQL specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants