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

Stricter type inference and enforcement? #16310

Closed
deklanw opened this issue Aug 1, 2019 · 18 comments
Closed

Stricter type inference and enforcement? #16310

deklanw opened this issue Aug 1, 2019 · 18 comments
Labels
topic: GraphQL Related to Gatsby's GraphQL layer

Comments

@deklanw
Copy link

deklanw commented Aug 1, 2019

Is there a way to assume that every field seen in a frontmatter is non-nullable? It seems like that would be a reasonable option to have.

I solved it like this,

exports.createSchemaCustomization = ({ actions }) => {
   const { createTypes } = actions;
   const typeDefs = `
    type Frontmatter {
      title: String!
      date(formatString: String fromNow: Boolean difference: String locale: String): Date!
      description: String!
      tags: [String!]!
    }

    type MarkdownRemark implements Node {
      html: String!
      frontmatter: Frontmatter!
    }
  `;
   createTypes(typeDefs);
};

Which is fine.

But, it all works except for the html field which still ends up being nullable. Why? And, I'm not even sure why that is nullable in the first place. In what situation would the html be null?

Also, will this schema be enforced in any way? If one of my blog posts, for example, are missing a date field will the CLI yell at me? I'm using this with graphql codegen for convenience, but that would be nice too.

Thanks

@gatsbot gatsbot bot added the type: question or discussion Issue discussing or asking a question about Gatsby label Aug 1, 2019
@stefanprobst
Copy link
Contributor

stefanprobst commented Aug 1, 2019

@deklanw

  • Gatsby v2.2 has introduced new schema-related APIs like the createTypes action, but many plugins are still relying on the setFieldsOnGraphQLNodeType API, which - while not officially deprecated - will just overwrite user-provided field-types. see here for the html field
  • slightly unrelated: you can put the dateformat directive on the Frontmatter.date field so you don't have to manually state all those input args: date: Date! @dateformat

@deklanw
Copy link
Author

deklanw commented Aug 2, 2019

@stefanprobst

That explains the html thing. Thanks for the date tip. I'm still wondering why html is nullable to begin with, and whether these types get enforced via CLI complaining at me.

@stefanprobst
Copy link
Contributor

yes would probably make sense to make it not nullable.
if you want you can enforce this yourself with the createResolvers API, which is run as the last step in schema generation (i.e. after setFieldsOnGraphQLNodeType):

// gatsby-node.js
exports.createResolvers = ({ createResolvers }) => {
  createResolvers({
    MarkdownRemark: {
      html: {
        type: 'String!',
      }
    }
  })
}

GraphQL will complain when there are null values on GraphQLNonNull fields. You can check the error you'll see with something like this:

const { graphql, buildSchema } = require('graphql')
graphql(
  buildSchema('type Query { html: String! }'),
  'query { html }'
).then(result => console.error(result.errors))

@RIP21
Copy link
Contributor

RIP21 commented Aug 4, 2019

Hi everyone. I'm with the author, many fields like timeToRead and html and mostly all others are optional and it drives me nuts a little :)

Why? Because I'm full-blown on TypeScript and https://graphql-code-generator.com/ and it's indeed much easier to navigate all those deep trees of nodes, edges, JSON files, and other things when it's strongly typed, also, it's nice to be sure that everything is nicely typed and "there" and of a correct type based on typings generated from the schema, queries, fragments etc.

Basically, the fact that 99% of the fields in remark types are optional whether they are not, makes it hell on earth for 100% TS typed Gatsby sites with LOTS of null checks. Also, types like [String] is even worse, not only an array part needs null check but also all elements inside while in real life it almost never they will be nullable.

So, I will be super happy if the types that plugins are generating will become stricter.

@deklanw
Copy link
Author

deklanw commented Aug 5, 2019

@RIP21
Yes, that is my motivation for this post also.

@RIP21
Copy link
Contributor

RIP21 commented Aug 5, 2019

@stefanprobst Can we get this tagged properly maybe? So it will be in backlog or so? Because it's kinda painful :)

@gatsbot gatsbot bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Aug 26, 2019
@RIP21
Copy link
Contributor

RIP21 commented Aug 27, 2019

Not stale, still pain.

@gatsbot gatsbot bot closed this as completed Sep 7, 2019
@RIP21
Copy link
Contributor

RIP21 commented Sep 7, 2019

This is still a problem.

@RIP21 RIP21 reopened this Sep 7, 2019
@sidharthachatterjee sidharthachatterjee added not stale and removed stale? Issue that may be closed soon due to the original author not responding any more. labels Sep 8, 2019
@MorrisonCole
Copy link
Contributor

New to Gatsby and have been running into this too - it's definitely painful for TS sites!

@gatsbyjs gatsbyjs deleted a comment from gatsbot bot Sep 10, 2019
@gatsbyjs gatsbyjs deleted a comment from gatsbot bot Sep 10, 2019
@LekoArts LekoArts added type: feature or enhancement topic: GraphQL Related to Gatsby's GraphQL layer and removed not stale type: question or discussion Issue discussing or asking a question about Gatsby labels Nov 6, 2019
@github-actions
Copy link

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Nov 27, 2019
@github-actions
Copy link

github-actions bot commented Dec 7, 2019

Hey again!

It’s been 30 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it.
Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to reopen this issue or create a new one if you need anything else.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks again for being part of the Gatsby community! 💪💜

@github-actions github-actions bot closed this as completed Dec 7, 2019
@RIP21
Copy link
Contributor

RIP21 commented Dec 7, 2019

It's not stale. Thanks.

@RIP21 RIP21 reopened this Dec 7, 2019
@github-actions github-actions bot removed the stale? Issue that may be closed soon due to the original author not responding any more. label Dec 8, 2019
@wardpeet
Copy link
Contributor

We're moving official plugins to use the schema customization API like @stefanprobst mentioned. For now, you can do it yourself in gatsby-node.js. It's still going to be impossible to do this correctly 100% of the time. We could use inference and if a property is available everywhere mark it as non nullable.

cc @vladar

@vladar
Copy link
Contributor

vladar commented Dec 16, 2019

Yes, you can follow #20069 for schema customization updates. We will try to convert as much as possible to non-null as a part of this process.

@github-actions
Copy link

github-actions bot commented Jan 6, 2020

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Jan 6, 2020
@m-allanson m-allanson removed the stale? Issue that may be closed soon due to the original author not responding any more. label Jan 6, 2020
@github-actions
Copy link

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Jan 26, 2020
@RIP21
Copy link
Contributor

RIP21 commented Jan 26, 2020

Not stale, dear bot.

@github-actions github-actions bot removed the stale? Issue that may be closed soon due to the original author not responding any more. label Jan 27, 2020
@LekoArts
Copy link
Contributor

I'll close this in favor of #20069 as the original question was answered and nothing actionable is present. We'd be happy to receive PRs to update the plugins to use the stricter variant :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: GraphQL Related to Gatsby's GraphQL layer
Projects
None yet
Development

No branches or pull requests

10 participants