Skip to content

docs(gatsby-source-filesystem): add warning about parentNodeId for util functions like createFileNodeFromBuffer #34313

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

georgiee
Copy link
Contributor

@georgiee georgiee commented Dec 23, 2021

Description

We wanted to implement a social card that is generated on the fly for our pages. createFileNodeFromBuffer was our obvious choice. We followed the playbook given by Gatsby, here the relevant docs:

Steps:

  1. Create the file node with createFileNodeFromBuffer
  2. Write the file node id to given node with createNodeField
  3. Resolve the fk relationship through createSchemaCustomization & @link

Everything worked, but only on the first start. Repeated starts showed the file being null. Clearly something was going on when loaded from cache. We studied the documentation and searched for examples on github through the code search. We didn't found obvious solutions only a few issues describing similar issues in other contexts.

When I compared the two function's parameter signature I noted a small difference that technically did not make a difference but semantically. Inside createFileNodeFromBuffer parentNodeId receives a default value of null and that value is not mentioned in the documentation.

On the other hand the documentation for createRemoteFileNode mentions that value and the source code shows now default null assignment for parentNodeId.

When I thought about the cache and dangling file nodes I guessed this is the problem and in the end this was exactly the issue. @KyleAMathews himself created an issue (#11942) almost 3 years ago to handle this problem but it was closed by the stale bot and forgotten I guess? The related PR which introduced the parentNodeId and where Kyle commented the problem can be found here #11795 (comment).

Documentation

This PR brings the necessary changes in the documentation and I chose to print a warning for createFileNodeFromBuffer & createRemoteFileNode which reads like this for createFileNodeFromBuffer:

It seems that you forgot to pass in 'parentNodeId' for a file you try to create with 'createFileNodeFromBuffer'. Not doing this is causing problems as a) when the server is restarted, if the parent of the created file node is loaded from the cache, the linked file node won't be recreated which means it will be garbage collected and b) if a parent node is deleted, the linked file node won't also be deleted.

The documentation was adapted for the following parts:

Note: I copied and adapted large positions from Kyle's feedback to this issue but I had no idea to give proper credit, so I'm doing it here instead. Thanks.

Remarks

I actually would like to make parentNodeId a mandatory field and throw an exception like it's done for other inputs. That would be a breaking change I fear so I chose to only warn about the issue. Does some maintainer have an opinion on this?

Preview

The rendered documentation files change in those aspects. Red highlights only to highlight the screenshots.

Warning for createFileNodeFromBuffer

Screen Shot 2021-12-23 at 11 51 20

Warning for createRemoteFileNode

Screen Shot 2021-12-23 at 11 51 14

Updated Example

Screen Shot 2021-12-23 at 11 52 38

Troubleshooting Addition

Screen Shot 2021-12-23 at 11 51 26

Related Issues

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 23, 2021
Comment on lines +151 to +156
if (parentNodeId === null) {
console.warn(
`It seems that you forgot to pass in 'parentNodeId' for a file you try to create with 'createFileNodeFromBuffer'. Not doing this is causing problems as a) when the server is restarted, if the parent of the created file node is loaded from the cache, the linked file node won't be recreated which means it will be garbage collected and b) if a parent node is deleted, the linked file node won't also be deleted.`
)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I would like to enforce this and throw an error, but I don't dare to suggest such a potential breaking chance. Does one of the maintainers has an opinion on that? Do we have use cases where parentNodeId is indeed allowed to be null?

Comment on lines +240 to +245
if (parentNodeId === null) {
console.warn(
`It seems that you forgot to pass in 'parentNodeId' for a file you try to create with 'createRemoteFileNode'. Not doing this is causing problems as a) when the server is restarted, if the parent of the created file node is loaded from the cache, the linked file node won't be recreated which means it will be garbage collected and b) if a parent node is deleted, the linked File node won't also be deleted.`
)
}

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 as in create-file-node-from-buffer.js:

Actually I would like to enforce this and throw an error, but I don't dare to suggest such a potential breaking chance. Does one of the maintainers has an opinion on that? Do we have use cases where parentNodeId is indeed allowed to be null?

@georgiee georgiee force-pushed the gk-file-node-suggestions branch from 8867ba9 to b99c43a Compare December 23, 2021 11:00
@TylerBarnes TylerBarnes added topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants