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

[Feature] Support lexical parser options #567

Merged
merged 12 commits into from
Feb 12, 2018

Conversation

coleturner
Copy link
Contributor

@coleturner coleturner commented Jan 6, 2018

This adds the ability to specify parseOptions through makeExecutableSchema into graphql.parse - unlocking finer control of lexical options when building a schema.

Motivation

I'm working with tooling where I would like to be able to parse an empty schema Query type. As of today, it's not possible with graphql-tools because types must have at least one field. This is possible with graphql currently in master, albeit as of now those options are legacy or experimental feature flags. There are other use cases, such as noLocation which can be used to optimize AST building. It's nice to have these options as they are now and possibly what may be added in the future.

Creating this pull request for consideration once the master branch and all current options are deployed to NPM.

What's changed

  • Added parseOptions to makeExecutableSchema parameters
  • Carried parseOptions from makeExecutableSchema to graphql/parse
  • Added tests for current lexical parser options.

Current options supported:

  • noLocation
  • allowLegacySDLEmptyFields (pending publish)
  • allowLegacySDLImplementsInterfaces (pending publish)
  • experimentalFragmentVariables (pending publish)

Example

makeExecutableSchema({
  typeDefs: `
    type RootQuery {
    }
    schema {
      query: RootQuery
    }
  `,
  resolvers: {
  },
  parseOptions: {
    allowLegacySDLEmptyFields: true
  },
});

Related

graphql/graphql-js#1171
graphql/graphql-js@c496fb7
graphql/graphql-js@ce0a4b9

Tasks

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

@freiksenet
Copy link
Contributor

Sorry for update spam, github was misbehaving with conflict resolution. This fails because graphql-js haven't released version with those changes yet. I'll merge it once it's released.

@coleturner
Copy link
Contributor Author

Thanks for reviewing, I'll keep an eye out for the next release of graphql. Cheers!

@stubailo
Copy link
Contributor

Has the upstream stuff been published yet?

@coleturner
Copy link
Contributor Author

Not yet, it's been over a month since last release of graphql-js

@coleturner
Copy link
Contributor Author

coleturner commented Jan 30, 2018

This adds the ability to specify `parseOptions` through makeExecutableSchema
into graphql.parse - unlocking finer control of lexical options when
building a schema.
@coleturner
Copy link
Contributor Author

GraphQL-JS @ 0.13.0 has released

I rebased my PR from master, updated the package versions, and ran tests again (passing locally ✅)

@freiksenet
Copy link
Contributor

@colepatrickturner I get typescript errors locally atm:

src/schemaGenerator.ts(241,7): error TS2365: Operator '===' cannot be applied to types '"OperationDefinition" | "FragmentDefinition" | "SchemaDefinition" | "ScalarTypeDefinition" | "Obj...' and '"TypeExtensionDefinition"'.

@freiksenet
Copy link
Contributor

Other stuff this PR needs:

  • Get new TS definitions that include parseOptions and update them in devDependencies
  • Update graphql version in peerDependencies and devDependencies
  • Add 0.13 to travis feature matrix
  • Disable new tests for 0.11 and 0.12 using feature matrix env var
  • Update tests in mergeSchema to use new inheritance syntax, put 0.12 definitions under flag.

@freiksenet
Copy link
Contributor

(Though we could possibly split the above into "support 0.13" PR)

@coleturner
Copy link
Contributor Author

Woops I forgot to push it yesterday. Will go through the checklist

@coleturner
Copy link
Contributor Author

coleturner commented Feb 9, 2018

With the versioning matrix, I wrote it such that npm run test will run all tests if no matrix version is set. When set the various options are only available if they are >= the matrix version.

@freiksenet
Copy link
Contributor

@colepatrickturner Thank you so much for handling this! Waiting for the TS to merge and I'll release.

…r/graphql-tools into colepatrickturner-ct/parser-options
@freiksenet
Copy link
Contributor

@colepatrickturner Could you enable me to push to your branch? I have fixes that make tests pass.

@coleturner
Copy link
Contributor Author

@freiksenet I have added you as a contributor to my fork

@freiksenet
Copy link
Contributor

Urgh, there is issue with graphql-subscriptions not supporting 0.13 yet, so non-v5 npm fails. Waiting for this to land. apollographql/graphql-subscriptions#135

@freiksenet
Copy link
Contributor

(╯°□°)╯︵ ┻━┻

Now it works.

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