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.

Sign up for GitHub

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

Jump to bottom

[1.0] Make nodes fully immutable by adding API for allowing plugins to add fields #1035

Merged
merged 17 commits into from
May 26, 2017
Merged

[1.0] Make nodes fully immutable by adding API for allowing plugins to add fields #1035

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
449173e
remove updateNode action creator, rename 'pluginName' to 'pluginOwner…
KyleAMathews May 24, 2017
cdf2539
Add top-level field to nodes 'pluginFields'
KyleAMathews May 24, 2017
f9faf17
Replace 'updateNode' action creator with 'addChildNodeToParentNode' a…
KyleAMathews May 24, 2017
7cdbda6
Add test for nodes being created with field 'pluginFields'
KyleAMathews May 24, 2017
b8317cf
Use stripIndent so multiline strings look nicer in code
KyleAMathews May 25, 2017
62bbd99
Rename addChildNodeToParentNode to addNodeToParent
KyleAMathews May 25, 2017
d89607f
Update transformer tests
KyleAMathews May 25, 2017
918003a
Fixed bit of code that didn't get updated for node structure refactor
KyleAMathews May 25, 2017
8f77c1a
Simplify error message
KyleAMathews May 25, 2017
a31d391
Check if there's actually results so avoiding access error
KyleAMathews May 25, 2017
54d271b
Test nodes often don't have internal set
KyleAMathews May 26, 2017
e883aa1
Rename 'pluginFields' to 'fields'
KyleAMathews May 26, 2017
f44aa4b
Rename other fields with 'plugin' in them
KyleAMathews May 26, 2017
9e23038
Merge branch '1.0' into immutable-fields
KyleAMathews May 26, 2017
c847be1
Update migration guide with changes
KyleAMathews May 26, 2017
02a8ac3
Update gatsbygram example code to wrap graphql call in resolve
KyleAMathews May 26, 2017
a57cbc2
Update snapshot
KyleAMathews May 26, 2017
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Replace 'updateNode' action creator with 'addChildNodeToParentNode' a… 
…nd 'addFieldToNode'
  • Loading branch information
@KyleAMathews
KyleAMathews committed May 24, 2017
commit f9faf1766e704a7b0f342f228acc49b1f0fe68e9
5 changes: 5 additions & 0 deletions docs/docs/node-interface.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,13 @@ pluginFields: Object,
internal: {
contentDigest: String,
mediaType: String,
// A globally unique node type choosen by the plugin owner.
type: String,
// The plugin which created this node.
pluginOwner: String,
// Stores which plugins created which fields.
fieldPluginOwners: Object,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor Author

@KyleAMathews KyleAMathews May 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also @ivanoats
// Raw content for this node.
content: String,
}
...other node type specific fields
Expand Down
16 changes: 8 additions & 8 deletions packages/gatsby-transformer-json/src/__tests__/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already existed?

Noted on length. Yeah, I like really explicit APIs. I'll make it addNodeToParent.
const boundActionCreators = { createNode, addChildNodeToParentNode }

await onNodeCreate({
node,
loadNodeContent,
boundActionCreators,
}).then(() => {
expect(createNode.mock.calls).toMatchSnapshot()
expect(updateNode.mock.calls).toMatchSnapshot()
expect(addChildNodeToParentNode.mock.calls).toMatchSnapshot()
expect(createNode).toHaveBeenCalledTimes(2)
expect(updateNode).toHaveBeenCalledTimes(1)
expect(addChildNodeToParentNode).toHaveBeenCalledTimes(1)
})
})

Expand All @@ -49,8 +49,8 @@ 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()
const boundActionCreators = { createNode, addChildNodeToParentNode }

await onNodeCreate({
node,
Expand All @@ -71,8 +71,8 @@ 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()
const boundActionCreators = { createNode, addChildNodeToParentNode }

await onNodeCreate({
node,
Expand Down
9 changes: 5 additions & 4 deletions packages/gatsby-transformer-json/src/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const _ = require(`lodash`)
const crypto = require(`crypto`)

async function onNodeCreate({ node, boundActionCreators, loadNodeContent }) {
const { createNode, updateNode } = boundActionCreators
const { createNode, addChildNodeToParentNode } = boundActionCreators

// Don't reprocess our own nodes! (note: this doesn't normally happen
// but since this transformer creates new nodes with the same media-type
Expand Down Expand Up @@ -49,9 +49,10 @@ async function onNodeCreate({ node, boundActionCreators, loadNodeContent }) {
}
})

node.children = node.children.concat(JSONArray.map(n => n.id))
updateNode(node)
_.each(JSONArray, j => createNode(j))
_.each(JSONArray, j => {
createNode(j)
addChildNodeToParentNode({ parent: node, child: j })
})
}

return
Expand Down
2 changes: 1 addition & 1 deletion packages/gatsby-transformer-react-docgen/src/__tests__/on-node-create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe(`transformer-react-doc-gen: onNodeCreate`, () => {
loadNodeContent = jest.fn(node => readFile(node.__fixture))
boundActionCreators = {
createNode: jest.fn(n => createdNodes.push(n)),
updateNode: jest.fn(n => updatedNodes.push(n)),
addChildNodeToParentNode: jest.fn(n => updatedNodes.push(n)),
}
})

Expand Down
32 changes: 20 additions & 12 deletions packages/gatsby-transformer-react-docgen/src/on-node-create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ const propsId = (parentId, name) => `${parentId}--ComponentProp-${name}`
const descId = parentId => `${parentId}--ComponentDescription`

function createDescriptionNode(node, entry, boundActionCreators) {
if (!entry.description) return
const { createNode, updateNode } = boundActionCreators
if (!entry.description) return node
const { createNode } = boundActionCreators

const descriptionNode = {
id: descId(node.id),
Expand All @@ -25,8 +25,9 @@ function createDescriptionNode(node, entry, boundActionCreators) {

node.description___NODE = descriptionNode.id
node.children = node.children.concat([descriptionNode.id])
updateNode(node)
createNode(descriptionNode)

return node
}

function createPropNodes(node, component, boundActionCreators) {
Expand All @@ -37,7 +38,7 @@ function createPropNodes(node, component, boundActionCreators) {
let propNodeId = propsId(node.id, prop.name)
let content = JSON.stringify(prop)

const propNode = {
let propNode = {
...prop,
id: propNodeId,
children: [],
Expand All @@ -51,20 +52,20 @@ function createPropNodes(node, component, boundActionCreators) {
},
}
children[i] = propNode.id
propNode = createDescriptionNode(propNode, prop, boundActionCreators)
createNode(propNode)
createDescriptionNode(propNode, prop, boundActionCreators)
})

node.props___NODE = children
node.children = node.children.concat(children)
updateNode(node)
return node
}

export default function onNodeCreate(
{ node, loadNodeContent, boundActionCreators },
pluginOptions
) {
const { createNode, updateNode } = boundActionCreators
const { createNode, addChildNodeToParentNode } = boundActionCreators

if (node.internal.mediaType !== `application/javascript`) return null

Expand All @@ -77,7 +78,7 @@ export default function onNodeCreate(
const contentDigest = digest(strContent)
const nodeId = `${node.id}--${component.displayName}--ComponentMetadata`

const metadataNode = {
let metadataNode = {
...component,
props: null, // handled by the prop node creation
id: nodeId,
Expand All @@ -91,11 +92,18 @@ export default function onNodeCreate(
},
}

node.children = node.children.concat([metadataNode.id])
updateNode(node)
addChildNodeToParentNode({ parent: node, child: metadataNode })
metadataNode = createPropNodes(
metadataNode,
component,
boundActionCreators
)
metadataNode = createDescriptionNode(
metadataNode,
component,
boundActionCreators
)
createNode(metadataNode)
createPropNodes(metadataNode, component, boundActionCreators)
createDescriptionNode(metadataNode, component, boundActionCreators)
})
})
.catch(err => console.log(err))
Expand Down
8 changes: 4 additions & 4 deletions packages/gatsby-transformer-remark/src/__tests__/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,18 @@ Where oh where is my little pony?
node.content = content

const createNode = jest.fn()
const updateNode = jest.fn()
const boundActionCreators = { createNode, updateNode }
const addChildNodeToParentNode = jest.fn()
const boundActionCreators = { createNode, addChildNodeToParentNode }

await onNodeCreate({
node,
loadNodeContent,
boundActionCreators,
}).then(() => {
expect(createNode.mock.calls).toMatchSnapshot()
expect(updateNode.mock.calls).toMatchSnapshot()
expect(addChildNodeToParentNode.mock.calls).toMatchSnapshot()
expect(createNode).toHaveBeenCalledTimes(1)
expect(updateNode).toHaveBeenCalledTimes(1)
expect(addChildNodeToParentNode).toHaveBeenCalledTimes(1)
})
})
})
5 changes: 2 additions & 3 deletions packages/gatsby-transformer-remark/src/on-node-create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ module.exports = async function onNodeCreate({
loadNodeContent,
boundActionCreators,
}) {
const { createNode, updateNode } = boundActionCreators
const { createNode, addChildNodeToParentNode } = boundActionCreators

// Don't reprocess our own nodes! (note: this doesn't normally happen
// but since this transformer creates new nodes with the same media-type
Expand Down Expand Up @@ -50,7 +50,6 @@ module.exports = async function onNodeCreate({
markdownNode.fileAbsolutePath = node.absolutePath
}

node.children = node.children.concat([markdownNode.id])
updateNode(node)
createNode(markdownNode)
addChildNodeToParentNode({ parent: node, child: markdownNode })
}
8 changes: 4 additions & 4 deletions packages/gatsby-transformer-sharp/src/__tests__/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,17 @@ describe(`Process image nodes correctly`, () => {
},
}
const createNode = jest.fn()
const updateNode = jest.fn()
const boundActionCreators = { createNode, updateNode }
const addChildNodeToParentNode = jest.fn()
const boundActionCreators = { createNode, addChildNodeToParentNode }

await onNodeCreate({
node,
boundActionCreators,
}).then(() => {
expect(createNode.mock.calls).toMatchSnapshot()
expect(updateNode.mock.calls).toMatchSnapshot()
expect(addChildNodeToParentNode.mock.calls).toMatchSnapshot()
expect(createNode).toHaveBeenCalledTimes(1)
expect(updateNode).toHaveBeenCalledTimes(1)
expect(addChildNodeToParentNode).toHaveBeenCalledTimes(1)
})
})
})
5 changes: 2 additions & 3 deletions packages/gatsby-transformer-sharp/src/on-node-create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const _ = require(`lodash`)

module.exports = async function onNodeCreate({ node, boundActionCreators }) {
const { createNode, updateNode } = boundActionCreators
const { createNode, addChildNodeToParentNode } = boundActionCreators

const extensions = [`jpeg`, `jpg`, `png`, `webp`, `tif`, `tiff`, `svg`]
if (!_.includes(extensions, node.extension)) {
Expand All @@ -19,9 +19,8 @@ module.exports = async function onNodeCreate({ node, boundActionCreators }) {
},
}

node.children = node.children.concat([imageNode.id])
updateNode(node)
createNode(imageNode)
addChildNodeToParentNode({ parent: node, child: imageNode })

return
}
16 changes: 8 additions & 8 deletions packages/gatsby-transformer-yaml/src/__tests__/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,18 @@ describe(`Process YAML nodes correctly`, () => {
node.content = yaml.safeDump(data)

const createNode = jest.fn()
const updateNode = jest.fn()
const boundActionCreators = { createNode, updateNode }
const addChildNodeToParentNode = jest.fn()
const boundActionCreators = { createNode, addChildNodeToParentNode }

await onNodeCreate({
node,
loadNodeContent,
boundActionCreators,
}).then(() => {
expect(createNode.mock.calls).toMatchSnapshot()
expect(updateNode.mock.calls).toMatchSnapshot()
expect(addChildNodeToParentNode.mock.calls).toMatchSnapshot()
expect(createNode).toHaveBeenCalledTimes(2)
expect(updateNode).toHaveBeenCalledTimes(1)
expect(addChildNodeToParentNode).toHaveBeenCalledTimes(1)
})
})

Expand All @@ -47,8 +47,8 @@ describe(`Process YAML nodes correctly`, () => {
node.content = yaml.safeDump(data)

const createNode = jest.fn()
const updateNode = jest.fn()
const boundActionCreators = { createNode, updateNode }
const addChildNodeToParentNode = jest.fn()
const boundActionCreators = { createNode, addChildNodeToParentNode }

await onNodeCreate({
node,
Expand All @@ -69,8 +69,8 @@ describe(`Process YAML nodes correctly`, () => {
node.content = yaml.safeDump(data)

const createNode = jest.fn()
const updateNode = jest.fn()
const boundActionCreators = { createNode, updateNode }
const addChildNodeToParentNode = jest.fn()
const boundActionCreators = { createNode, addChildNodeToParentNode }

await onNodeCreate({
node,
Expand Down
9 changes: 5 additions & 4 deletions packages/gatsby-transformer-yaml/src/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const _ = require(`lodash`)
const crypto = require(`crypto`)

async function onNodeCreate({ node, boundActionCreators, loadNodeContent }) {
const { createNode, updateNode } = boundActionCreators
const { createNode, addChildNodeToParentNode } = boundActionCreators
if (node.internal.mediaType !== `text/yaml`) {
return
}
Expand Down Expand Up @@ -40,9 +40,10 @@ async function onNodeCreate({ node, boundActionCreators, loadNodeContent }) {
}
})

node.children = node.children.concat(yamlArray.map(y => y.id))
updateNode(node)
_.each(yamlArray, y => createNode(y))
_.each(yamlArray, y => {
createNode(y)
addChildNodeToParentNode({ parent: node, child: y })
})
}

return
Expand Down
1 change: 1 addition & 0 deletions packages/gatsby/src/joi-schemas/joi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export const nodeSchema = Joi.object()
mediaType: Joi.string().required(),
type: Joi.string().required(),
pluginOwner: Joi.string().required(),
fieldPluginOwners: Joi.array(),
content: Joi.string(),
}),
})
Expand Down
Loading