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-source-filesystem): Let plugins set parent when creating File nodes with createRemoteFileNode #11795

Merged
merged 5 commits into from
Feb 20, 2019

Conversation

KyleAMathews
Copy link
Contributor

Not doing this is causing problems as a) when the server is restarted, if the parent of a remote 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.

This PR is experimental still. Will be putting together a quick test case to make sure this fixes things.

@KyleAMathews
Copy link
Contributor Author

Validated this code change with this gatsby-node.js:

const { createRemoteFileNode } = require(`gatsby-source-filesystem`);

exports.sourceNodes = ({ actions, createNodeId }) => {
  const { createNode } = actions;
  createNode({
    id: createNodeId(`test`),
    cool: `yeah`,
    url: `https://t3.ftcdn.net/jpg/00/92/53/56/240_F_92535664_IvFsQeHjBzfE6sD4VHdO8u5OHUSc6yHF.jpg`,
    internal: {
      type: `TESTING`,
      contentDigest: `1`,
    },
  });
};

exports.onCreateNode = async ({
  node,
  actions,
  store,
  cache,
  createNodeId,
}) => {
  const { createNode, createNodeField } = actions;
  if (node.internal.type === `TESTING`) {
    console.log({ node, actions, createNodeId });
    const fileNode = await createRemoteFileNode({
      url: node.url,
      store,
      cache,
      createNode,
      createNodeId,
      parentNodeId: node.id,
    });
    console.log({ fileNode });
    createNodeField({
      node,
      name: `urlFile___NODE`,
      value: fileNode.id,
    });
  }
};

@KyleAMathews KyleAMathews marked this pull request as ready for review February 15, 2019 16:09
@KyleAMathews KyleAMathews changed the title Let plugins set parent when creating File nodes with createRemoteFileNode fix(gatsby-source-filesystem): Let plugins set parent when creating File nodes with createRemoteFileNode Feb 15, 2019
@KyleAMathews
Copy link
Contributor Author

@gatsbyjs/core bump

@sidharthachatterjee sidharthachatterjee self-assigned this Feb 20, 2019
@pieh
Copy link
Contributor

pieh commented Feb 20, 2019

I think this is good - we need to document that people need to use parentNodeId when using createRemoteFileNode in onCreateNode API.

Should we also add warning for nodes creaated in onCreateNode that don't have parent set? Or will we do this implicit parent?

@sidharthachatterjee
Copy link
Contributor

Is there value in adding the above code that you validated against as an end to end test?

@KyleAMathews
Copy link
Contributor Author

Is there value in adding the above code that you validated against as an end to end test?

Probably not — this is just normal node stuff which is tested elsewhere — the problem was just that we didn't let plugins set themselves as the parent when using createRemoteFileNode.

@pieh yeah, will document & add warning.

@sidharthachatterjee
Copy link
Contributor

@KyleAMathews Alright, testing and verifying this locally

@KyleAMathews
Copy link
Contributor Author

Hmm actually their is no way to warn since there's no way to know which plugin called createRemoteFileNode. I think the main thing to do is to file issues in various plugins that use this helper.

@sidharthachatterjee sidharthachatterjee removed their assignment Feb 20, 2019
@pieh
Copy link
Contributor

pieh commented Feb 20, 2019

Hmm actually their is no way to warn since there's no way to know which plugin called createRemoteFileNode. I think the main thing to do is to file issues in various plugins that use this helper.

So there is this trick I've used for createPage warning - we can create ad hoc stack-trace and then get first frame that is not originating from gatsby-source-filestem to get actual file (and location in file) where this was called

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee 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, thanks Kyle!

@sidharthachatterjee sidharthachatterjee merged commit 5a3c1fc into master Feb 20, 2019
@DSchau DSchau deleted the file-parent branch February 20, 2019 19:54
@sidharthachatterjee
Copy link
Contributor

sidharthachatterjee commented Feb 20, 2019

Successfully published:

- gatsby-source-drupal@3.0.24
- gatsby-source-filesystem@2.0.21
- gatsby-source-shopify@2.0.15
- gatsby-source-wordpress@3.0.37
- gatsby-transformer-screenshot@2.0.12
- gatsby@2.1.11

@KyleAMathews
Copy link
Contributor Author

So there is this trick I've used for createPage warning - we can create ad hoc stack-trace and then get first frame that is not originating from gatsby-source-filestem to get actual file (and location in file) where this was called

Ooo that's cool — I'll create an issue for it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants