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): always add both childField and childrenField in GraphQL #28656

Merged
merged 9 commits into from
Dec 17, 2020

Conversation

vladar
Copy link
Contributor

@vladar vladar commented Dec 16, 2020

Description

Before this PR we create only one of the child fields depending on the number of elements in the node children array.

E.g. when children array contains 1 item we create this field:

type Foo {
  childBar: Bar
}

But when children array contains 2+ items, we create this field:

type Foo {
  childrenBar: [Bar]
}

The problem is that it relies on inference. And if at some point in time you had a single child node and then suddenly added a second child node - your schema will change from childBar to childrenBar and your queries will break.

With this PR we always add both fields instead:

type Foo {
  childBar: Bar
  childrenBar: [Bar]
}

In this case, childBar will always return the first child from the array of node children. And then it is up to users to pick the most appropriate field for their queries.

This also deprecates the many argument of the childOf directive 1

This is a non-breaking change. Also required to unblock #28483

Credits for this idea go to @pieh

Documentation

  • [childOf directive]1
  • [Child/parent fields]2
  • [Schema generation]3

Related Issues

#28483

Footnotes

  1. https://www.gatsbyjs.com/docs/reference/graphql-data-layer/schema-customization/#defining-child-relations 2

  2. https://www.gatsbyjs.com/docs/schema-inference/#childparent-fields

  3. https://www.gatsbyjs.com/docs/schema-generation/#4-parent--children-relationships

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 16, 2020
@vladar vladar 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 Dec 16, 2020
@vladar vladar marked this pull request as ready for review December 16, 2020 12:52
}
children.forEach(child => {
typeComposer.addFields(createChildrenField(child.getTypeName()))
typeComposer.addFields(createChildField(child.getTypeName()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's basically the essence of this PR. Other changed files are just tests/docs updates.

}
children.forEach(child => {
typeComposer.addFields(createChildrenField(child.getTypeName()))
typeComposer.addFields(createChildField(child.getTypeName()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same change, just in the different codepath

typeComposer.addFields(createChildField(typeName))
}
typeComposer.addFields(createChildrenField(typeName))
typeComposer.addFields(createChildField(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.

And one more entry. Other changes are mostly chores and don't have any effect on behavior.

@pieh pieh self-assigned this Dec 17, 2020
@pieh
Copy link
Contributor

pieh commented Dec 17, 2020

Just to leave papar trail for the future from our offline chat:

do you think, given that we will add both of them, it would help a little to add description to those fields?

const createChildrenField = typeName => {
return {
[fieldNames.convenienceChildren(typeName)]: {
type: () => [typeName],
resolve(source, args, context) {
const { path } = context
return context.nodeModel.getNodesByIds(
{ ids: source.children, type: typeName },
{ path }
)
},
},
}
}
const createChildField = typeName => {
return {
[fieldNames.convenienceChild(typeName)]: {
type: () => typeName,
resolve(source, args, context) {
const { path } = context
const result = context.nodeModel.getNodesByIds(
{ ids: source.children, type: typeName },
{ path }
)
if (result && result.length > 0) {
return result[0]
} else {
return null
}
},
},
}
}

another question is wether we should split PR due to docs site building from master branch for now
similar case to ( #28666 )

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.

Thank you!

@vladar vladar added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Dec 17, 2020
@gatsbybot gatsbybot merged commit 739df13 into master Dec 17, 2020
@gatsbybot gatsbybot deleted the vladar/stable-convenience-children branch December 17, 2020 22:02
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
…hQL (gatsbyjs#28656)

* fix(gatsby): always add both `childField` and `childrenField` in GraphQL

* Update docs

* fix null reference

* Update tests

* TODO item to remove `many` arg in `v3`

* Update snapshot

* add description to convenience child fields

* update description

* Revert docs for now
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.

None yet

3 participants