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(gatsby): Make root plural fields non nullable #15321

Merged
merged 5 commits into from Aug 9, 2019

Conversation

@spawnia
Copy link
Contributor

commented Jul 2, 2019

Description

Change the setup of both the internal schema and the gatsby-source-graphql plugin to add root query fields as non-nullable.

This helps to reduce tedious null-checks when querying via useStaticQuery or other mechanisms. This became especially apparent when using TypeScript code generation:

image

Actually, the file query will never return null - and if it does, there is something wrong anyways, so throwing would be the right thing to do. I am forced to ignore the error or add type coercion like data!.placeholderImage.

With this change, TypeScript users will have a much better time:

image

@spawnia spawnia requested a review from gatsbyjs/core as a code owner Jul 2, 2019
@spawnia

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

Is this something you are generally interested in pursuing further? There are a bunch more places in the GraphQL schema where adding NonNull types would make type safety easier to achieve.

@@ -741,8 +741,10 @@ const addTypeToRootQuery = ({ schemaComposer, typeComposer }) => {
const queryName = _.camelCase(typeName)
const queryNamePlural = _.camelCase(`all ${typeName}`)
schemaComposer.Query.addFields({
[queryName]: typeComposer.getResolver(`findOne`),
[queryNamePlural]: typeComposer.getResolver(`findManyPaginated`),
[queryName]: new GraphQLNonNull(typeComposer.getResolver(`findOne`)),

This comment has been minimized.

Copy link
@pieh

pieh Jul 3, 2019

Contributor

This technically isn't guaranteed to always not be null

This comment has been minimized.

Copy link
@spawnia

spawnia Jul 3, 2019

Author Contributor

@pieh What would a scenario were that field returns null look like?

Can a page still be successfully built or does it indicate an error that fails the build?

This comment has been minimized.

Copy link
@pieh

pieh Jul 7, 2019

Contributor

It might not be reasonable, but you can do

someType(id: { eq: "id-of-node-that-doesnt-exist" }) {
  id
}

Which will return null because node can't be found.

Other scenarios are - with schema customizations you can define types even if there are no nodes for that type - so those will always be empty (even without any filters)

This comment has been minimized.

Copy link
@spawnia

spawnia Jul 7, 2019

Author Contributor

I do get the first scenario. Marking the field as NonNull could still be better, as it would uncover an error right away. My assumption is that you always require the data you want to find.

If there are fields where you try to find something dynamically and don't know about it's existence and handle the case of not finding something, a nullable field could make sense.

In the case of the allSomeType fields - i think those should always return at least an empty list, so [SomeType]! instead of [SomeType].

@pieh

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

I think this makes a lot of sense to do, just need to be careful where non-null decorator is applied

@spawnia

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

I am having a hard time figuring out why the tests are failing.

It seems like the queries to the schema are set up in a way such that the data is not actually returned from the registered resolver, which is why the tests are failing when trying to query such a field:

  ● Query schema › on children fields › handles Node interface children field

    expect(received).toBeUndefined()

    Received: [[GraphQLError: Cannot return null for non-nullable field Query.allFile.]]

      182 |         },
      183 |       }
    > 184 |       expect(results.errors).toBeUndefined()
          |                              ^
      185 |       expect(results.data).toEqual(expected)
      186 |     })
      187 | 

      at Object.toBeUndefined (packages/gatsby/src/schema/__tests__/queries.js:184:30)

How can we fix this?

Copy link

left a comment

Might require more changes

@@ -925,7 +925,7 @@ const addTypeToRootQuery = ({ schemaComposer, typeComposer }) => {
},
resolve: findManyPaginated(typeName),
},
})
}).makeFieldNonNull([queryName, queryNamePlural])

This comment has been minimized.

Copy link
@sidharthachatterjee

sidharthachatterjee Aug 8, 2019

Member

As @pieh noted queryName is not non-nullable since there are indeed legitimate instances where a query to someType(id: { eq: "id-of-node-that-doesnt-exist" }) { id } could return null

Take for instance a series of queries one after another in gatsby-node whether someType is fetched for an id that may come from somewhere else.

queryNamePlural can on the other hand safely be a [Type]!

What are your thoughts to switching over to [Type]! for just the plural form in this PR?

This comment has been minimized.

Copy link
@spawnia

spawnia Aug 9, 2019

Author Contributor

I am now convinced that is correct, i changed the PR.

@sidharthachatterjee sidharthachatterjee changed the title Make root query fields of the GraphQL API non-null fix(gatsby): Make root plural fields non nullable Aug 9, 2019
@sidharthachatterjee sidharthachatterjee merged commit 2c79309 into gatsbyjs:master Aug 9, 2019
18 of 19 checks passed
18 of 19 checks passed
ci/circleci: e2e_tests_production_runtime Your tests failed on CircleCI
Details
Danger All good
Details
Peril All green. Well done.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
cypress: default-group 68 tests passed in 00:30
Details
unit_tests_windows Build #20190809.7 succeeded
Details
@spawnia spawnia deleted the spawnia:non-null-root-query-fields branch Aug 9, 2019
@sidharthachatterjee

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Featuring your work in this month's Gazette at #17548 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.