-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Conversation
Deploy preview for gatsbygram ready! Built with commit f017248 |
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://github.com/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. |
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.
Yeah, on second thought, let's not settle on a helper in core just yet. There's no rush to create higher order abstractions. |
And yeah, let's just auto-generate UUIDs 💯 |
That does the uuid stuff in |
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 |
If the node id test fails, we just warn for now and then in Gatsby v3 we hard fail. |
Thinking more about this — it shouldn't be an |
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. |
It should stay stable across content changes so content digest doesn't help. Files do have file paths :-) |
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. |
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. |
Yep, I'm gonna close this PR and start fresh in that case since there have been iterations on here. |
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