[1.0] Make nodes fully immutable by adding API for allowing plugins to add fields #1035
Conversation
…', and add check that only original node creator is updating a node
…nd 'addFieldToNode'
|
WIP still |
KyleAMathews
changed the title
|
Deploy preview ready! Built with commit f9faf17 |
|
Deploy preview ready! Built with commit a57cbc2 |
|
Deploy preview ready! Built with commit a57cbc2 |
|
Deploy preview ready! Built with commit a57cbc2 |
docs/docs/node-interface.md
Outdated
| // The plugin which created this node. | ||
| pluginOwner: String, | ||
| // Stores which plugins created which fields. | ||
| fieldPluginOwners: Object, |
There was a problem hiding this comment.
I would leave out the word plugin in all of these keys to make things more terse. I think the fact that they are created / owned by plugins is not that interesting. Because these fields will be used in users graphql queries I'd keep them as simple as possible without obscuring things.
There was a problem hiding this comment.
It was more to namespace things as "fields" is a pretty common name nodes might want to use... agree/disagree? And yeah, I dislike how long it is plus it's heavier conceptually which I dislike too.
There was a problem hiding this comment.
It's true that a name clash with fields is a little more likely to occur, but I would still choose a cleaner api over that.
There was a problem hiding this comment.
Also, thinking about it more, fields would be a bit of a weird name within Gatsby for a plugin to use when creating nodes as either you just put data as a top-level field or they should be children nodes.
Another argument for pluginFields however is that people will be confused as to why some data is in top-level fields and other is under fields. pluginFields would make it sorta obvious what the difference is. Though... perhaps not obvious enough to counter the extra length.
Most people doing anything with Gatsby will soon learn the distinction as adding custom fields to nodes is something many people will need to do for anything of medium or higher complexity...
Other thoughts? @jquense @fabien0102 @fk @scottyeck @nicholaswyoung
There was a problem hiding this comment.
Also @ivanoats
| @@ -26,18 +26,18 @@ describe(`Process JSON nodes correctly`, () => { | |||
| node.content = JSON.stringify(data) | |||
|
|
|||
| const createNode = jest.fn() | |||
| const updateNode = jest.fn() | |||
| const boundActionCreators = { createNode, updateNode } | |||
| const addChildNodeToParentNode = jest.fn() | |||
There was a problem hiding this comment.
This one already existed, but I can't help to suggest making this one shorter too 🙂 If you add a node to a parent it is obviously a child. So addChildNode or addNodeToParent would have been enough imo.
There was a problem hiding this comment.
It already existed?
Noted on length. Yeah, I like really explicit APIs. I'll make it addNodeToParent.
packages/gatsby/src/redux/actions.js
Outdated
| // Ensure node isn't directly setting pluginFields. | ||
| if (node.pluginFields) { | ||
| throw new Error( | ||
| `Plugins creating nodes can not set data on the reserved field "pluginFields" |
There was a problem hiding this comment.
I remember reading somewhere that common-tags were used. Would be nice to use stripIndent here, so that indentation can follow the code.
packages/gatsby/src/redux/actions.js
Outdated
| } else { | ||
| if (typeOwners[node.internal.type] !== plugin.name) { | ||
| throw new Error( | ||
| `The plugin "${plugin.name}" created a node of a type owned by another plugin. |
packages/gatsby/src/redux/actions.js
Outdated
| // the current plugin is the same as the previous. | ||
| if (oldNode && oldNode.internal.pluginOwner !== plugin.name) { | ||
| throw new Error( | ||
| `Nodes can only be updated by their owner. Node ${node.id} is |
packages/gatsby/src/redux/actions.js
Outdated
| const fieldOwner = node.internal.fieldPluginOwners[fieldName] | ||
| if (fieldOwner && fieldOwner !== plugin.name) { | ||
| throw new Error( | ||
| ` |
KyleAMathews
force-pushed
the
immutable-fields
branch
from
2561543 to
62bbd99
Compare
|
Deploy preview failed. Built with commit 62bbd99 https://app.netlify.com/sites/using-postcss-sass/deploys/592720f56f4c503c0969834e |
|
Andddddd we're done! |
No description provided.