feat(gatsby): Match node manifest pages by page context slug (#34790)

TylerBarnes · web-flow · commit ba8e21c32b9a · 2022-02-11T12:20:12.000-08:00
* Match node manifest pages by page context slug

* add docs link
diff --git a/integration-tests/node-manifest/__tests__/create-node-manifest.test.js b/integration-tests/node-manifest/__tests__/create-node-manifest.test.js
@@ -70,6 +70,14 @@ describe(`Node Manifest API in "gatsby ${gatsbyCommandName}"`, () => {
     expect(manifestFileContents.foundPageBy).toBe(`context.id`)
   })
 
+  it(`Creates an accurate node manifest when ownerNodeId isn't present but there's a matching "slug" in pageContext`, async () => {
+    const manifestFileContents = await getManifestContents(5)
+
+    expect(manifestFileContents.node.id).toBe(`5`)
+    expect(manifestFileContents.page.path).toBe(`/slug-test-path`)
+    expect(manifestFileContents.foundPageBy).toBe(`context.slug`)
+  })
+
   if (gatsbyCommandName === `build`) {
     // this doesn't work in gatsby develop since query tracking
     // only runs when visiting a page in browser.
diff --git a/integration-tests/node-manifest/gatsby-node.js b/integration-tests/node-manifest/gatsby-node.js
@@ -4,7 +4,7 @@ const createManifestId = nodeId => `${commandName}-${nodeId}`
 
 exports.sourceNodes = ({ actions }) => {
   // template nodes
-  for (let id = 1; id < 5; id++) {
+  for (let id = 1; id < 6; id++) {
     const node = {
       id: `${id}`,
       internal: {
@@ -13,6 +13,10 @@ exports.sourceNodes = ({ actions }) => {
       },
     }
 
+    if (id === 5) {
+      node.slug = `test-slug`
+    }
+
     actions.createNode(node)
 
     actions.unstable_createNodeManifest({
@@ -108,4 +112,12 @@ exports.createPages = ({ actions }) => {
     path: `three-alternative`,
     component: require.resolve(`./src/templates/three.js`),
   })
+
+  actions.createPage({
+    path: `slug-test-path`,
+    context: {
+      slug: `test-slug`,
+    },
+    component: require.resolve(`./src/templates/four.js`),
+  })
 }
diff --git a/integration-tests/node-manifest/src/templates/four.js b/integration-tests/node-manifest/src/templates/four.js
@@ -0,0 +1,17 @@
+import { graphql } from "gatsby"
+import React from "react"
+
+export default function Four({ data }) {
+  return <div>Template 4. Node by slug {data.testNode.slug}</div>
+}
+
+export const query = graphql`
+  query SLUG_TEST($slug: String) {
+    testNode(slug: { eq: $slug }) {
+      id
+    }
+    otherNode: testNode(id: { eq: "2" }) {
+      id
+    }
+  }
+`
diff --git a/packages/gatsby-cli/src/structured-errors/error-map.ts b/packages/gatsby-cli/src/structured-errors/error-map.ts
@@ -627,31 +627,37 @@ const errors = {
 
   /** Node Manifest warnings */
   "11801": {
-    // @todo add docs link to "using Preview" once it's updated with an explanation of ownerNodeId
     text: ({ inputManifest }): string => `${getSharedNodeManifestWarning(
       inputManifest
     )} but Gatsby couldn't find a page for this node.
-      If you want a manifest to be created for this node (for previews or other purposes), ensure that a page was created (and that a ownerNodeId is added to createPage() if you're not using the Filesystem Route API).\n`,
+      If you want a manifest to be created for this node (for previews or other purposes), ensure that a page was created (and that a ownerNodeId is added to createPage() if you're not using the Filesystem Route API). See https://www.gatsbyjs.com/docs/conceptual/content-sync for more info.\n`,
     level: Level.WARNING,
     category: ErrorCategory.USER,
   },
 
   "11802": {
-    // @todo add docs link to "using Preview" once it's updated with an explanation of ownerNodeId
     text: ({ inputManifest, pagePath }): string =>
       `${getSharedNodeManifestWarning(
         inputManifest
-      )} but Gatsby didn't find a ownerNodeId for the page at ${pagePath}\nUsing the first page that was found with the node manifest id set in pageContext.id in createPage().\nThis may result in an inaccurate node manifest (for previews or other purposes).`,
+      )} but Gatsby didn't find an ownerNodeId for the page at ${pagePath}\nUsing the first page that was found with the node manifest id set in pageContext.id in createPage().\nThis may result in an inaccurate node manifest (for previews or other purposes). See https://www.gatsbyjs.com/docs/conceptual/content-sync for more info.`,
+    level: Level.WARNING,
+    category: ErrorCategory.USER,
+  },
+
+  "11805": {
+    text: ({ inputManifest, pagePath }): string =>
+      `${getSharedNodeManifestWarning(
+        inputManifest
+      )} but Gatsby didn't find an ownerNodeId for the page at ${pagePath}\nUsing the first page that was found with the node manifest id set in pageContext.slug in createPage().\nThis may result in an inaccurate node manifest (for previews or other purposes). See https://www.gatsbyjs.com/docs/conceptual/content-sync for more info.`,
     level: Level.WARNING,
     category: ErrorCategory.USER,
   },
 
   "11803": {
-    // @todo add docs link to "using Preview" once it's updated with an explanation of ownerNodeId
     text: ({ inputManifest, pagePath }): string =>
       `${getSharedNodeManifestWarning(
         inputManifest
-      )} but Gatsby didn't find a ownerNodeId for the page at ${pagePath}\nUsing the first page where this node is queried.\nThis may result in an inaccurate node manifest (for previews or other purposes).`,
+      )} but Gatsby didn't find an ownerNodeId for the page at ${pagePath}\nUsing the first page where this node is queried.\nThis may result in an inaccurate node manifest (for previews or other purposes). See https://www.gatsbyjs.com/docs/conceptual/content-sync for more info.`,
     level: Level.WARNING,
     category: ErrorCategory.USER,
   },
diff --git a/packages/gatsby-plugin-gatsby-cloud/src/utils/use-poll-for-node-manifest/types.ts b/packages/gatsby-plugin-gatsby-cloud/src/utils/use-poll-for-node-manifest/types.ts
@@ -12,6 +12,7 @@ type FoundPageBy =
   | `ownerNodeId`
   | `filesystem-route-api`
   | `context.id`
+  | `context.slug`
   | `queryTracking`
   | `none`
 
diff --git a/packages/gatsby/src/utils/node-manifest.ts b/packages/gatsby/src/utils/node-manifest.ts
@@ -28,11 +28,12 @@ type FoundPageBy =
   | `filesystem-route-api`
   // for these three we warn to use ownerNodeId instead
   | `context.id`
+  | `context.slug`
   | `queryTracking`
   | `none`
 
 /**
- * Finds a final built page by nodeId
+ * Finds a final built page by nodeId or by node.slug as a fallback.
  *
  * Note that this function wont work properly in `gatsby develop`
  * since develop no longer runs all page queries when creating pages.
@@ -41,7 +42,13 @@ type FoundPageBy =
  * When this fn is being used for routing to previews the user wont necessarily have
  * visited the page in the browser yet.
  */
-async function findPageOwnedByNodeId({ nodeId }: { nodeId: string }): Promise<{
+async function findPageOwnedByNode({
+  nodeId,
+  slug,
+}: {
+  nodeId: string
+  slug?: string
+}): Promise<{
   page: INodeManifestPage
   foundPageBy: FoundPageBy
 }> {
@@ -78,10 +85,10 @@ async function findPageOwnedByNodeId({ nodeId }: { nodeId: string }): Promise<{
       // if we haven't found a page with this nodeId
       // set as page.ownerNodeId then run this logic.
       // this condition is on foundOwnerNodeId instead of ownerPagePath
-      // in case we find a page with the nodeId in page.context.id
+      // in case we find a page with the nodeId in page.context.id/context.slug
       // and then later in the loop there's a page with this nodeId
       // set on page.ownerNodeId.
-      // We always want to prefer ownerPagePath over context.id
+      // We always want to prefer ownerPagePath over context.id/context.slug
       if (foundOwnerNodeId) {
         break
       }
@@ -98,7 +105,10 @@ async function findPageOwnedByNodeId({ nodeId }: { nodeId: string }): Promise<{
 
       foundOwnerNodeId = fullPage?.ownerNodeId === nodeId
 
-      const foundPageIdInContext = fullPage?.context.id === nodeId
+      const foundPageIdInContext = fullPage?.context?.id === nodeId
+
+      // querying by node.slug in GraphQL queries is common enough that we can search for it as a fallback after ownerNodeId, filesystem routes, and context.id
+      const foundPageSlugInContext = slug && fullPage?.context?.slug === slug
 
       if (foundOwnerNodeId) {
         foundPageBy = `ownerNodeId`
@@ -113,6 +123,8 @@ async function findPageOwnedByNodeId({ nodeId }: { nodeId: string }): Promise<{
         foundPageBy = pageCreatedByFilesystemPlugin
           ? `filesystem-route-api`
           : `context.id`
+      } else if (foundPageSlugInContext && fullPage) {
+        foundPageBy = `context.slug`
       }
 
       if (
@@ -128,7 +140,8 @@ async function findPageOwnedByNodeId({ nodeId }: { nodeId: string }): Promise<{
           // that's mapped to this node id.
           // this also makes this work with the filesystem Route API without
           // changing that API.
-          foundPageIdInContext)
+          foundPageIdInContext ||
+          foundPageSlugInContext)
       ) {
         // save this path to use in our manifest!
         ownerPagePath = fullPage.path
@@ -153,6 +166,7 @@ async function findPageOwnedByNodeId({ nodeId }: { nodeId: string }): Promise<{
 export const foundPageByToLogIds = {
   none: `11801`,
   [`context.id`]: `11802`,
+  [`context.slug`]: `11805`,
   queryTracking: `11803`,
   [`filesystem-route-api`]: `success`,
   ownerNodeId: `success`,
@@ -177,6 +191,7 @@ export function warnAboutNodeManifestMappingProblems({
   switch (foundPageBy) {
     case `none`:
     case `context.id`:
+    case `context.slug`:
     case `queryTracking`: {
       logId = foundPageByToLogIds[foundPageBy]
       if (verbose) {
@@ -236,8 +251,10 @@ export async function processNodeManifest(
   }
 
   // map the node to a page that was created
-  const { page: nodeManifestPage, foundPageBy } = await findPageOwnedByNodeId({
+  const { page: nodeManifestPage, foundPageBy } = await findPageOwnedByNode({
     nodeId,
+    // querying by node.slug in GraphQL queries is common enough that we can search for it as a fallback after ownerNodeId, filesystem routes, and context.id
+    slug: fullNode?.slug as string,
   })
 
   const nodeManifestMappingProblemsContext = {
