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): print childOf directive for implicit child fields #28483

Merged
merged 7 commits into from
Dec 23, 2020

Conversation

vladar
Copy link
Contributor

@vladar vladar commented Dec 4, 2020

Description

Fixes a bug with schema printing of inferred childOf directive. Before this PR if we print a schema with infered child fields (e.g. File.childImageSharp) it won't include childOf directive:

type ImageSharp implements Node @dontInfer {
  id: ID!
}

After this PR it correctly prints this:

type ImageSharp implements Node @dontInfer @childOf(types: ["File"]) {
  id: ID!
}

This fixed variant will correctly wotk in Gatsby v3. The variant before this PR will fail to work in Gatsby v3 (the printed schema snapshot won't add the File.childImageSharp field).

Potentially a breaking change :/

Worked around breaking change via #28656

See details (no longer actual) Requires a bit of investigation. To illustrate a possible break. With this change, we automatically apply the `childOf` extension (instead of inferring child fields):
type Foo implements Node @childOf(types: ["Bar", "Baz"], many: true) {
  id: ID!
}

The many argument here is what is problematic. Before this change (with simple inference) we could get these two types (at least theoretically):

type Bar {
  childFoo: Foo
}
type Baz {
  childrenFoo: [Foo]
}

so in one case - singular Bar.childFoo in the other case - array Baz.childrenFoo. After this change, we will always have both singular or both arrays. Mixing is not allowed with this directive.

The many argument applies to all parent types. Not sure if it is intentional or an oversight.

Ways to fix this

  1. Suggested by @pieh : Always add both child{Field} and children{Field} to the schema and deprecate many option altogether. Wih this change child{Field} will always return the first child (as listed in node children array)

  2. Support the new syntax in childOf directive where we can define many per type. Something like:

type Foo implements Node @childOf(parents: [{name: "Bar", many: false}, { name: "Baz", many: true }]) {
  id: ID!
}

Related issues

Fixes #19674

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 4, 2020
@vladar vladar added topic: GraphQL Related to Gatsby's GraphQL layer breaking change If implemented, this proposed work would break functionality for older versions of Gatsby and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Dec 4, 2020
@vladar vladar removed the breaking change If implemented, this proposed work would break functionality for older versions of Gatsby label Dec 21, 2020
@vladar vladar marked this pull request as ready for review December 21, 2020 12:15
childOfExtension = { types: [] }
}
childOfExtension.types.push(parentTypeName)
childTypeComposer.setExtension(`childOf`, childOfExtension)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The essence of this PR: instead of adding fields directly we add the childOf directive here. And fields are always added in a single place now - where this directive is processed (in addConvenienceChildrenFields):

children.forEach(child => {
typeComposer.addFields(createChildrenField(child.getTypeName()))
typeComposer.addFields(createChildField(child.getTypeName()))
})

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Looks great! That was quite a journey to get to this point (including other PR(s) on the way heh and possibly some ahead (weird behaviour of update option on schema snapshot plugin ;) ).

Let's get this in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: GraphQL Related to Gatsby's GraphQL layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gatsby-plugin-schema-snapshot Issue with Contentful LongText fields
3 participants