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

[gatsby-source-contentful] create children nodes after parent was created #4429

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Mar 8, 2018

This fixes #4401, but issue a bit more nuanced than this fix alone:

Deleting possibly stale descendant nodes in createNode now force to create children nodes after creating parent node (or else children might be deleted upon parent node creation). This is fine (as there is no other way to remove stale nodes), but I don't think it's documented anywhere yet.

But before documenting that we should probably define when to use node parent/children because as of right now plugins do use them in at least 2 different ways:

Is it fine to use parent/children in both ways?
If so I will try to add some warnings about situation like this (creating parent after the children) so plugin maintainers will have (more) helpful warning/error messages instead of non-descriptive error in gatsby internals. If parent/children should be reserved for transformers then parent/children should be more precisely documented and plugins updated to not use those fields in that cases.

I searched docs and currently found very few mentions of parent/children:

This is used when you transform content from a node creating a new child node

Nodes created by transformer plugins are set as “children” of their “parent” nodes.

@ghost ghost assigned pieh Mar 8, 2018
@ghost ghost added the review label Mar 8, 2018
@gatsbybot
Copy link
Collaborator

gatsbybot commented Mar 8, 2018

Deploy preview for gatsbygram ready!

Built with commit 9ff09b1

https://deploy-preview-4429--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

Deploy preview for using-drupal ready!

Built with commit 6c5d52540ee3b3c2175f7068a96ab2acf22cc313

https://deploy-preview-4429--using-drupal.netlify.com

@pieh
Copy link
Contributor Author

pieh commented Mar 8, 2018

I will check failed test tomorrow, but from quick glance it's just snapshot not updated (order of nodes is changed)

@KyleAMathews
Copy link
Contributor

Yeah, we've been doing this inconsistently. There's some source plugins that don't create the parent/child link and it's fine.

I can't think of any reason not to restrict parent/child relationships to source/transformed node relationships. Clearly defining that would simplify understanding I think how Gatsby constructs data relationships and how you should model your data in Gatsby if you're doing something custom.

parent/child is for transformers

Want to create links between nodes? Creating mappings or use ___NODE w/ createNodeField in onCreateNode.

@pieh pieh changed the title [gatsby-source-contentful] create children nodes after parent was created [gatsby-source-contentful] don't use parent/children for linked nodes Mar 8, 2018
@pieh
Copy link
Contributor Author

pieh commented Mar 8, 2018

Updated PR with quick fix for contentful. Will be doing pass through plugins to find other places where parent/children is used in addition to node linking in follow up PR.

This might potentially break some websites if they use childrenX instead of linked-nodes fields, but I think this need to be done.

I will also update parent/children docs. Is it sensible that setting parent should be most often (if not always) done in onCreateNode hook?

@KyleAMathews
Copy link
Contributor

Yeah, in onCreateNode with https://www.gatsbyjs.org/docs/bound-action-creators/#createParentChildLink since you can't mutate other plugin's nodes.

Doing this for v2 would be great as most plugins are going to have breaking changes anyways and people will be expecting to do a lot of little upgrades. It'd be a pretty simple upgrade for most plugins as you'd just find/replace in your GraphQL queries.

@KyleAMathews
Copy link
Contributor

Oh but yeah, don't make the breaking change here for gatsby-source-contentful. Do that on the v2 branch since I think we already incremented its major version there.

@pieh
Copy link
Contributor Author

pieh commented Mar 9, 2018

OK, I reverted PR to previous fix for master (keep parent/children, just delay creating children nodes to after parent node was created) - it wasn't potentially breaking change. I will create separate PR for v2 when I'll finish with similar fixes for other plugins in master.

@pieh pieh changed the title [gatsby-source-contentful] don't use parent/children for linked nodes [gatsby-source-contentful] create children nodes after parent was created Mar 9, 2018
@KyleAMathews KyleAMathews merged commit 324dfd9 into gatsbyjs:master Mar 9, 2018
@ghost ghost removed the review label Mar 9, 2018
@KyleAMathews
Copy link
Contributor

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants