From ba8e21c32b9acb4c209e1dd7cffbf8bff4da58dd Mon Sep 17 00:00:00 2001 From: Tyler Barnes Date: Fri, 11 Feb 2022 12:20:12 -0800 Subject: [PATCH] feat(gatsby): Match node manifest pages by page context slug (#34790) * Match node manifest pages by page context slug * add docs link --- .../__tests__/create-node-manifest.test.js | 8 +++++ .../node-manifest/gatsby-node.js | 14 ++++++++- .../node-manifest/src/templates/four.js | 17 ++++++++++ .../src/structured-errors/error-map.ts | 18 +++++++---- .../utils/use-poll-for-node-manifest/types.ts | 1 + packages/gatsby/src/utils/node-manifest.ts | 31 ++++++++++++++----- 6 files changed, 75 insertions(+), 14 deletions(-) create mode 100644 integration-tests/node-manifest/src/templates/four.js diff --git a/integration-tests/node-manifest/__tests__/create-node-manifest.test.js b/integration-tests/node-manifest/__tests__/create-node-manifest.test.js index d9967cf1dbdf4..c9416bb78f896 100644 --- 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 index 6277c558dc425..8a9c03c34b46d 100644 --- 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 new file mode 100644 index 0000000000000..1874dfccbc20f --- /dev/null +++ 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
Template 4. Node by slug {data.testNode.slug}
+} + +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 index 6617a68ee2430..069bd571b48fb 100644 --- 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 index 9fe0cb282ed01..6948eae2ffc40 100644 --- 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 index 91298109b761b..1781f2ae4caee 100644 --- 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 = {