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

Switch Sources and Transformers to use UUIDv5 for id #3505

Closed
wants to merge 5 commits into from

Conversation

danielfarrell
Copy link
Contributor

This follows the pattern used in the lever and wordpress sources. The seedContent provided to each is a uuidv4 that is unique to the plugin to prevent any potential conflicts.

I also removed the tests on transformers that were checking that the id could be set manually.

fixes #1853

@gatsbybot
Copy link
Collaborator

Deploy preview for gatsbygram ready!

Built with commit f0172480937bc2be0643de527de072b03812fed8

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

@KyleAMathews
Copy link
Contributor

Cool! Thanks for jumping on this.

A few thoughts — I'm starting to feel we should centralize some of this — when I see really common & solid patterns emerge across a bunch of plugins, we should probably pull that into core.

E.g. @angeloashmore's project https:/angeloashmore/gatsby-node-helpers — it might make sense to pull some/all of his code into core to simplify creating nodes.

Also instead of individual plugins creating seeds — core could create seed UUIDs automatically based on the plugin name (which have to be unique), etc.

This would go a long ways towards ensuring IDs are random and not to be relied on.

Thoughts?

Also, this PR should be to the v2 branch as v1 IDs do have data in them that are sometimes (cough gatsbyjs.org cough) being used for filtering in GraphQL queries. So there's probably others in the wild doing the same thing.

@danielfarrell
Copy link
Contributor Author

Yeah, I think it makes sense to move that to core. There is no way to enforce the usage of this pattern otherwise. Not sure the helpers even need to be pulled in(might be worth it for other reasons), but we could just make a uuid from the id passed to createNode. Thoughts?

I don't think the seed matters that much in this case, because v5(and v3) are namespaced, so we namespace on the plugin name for the creation of that.

And I'll get this pointed to v2, thanks for pointing that out.

…#3503)

I came across this while browsing through the documentation. The `withPrefix` import was left unused in the "escape hatch" example, which I'm pretty sure is not intentional.
@KyleAMathews
Copy link
Contributor

Yeah, on second thought, let's not settle on a helper in core just yet. There's no rush to create higher order abstractions.

@KyleAMathews
Copy link
Contributor

And yeah, let's just auto-generate UUIDs 💯

@danielfarrell
Copy link
Contributor Author

danielfarrell commented Jan 13, 2018

That does the uuid stuff in createNode, but I have concerns that it is going to break scenarios where someone creates a node and then creates children nodes right after it based on it's id. :-/

@KyleAMathews
Copy link
Contributor

but I have concerns that it is going to break scenarios where someone creates a node and then creates children nodes right after it based on it's id. :-/

Agree this is a concern — perhaps actually we create an explicit "createId" action and then add some sort of signature to it that we can validate the eventual node's ID against so to ensure people are really using createId.

@KyleAMathews
Copy link
Contributor

If the node id test fails, we just warn for now and then in Gatsby v3 we hard fail.

@KyleAMathews
Copy link
Contributor

Thinking more about this — it shouldn't be an action — just a regular helper function passed in. Perhaps the name should be createNodeId.

@angeloashmore
Copy link
Contributor

The ID could be closely related to the content digest. Not all sources have content with an associated ID, e.g. files, but everything has a content digest.

I agree that it should be a helper function – one that Gatsby uses internally but also publicly available for plugins. Potentially it could just be concatenating the node type and content digest.

@KyleAMathews
Copy link
Contributor

It should stay stable across content changes so content digest doesn't help. Files do have file paths :-)

@danielfarrell
Copy link
Contributor Author

I am back in the land of the living now(the flu sucks) and can get back to this. Yeah, sounds like a helper is the way to go because we need to return the id. If we want to keep a list of who is not using it to warn them(and in v3 to blow up), the helper could call the action.

@KyleAMathews
Copy link
Contributor

Glad you're feeling better!

Sounds good re) helper.

Want to add that to the master branch first and replace the plugins with custom UUID creation? Then we can start replacing the ids for all core plugins for the v2 release.

@danielfarrell
Copy link
Contributor Author

Yep, I'm gonna close this PR and start fresh in that case since there have been iterations on here.

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.

5 participants