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 referencing derived types in schema customization #34787

Merged
merged 13 commits into from
Feb 15, 2022

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Feb 10, 2022

Changes:

  • bump to get fix in graphql-compose ( fix: don't try to instantiate GraphQLType when getting name of named type composer graphql-compose/graphql-compose#373 )
  • swap schema printer to use TypeComposers instead of graphql-js types - we do print schema before all "derived types" (such as our various filter and sort inputs etc) are created. This meant we couldn't ever reference such derived types and be able to print schema, because at the time of printing the type doesn't exist yet (causing printing to throw). I did try initially moving printing to after we create derived types but this changed too many things - it started printing everything and then we were getting exceptions that we try to create types reserved for Gatsby -
    const checkIsAllowedTypeName = name => {
    invariant(
    name !== `Node`,
    `The GraphQL type \`Node\` is reserved for internal use.`
    )
    invariant(
    !name.endsWith(`FilterInput`) && !name.endsWith(`SortInput`),
    `GraphQL type names ending with "FilterInput" or "SortInput" are ` +
    `reserved for internal use. Please rename \`${name}\`.`
    )
    invariant(
    ![`Boolean`, `Date`, `Float`, `ID`, `Int`, `JSON`, `String`].includes(name),
    `The GraphQL type \`${name}\` is reserved for internal use by ` +
    `built-in scalar types.`
    )
    assertValidName(name)
    }
    )
  • minor fix when we apply deprecation on sort fields that use arguments

[sc-46064]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 10, 2022
@pieh pieh added topic: GraphQL Related to Gatsby's GraphQL layer and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Feb 10, 2022
\\"\\"\\"This is description too\\"\\"\\"
withoutDefault: String
usingDerivedType: BarChildSortInput
): String
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all snapshot changes are additive - I expanded tests to include more cases

… iteration, only apply it if field or ancestor has args
@@ -65,6 +65,7 @@ const convert = <TContext = any>({
const sortFields = {}

Object.keys(fields).forEach(fieldName => {
let deprecationReason = parentFieldDeprecationReason
Copy link
Contributor Author

@pieh pieh Feb 10, 2022

Choose a reason for hiding this comment

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

Github doesn't show full picture in the diff by default as rest of the code is few lines below but this change is because

Object.keys(fields).forEach(fieldName => {
const fieldConfig = fields[fieldName]
const sortable =
typeComposer instanceof UnionTypeComposer ||
typeComposer instanceof ScalarTypeComposer
? undefined
: typeComposer.getFieldExtension(fieldName, `sortable`)
if (sortable === SORTABLE_ENUM.NOT_SORTABLE) {
return
} else if (sortable === SORTABLE_ENUM.DEPRECATED_SORTABLE) {
deprecationReason = `Sorting on fields that need arguments to resolve is deprecated.`
}

In particular on line 77 once a field is marked as deprecated, all the remaining fields in current iteration are also deprecated

For example:

type Foo {
  field1: Int # this is fine
  field2(arg1: Int): Int # this sets `deprecationReason`
  field3: Int # this should be fine, but currently marked as deprecated
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this fix #31523 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most likely, yes - at least comments about fields that don't have arguments but still causing this deprecation to show up

Copy link
Contributor Author

@pieh pieh Feb 11, 2022

Choose a reason for hiding this comment

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

I can move this to separate PR as this change is not dependend on rest of this PR, I did add it here as this warning was part of "issues" I encountered when working on rest of the changes here. The main problem for me was that Graphiql was not autosuggesting fields due to them being deprecated, but it was still working if I manually typed them in

Copy link
Contributor

Choose a reason for hiding this comment

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

Can stay in imo, I was just curious :)

@pieh pieh marked this pull request as ready for review February 10, 2022 23:22
@pieh pieh changed the title fix(gatsby): allow referencing derived types in type merging scenarios feat(gatsby): allow referencing derived types in type merging scenarios Feb 10, 2022
@@ -177,134 +212,127 @@ const printType = (tc, typeName) => {
return printScalarType(tc)
} else if (tc instanceof InputTypeComposer) {
return printInputObjectType(tc)
} else {
throw new Error(`Did not recognize type of ${typeName}.`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS was complaining, because all possible types in NamedTypeComposer were handled. Also we never actually passed typeName to this function

@pieh pieh changed the title feat(gatsby): allow referencing derived types in type merging scenarios feat(gatsby): allow referencing derived types in schema customization Feb 11, 2022
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Added some questions around typings

packages/gatsby/src/schema/print.ts Outdated Show resolved Hide resolved
packages/gatsby/src/schema/print.ts Outdated Show resolved Hide resolved
packages/gatsby/src/schema/print.ts Outdated Show resolved Hide resolved
packages/gatsby/src/schema/print.ts Outdated Show resolved Hide resolved
wardpeet
wardpeet previously approved these changes Feb 13, 2022
@pieh pieh merged commit 3d74584 into master Feb 15, 2022
@pieh pieh deleted the fix/schema-referencing-derived-types branch February 15, 2022 12:04
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

Successfully merging this pull request may close these issues.

None yet

3 participants