V2 - Switch Sources and Transformers to use createNodeId for ids #3807
Conversation
ghost
assigned 
danielfarrell
changed the title
| @@ -28,8 +28,5 @@ | |||
| "main": "index.js", | |||
| "readme": "README.md", | |||
| "scripts": { | |||
| "build": "babel src --out-dir . --ignore __tests__", | |||
| "watch": "babel -w src --out-dir . --ignore __tests__", | |||
| "prepublish": "cross-env NODE_ENV=production npm run build" | |||
There was a problem hiding this comment.
this whole package should be deleted, the files here are leftover from a bad merge with master
| @@ -36,7 +36,7 @@ async function onCreateNode({ node, actions, loadNodeContent }, options) { | |||
|
|
|||
| return { | |||
| ...obj, | |||
| id: obj.id ? obj.id : `${node.id} [${i}] >>> CSV`, | |||
| id: createNodeId(obj.id ? obj.id : `${node.id} [${i}] >>> CSV`), | |||
There was a problem hiding this comment.
We should let people explicitly set IDs. There are use cases for that e.g. when creating mappings between nodes like we do for blog authors on gatsbyjs.org
Line 7 in 8261ac3
In nodes created from potentially manually created data, we should leave IDs alone.
| @@ -36,7 +36,7 @@ async function onCreateNode({ node, actions, loadNodeContent }, options) { | |||
|
|
|||
| return { | |||
| ...obj, | |||
| id: obj.id ? obj.id : `${node.id} [${n} ${i}] >>> ${node.extension}`, | |||
| id: createNodeId(obj.id ? obj.id : `${node.id} [${n} ${i}] >>> ${node.extension}`), | |||
There was a problem hiding this comment.
Same here, don't hash the manually created ID
| @@ -12,7 +14,7 @@ async function onCreateNode({ node, actions, loadNodeContent }) { | |||
| .digest(`hex`) | |||
| const jsonNode = { | |||
| ...obj, | |||
| id, | |||
| id: createNodeId(id), | |||
There was a problem hiding this comment.
Looks like we never were creating an ID if it was missing. Could you add that? But same here as the others, don't hash the manually created ID
| @@ -11,7 +11,7 @@ async function onCreateNode({ node, actions, loadNodeContent }) { | |||
| .digest(`hex`) | |||
| const jsonNode = { | |||
| ...obj, | |||
| id, | |||
| id: createNodeId(id), | |||
There was a problem hiding this comment.
Same here, don't hash the manually created ID
| @@ -26,7 +26,7 @@ async function onCreateNode({ node, actions, loadNodeContent }) { | |||
|
|
|||
| const newNode = { | |||
| ...parsedContent, | |||
| id: parsedContent.id ? parsedContent.id : `${node.id} >>> TOML`, | |||
| id: createNodeId(parsedContent.id ? parsedContent.id : `${node.id} >>> TOML`), | |||
There was a problem hiding this comment.
Same here, don't hash the manually created ID
|
I must be confused about the goal here. I thought we specifically wanted to push everything to get uuids for their ids. Using UUIDv5 only makes sense when you supply an id to it. It will be stable if you provide something stable, but still namespaced to the plugin. |
|
@danielfarrell I suspect reason to allow using supplied ids in json (and similar) transformers is that they are often used to link/join nodes with mapping. For example gatsby website use this to link blog post authors to author nodes. Below links how it is set up:
UUIDv5ing it would break mapping feature usage in current form - we would at very least need to figure out alternative convention to do this (so not try to link on node id, but on different field - maybe add another reserved field with original/source id and try to use that to link or allow specyfing in mapping settings which field to join on) |
|
Yeah, if a user manually adds an ID then it's reasonable that they'd want to be able to link to elsewhere. UUIDs are mostly for improving how source/transformers auto-generate IDs. Instead of leaving them 1/2 meaningful e.g. |
|
@danielfarrell I'll take this over — hope your health issues work out! |
|
Thanks @danielfarrell for your great work! |
…sbyjs#3807) * Switch Sources and Transformers to use createNodeId for ids * Only use createNodeId if an id isn't passed in * Remove unused imports
Fixes #1853