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

feat(gatsby): Allow printing type definitions to file (schema lock-down) #16291

Merged
merged 6 commits into from
Aug 9, 2019

Conversation

stefanprobst
Copy link
Contributor

@stefanprobst stefanprobst commented Aug 1, 2019

While the Schema Customization APIs allow defining types with the createTypes action and thus "lock down" a schema, it is cumbersome to do this manually for a lot of typedefs. It would be cool if we could save the current schema (defined+inferred types) to file, and then feed this back into Gatsby. This is what this PR enables.

We don't save the whole schema, but try to include as little as necessary, i.e. derived types for filtering, sorting etc. are not included, neither are fieldtypes added in setFieldsOnGraphQLNodeType or anything else that can be recreated from the provided typedefs.

By default, it does however include field types, inlined types, and interfaces. Configuration what to include/exclude is possible by typenames and by plugins. Types owned by internal-data-bridge, as well as built-in scalars, Internal and Node are excluded.

Every type implementing the Node interface automatically gets the dontInfer directive, so inference can be skipped when feeding the typedefs back in.

Fixes: #3344

Example:

exports.createSchemaCustomization = ({ actions }) => {
  // by default saves to `schema.gql`
  actions.printTypeDefinitions({
    path: `typedefs.graphql`,
    exclude: { types: [`TypeWeDontWant`] }
  })
}

To recreate the schema, just feed it into createTypes:

exports.createSchemaCustomization = ({ actions }) => {
  actions.createTypes(fs.readFileSync(`schema.gql`, { encoding: `utf-8` }))
}

The whole thing turned out to be more complicated than expected, because graphql-js's schema printer does not include directives. So this includes a modified version.

Could use some cleanup, but posting this now to hopefully get some real-world testing and feedback. Cleanup especially wrt to the schema print stuff -- importing from graphql-js internals is no good - this has actually changed from v14.1=>v14.2

@stefanprobst stefanprobst requested a review from a team as a code owner August 1, 2019 14:27
Copy link
Contributor

@freiksenet freiksenet left a comment

Choose a reason for hiding this comment

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

This is great!

What is the lifecycle to lock the schema and then read it if it's available? At this moment it looks quite complicated, should there be a cli command or a plugin that handles that?

@stefanprobst
Copy link
Contributor Author

You would do it like this:

const fs = require(`fs`)

const SAVED_SCHEMA = `my-typedefs.gql`

exports.createSchemaCustomization = ({ actions }) => {
  if (fs.existsSync(SAVED_SCHEMA)) {
    actions.createTypes(fs.readFileSync(SAVED_SCHEMA, { encoding: `utf-8` }))
  } else {
    actions.printTypeDefinitions({
      path: SAVED_SCHEMA,
      exclude: { types: [`TypeWeDontWant`] },
    })
  }
}

I can see this put into a plugin that takes the file path, and include/exclude from pluginOptions

@freiksenet
Copy link
Contributor

@stefanprobst We probably need a separate action for loading schema like this from file. Imagine a situtation where you use such plugin, but then there is also schemaCustomization in user code that defines types. That would mean that types will be defined twice. So schema loading action should know how to check against conflicts like that.

In addition, all the types defined in this "schema-loading" plugin will have ownership set by this plugin. We should have a directive (that probably should only work in this action, not in createTypes) that lets one set up the plugin metadata for a type.

@stefanprobst
Copy link
Contributor Author

stefanprobst commented Aug 6, 2019

  • a user's gatsby-node.js can always use createTypes, and if a type already exists (i.e. has been loaded from file) it will be merged in, so this should work.
  • plugin ownership is a good point. not sure how consequential it is since we only use the plugin metadata to check if the plugin is allowed to modify a type. need to check the behavior here
  • maybe a CLI flag would indeed be better option than a plugin (though passing all available config options could get complicated)
  • also, it would be great to get some feedback from the people in [Discussion] Feature: GraphQL schema snapshots for all data sources to solve undefined/empty data issues #3344 who have been asking for this for a while
@freiksenet
Copy link
Contributor

plugin ownership is a good point. not sure how consequential it is since we only use the plugin metadata to check if the plugin is allowed to modify a type. need to check the behavior here

It might get more relevant in future, so let's make sure it works correctly.

a user's gatsby-node.js can always use createTypes, and if a type already exists (i.e. has been loaded from file) it will be merged in, so this should work.

Will it merge or replace that?

maybe a CLI flag would indeed be better option than a plugin (though passing all available config options could get complicated)

I actually feel your initial intuition with plugin and action is a better long term solution. We can call actions in CLI too, after all.

@stefanprobst
Copy link
Contributor Author

It might get more relevant in future, so let's make sure it works correctly.

If the type definitions are loaded from file and added in createSchemaCustomization they technically are now owned by the user, not the plugin. This is also how we do it in master currently: if a user modifies a type, it is then owned by the user. So I think this is actually in line with master.

Will it merge or replace that?

It will be merged.

I actually feel your initial intuition with plugin and action is a better long term solution. We can call actions in CLI too, after all.

I'm not yet very clear on what the added value of a plugin should be over the short snippet above. What do you think?

@freiksenet
Copy link
Contributor

I'm not yet very clear on what the added value of a plugin should be over the short snippet above. What do you think?

It just feels like a nice reusable way use that. Add "gatsby-freeze-schema" plugin, have a schema that won't get changed. However we don't need to add it now.

If the type definitions are loaded from file and added in createSchemaCustomization they technically are now owned by the user, not the plugin. This is also how we do it in master currently: if a user modifies a type, it is then owned by the user. So I think this is actually in line with master.

OK, now I see it. You are correct.

@freiksenet freiksenet merged commit 23a460a into gatsbyjs:master Aug 9, 2019
@stefanprobst stefanprobst deleted the dump-schema branch August 9, 2019 07:23
@stefanprobst
Copy link
Contributor Author

Thanks for merging!

It just feels like a nice reusable way use that. Add "gatsby-freeze-schema" plugin, have a schema that won't get changed. However we don't need to add it now.

I'll work on that next. I guess one other advantage of a plugin is that we get a full README to document all config options instead of hiding somewhere in the overlong schema customization docs :)

@patrick91
Copy link

@stefanprobst will this also be a cli command to print the schema to a file?
Might be useful to replace this: https://www.gatsbyjs.org/packages/gatsby-plugin-extract-schema/

@stefanprobst
Copy link
Contributor Author

@patrick91 first step is to add this as a plugin (#16561), if it's useful we could also add a cli command.

wrt gatsby-plugin-extract-schema: i'm not super familiar with the plugin, but they might be serving different purposes -- this PR added functionality to save as minimal a schema as necessary to recreate all types, while the mentioned plugin saves the full built schema (minus the directives)? @NickyMeuleman i guess you have more context on the plugin usecase?

@NickyMeuleman
Copy link
Contributor

NickyMeuleman commented Aug 12, 2019

The main reason for creating that plugin was: I wanted squiggly lines in my editor when I made a typo in a query in my .js files. (ideally, also autocomplete, but that didn't happen)
It made using things like using eslint-plugin-graphql possible.
example:
https://twitter.com/NMeuleman/status/1015273538046038018/photo/1

Since publishing that plugin, a version that does that got integrated into Gatsby iirc.
If you make the same typo as in that picture, Gatsby will warn you in the CLI output.

From what I read here, the created gatsby-plugin-schema-snapshot can accomplish the same goal.
edit: Maybe not completely, since it doesn't include type definitions from plugin created types. That probably means using that linting plugin won't catch typos in something like childImageSharp { fluid { //things } }

PS: I haven't been keeping up with it and though I was (almost) the sole user of that plugin, this made my day 😄

@patrick91
Copy link

Similar to @NickyMeuleman, I'm using graphql-codegen to generate types from the schema, and I want to do that on CI too :)

@m4rrc0
Copy link
Contributor

m4rrc0 commented Sep 15, 2019

Hey. Quick reply to share my use case.
The only way I intend to use this is to generate the schema once when my CMS data structure is finalized before handing the project to editors. Then update it once in a while when the structure changes.
To save the schema file, a plugin is probably not the best fit for this because I will just create the schema once locally then comment out the appropriate lines for a long time. A CLI seems more appropriate but to be honest I really don't care. If a plugin it is, that is perfectly fine. I can always use some environment variable to change the build config if I need to.
Now when we want to use the schema we saved, a plugin is surely a good fit. This eases the use of schema customization.
To me (as I mentioned a while ago in #3344 ) the saving and using should be 2 different tools because we won't often use both at the same time...
Anyway, this is just details of implementation but I can probably make it work for my use case anyway.
Have I said this is awesome to have this? ;D
Thanks a lot @stefanprobst

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants