perf(gatsby): Create page object & SitePage node in same action creator (#31104)

* feat(gatsby): don't duplicate page data into SitePage nodes

* Fix lint

* treat pages as actual sitePage nodes

* change flag name to reflect change in strategy

* fix type

* Add missing field + test with LOCKED_IN

* Fix bugs... maybe

* fix types

* fixes hopefully

* Move this from reducer to action so node also gets it

* Avoid calling onCreatePage if can

* set pluginCreator___NODE and pluginCreatorId always, not just when flag is enabled

* Early return from node API calls if there's no implementing plugins

* Return something

* Also always run api when deferNodeMutation is set — apparently it triggers a side effect somewhere...

* Only call onCreateNode for SitePage nodes

* Only start plugin runner after plugins are created so we have plugins available to reason around

* Recreate pages from SitePage cache vs. persisting/restoring in seperately

Saves 100mb of heap for 100k page create-pages benchmark

* Maybe fix

* Ignore

* Revert attempt to restore page state from SitePage nodes

* Remove flag

* Remove unnecessary implementation of onCreatePage

Drops 100k create-pages benchmark from ~8s to 4.5s as it means
we don't need to invoke `onCreatePage` by default.

* Remove unnecessary changes

* Fix tests

* fix prod-404 and webpack suspending timing (#31236)

* fix 404

* drop checking defered node mutations when bailing

* suspend after first compilation

* actually check for plugins implementing onCreateNode

Co-authored-by: Michal Piechowiak <[email protected]>

Author: KyleAMathews

benchmarks/create-pages/package.json

@@ -9,9 +9,9 @@
     "serve": "gatsby serve"
   },
   "dependencies": {
-    "gatsby": "^2.19.5",
-    "react": "^16.12.0",
-    "react-dom": "^16.12.0"
+    "gatsby": "^3.4.0",
+    "react": "^17.0.2",
+    "react-dom": "^17.0.2"
   },
   "devDependencies": {
     "gatsby-plugin-benchmark-reporting": "*"

integration-tests/artifacts/.gitignore

@@ -1 +1,3 @@
-__tests__/__debug__
+__tests__/__debug__
+node_modules
+yarn.lock

packages/gatsby/src/bootstrap/load-plugins/__tests__/__snapshots__/load-plugins.ts.snap

@@ -37,7 +37,6 @@ Array [
     "nodeAPIs": Array [
       "sourceNodes",
       "createResolvers",
-      "onCreatePage",
     ],
     "pluginOptions": Object {
       "plugins": Array [],
@@ -50,9 +49,7 @@ Array [
     "browserAPIs": Array [],
     "id": "",
     "name": "prod-404",
-    "nodeAPIs": Array [
-      "onCreatePage",
-    ],
+    "nodeAPIs": Array [],
     "pluginOptions": Object {
       "plugins": Array [],
     },
@@ -287,7 +284,6 @@ Array [
     "nodeAPIs": Array [
       "sourceNodes",
       "createResolvers",
-      "onCreatePage",
     ],
     "pluginOptions": Object {
       "plugins": Array [],
@@ -300,9 +296,7 @@ Array [
     "browserAPIs": Array [],
     "id": "",
     "name": "prod-404",
-    "nodeAPIs": Array [
-      "onCreatePage",
-    ],
+    "nodeAPIs": Array [],
     "pluginOptions": Object {
       "plugins": Array [],
     },
@@ -567,7 +561,6 @@ Array [
     "nodeAPIs": Array [
       "sourceNodes",
       "createResolvers",
-      "onCreatePage",
     ],
     "pluginOptions": Object {
       "plugins": Array [],
@@ -580,9 +573,7 @@ Array [
     "browserAPIs": Array [],
     "id": "",
     "name": "prod-404",
-    "nodeAPIs": Array [
-      "onCreatePage",
-    ],
+    "nodeAPIs": Array [],
     "pluginOptions": Object {
       "plugins": Array [],
     },

packages/gatsby/src/internal-plugins/internal-data-bridge/gatsby-node.js

@@ -50,20 +50,6 @@ exports.sourceNodes = ({
   const { createNode, deleteNode } = actions
   const { program, flattenedPlugins, config } = store.getState()
 
-  // Add our default development page since we know it's going to
-  // exist and we need a node to exist so its query works :-)
-  const page = { path: `/dev-404-page/` }
-  createNode({
-    ...page,
-    id: createPageId(page.path),
-    parent: null,
-    children: [],
-    internal: {
-      type: `SitePage`,
-      contentDigest: createContentDigest(page),
-    },
-  })
-
   flattenedPlugins.forEach(plugin => {
     plugin.pluginFilepath = plugin.resolve
     createNode({
@@ -226,29 +212,8 @@ exports.createResolvers = ({ createResolvers }) => {
       },
     },
   }
-  createResolvers(resolvers)
-}
 
-exports.onCreatePage = ({ createContentDigest, page, actions }) => {
-  const { createNode } = actions
-  // eslint-disable-next-line
-  const { updatedAt, ...pageWithoutUpdated } = page
-
-  // Add page.
-  createNode({
-    ...pageWithoutUpdated,
-    id: createPageId(page.path),
-    parent: null,
-    children: [],
-    internal: {
-      type: `SitePage`,
-      contentDigest: createContentDigest(pageWithoutUpdated),
-      description:
-        page.pluginCreatorId === `Plugin default-site-plugin`
-          ? `Your site's "gatsby-node.js"`
-          : page.pluginCreatorId,
-    },
-  })
+  createResolvers(resolvers)
 }
 
 // Listen for DELETE_PAGE and delete page nodes.

packages/gatsby/src/internal-plugins/prod-404/gatsby-node.js

@@ -4,18 +4,27 @@ const { actions } = require(`../../redux/actions`)
 const PROD_404_PAGE_PATH = `/404.html`
 
 let page404 = null
-exports.onCreatePage = ({ page, store, actions }) => {
+emitter.on(`CREATE_PAGE`, action => {
   // Copy /404/ to /404.html as many static site hosts expect
   // site 404 pages to be named this.
   // https://www.gatsbyjs.org/docs/how-to/adding-common-features/add-404-page/
-  if (!page404 && /^\/?404\/?$/.test(page.path)) {
-    actions.createPage({
-      ...page,
-      path: PROD_404_PAGE_PATH,
-    })
-    page404 = page
+  if (!page404 && /^\/?404\/?$/.test(action.payload.path)) {
+    page404 = {
+      path: action.payload.path,
+      component: action.payload.component,
+      context: action.payload.context,
+    }
+    store.dispatch(
+      actions.createPage(
+        {
+          ...page404,
+          path: PROD_404_PAGE_PATH,
+        },
+        action.plugin
+      )
+    )
   }
-}
+})
 
 emitter.on(`DELETE_PAGE`, action => {
   if (page404 && action.payload.path === page404.path) {

packages/gatsby/src/redux/__tests__/index.js

@@ -81,7 +81,7 @@ jest.mock(`glob`, () => {
 function getFakeNodes() {
   // Set nodes to something or the cache will fail because it asserts this
   // Actual nodes content should match TS type; these are verified
-  const map /* : Map<string, IReduxNode>*/ = new Map()
+  const map /* : Map<string, IReduxNode> */ = new Map()
   map.set(`pageA`, {
     id: `pageA`,
     internal: {
@@ -108,10 +108,11 @@ describe(`redux db`, () => {
     Date.now = jest.fn(() => ++DateNowCallCount)
 
     store.dispatch(
-      (Array.isArray(pages) ? pages : [pages]).map(page =>
-        createPage(page, {
-          name: `default-site-plugin`,
-        })
+      (Array.isArray(pages) ? pages : [pages]).map(
+        page =>
+          createPage(page, {
+            name: `default-site-plugin`,
+          }).filter(a => a.type === `CREATE_PAGE`)[0]
       )
     )
 

packages/gatsby/src/redux/__tests__/pages.ts

@@ -24,13 +24,15 @@ glob.sync = jest.fn()
 
 describe(`Add pages`, () => {
   it(`allows you to add pages`, () => {
-    const action = actions.createPage(
-      {
-        path: `/hi/`,
-        component: `/whatever/index.js`,
-      },
-      { id: `test`, name: `test` }
-    )
+    const action = actions
+      .createPage(
+        {
+          path: `/hi/`,
+          component: `/whatever/index.js`,
+        },
+        { id: `test`, name: `test` }
+      )
+      .filter(a => a.type === `CREATE_PAGE`)[0]
     const state = reducer(undefined, action)
     expect(action).toMatchSnapshot()
     expect(state).toMatchSnapshot()
@@ -83,85 +85,98 @@ describe(`Add pages`, () => {
   })
 
   it(`adds an initial forward slash if the user doesn't`, () => {
-    const action = actions.createPage(
+    const actionsArray = actions.createPage(
       {
         path: `hi/`,
         component: `/whatever/index.js`,
       },
       { id: `test`, name: `test` }
     )
+    const action = actionsArray.filter(a => a.type === `CREATE_PAGE`)[0]
     const state = reducer(undefined, action)
     expect(Array.from(state.values())[0].path).toEqual(`/hi/`)
   })
 
   it(`allows you to add pages with context`, () => {
-    const action = actions.createPage(
-      {
-        path: `/hi/`,
-        component: `/whatever/index.js`,
-        context: {
-          id: 123,
+    const action = actions
+      .createPage(
+        {
+          path: `/hi/`,
+          component: `/whatever/index.js`,
+          context: {
+            id: 123,
+          },
         },
-      },
-      { id: `test`, name: `test` }
-    )
+        { id: `test`, name: `test` }
+      )
+      .filter(a => a.type === `CREATE_PAGE`)[0]
     const state = reducer(undefined, action)
     expect(action).toMatchSnapshot()
     expect(state).toMatchSnapshot()
   })
 
   it(`allows you to add pages with matchPath`, () => {
-    const action = actions.createPage(
-      {
-        path: `/hi/`,
-        component: `/whatever/index.js`,
-        matchPath: `/hi-from-somewhere-else/`,
-      },
-      { id: `test`, name: `test` }
-    )
+    const action = actions
+      .createPage(
+        {
+          path: `/hi/`,
+          component: `/whatever/index.js`,
+          matchPath: `/hi-from-somewhere-else/`,
+        },
+        { id: `test`, name: `test` }
+      )
+      .filter(a => a.type === `CREATE_PAGE`)[0]
     const state = reducer(undefined, action)
     expect(action).toMatchSnapshot()
     expect(state).toMatchSnapshot()
   })
 
   it(`allows you to add multiple pages`, () => {
-    const action = actions.createPage(
-      {
-        path: `/hi/`,
-        component: `/whatever/index.js`,
-      },
-      { id: `test`, name: `test` }
-    )
-    const action2 = actions.createPage(
-      {
-        path: `/hi/pizza/`,
-        component: `/whatever/index.js`,
-      },
-      { id: `test`, name: `test` }
-    )
+    const action = actions
+      .createPage(
+        {
+          path: `/hi/`,
+          component: `/whatever/index.js`,
+        },
+        { id: `test`, name: `test` }
+      )
+      .filter(a => a.type === `CREATE_PAGE`)[0]
+    const action2 = actions
+      .createPage(
+        {
+          path: `/hi/pizza/`,
+          component: `/whatever/index.js`,
+        },
+        { id: `test`, name: `test` }
+      )
+      .filter(a => a.type === `CREATE_PAGE`)[0]
     let state = reducer(undefined, action)
     state = reducer(state, action2)
     expect(state).toMatchSnapshot()
     expect(state.size).toEqual(2)
   })
 
   it(`allows you to update existing pages (based on path)`, () => {
-    const action = actions.createPage(
-      {
-        path: `/hi/`,
-        component: `/whatever/index.js`,
-      },
-      { id: `test`, name: `test` }
-    )
+    const action = actions
+      .createPage(
+        {
+          path: `/hi/`,
+          component: `/whatever/index.js`,
+        },
+        { id: `test`, name: `test` }
+      )
+      .filter(a => a.type === `CREATE_PAGE`)[0]
 
     // Change the component
-    const action2 = actions.createPage(
-      {
-        path: `/hi/`,
-        component: `/whatever2/index.js`,
-      },
-      { id: `test`, name: `test` }
-    )
+    const action2 = actions
+      .createPage(
+        {
+          path: `/hi/`,
+          component: `/whatever2/index.js`,
+        },
+        { id: `test`, name: `test` }
+      )
+      .filter(a => a.type === `CREATE_PAGE`)[0]
 
     let state = reducer(undefined, action)
     state = reducer(state, action2)
@@ -170,13 +185,15 @@ describe(`Add pages`, () => {
   })
 
   it(`allows you to delete paths`, () => {
-    const action = actions.createPage(
-      {
-        path: `/hi/`,
-        component: `/whatever/index.js`,
-      },
-      { id: `test`, name: `test` }
-    )
+    const action = actions
+      .createPage(
+        {
+          path: `/hi/`,
+          component: `/whatever/index.js`,
+        },
+        { id: `test`, name: `test` }
+      )
+      .filter(a => a.type === `CREATE_PAGE`)[0]
     const action2 = actions.deletePage({ path: `/hi/` })
 
     let state = reducer(undefined, action)