Skip to content

Commit

Permalink
Match node manifest pages by page context slug
Browse files Browse the repository at this point in the history
  • Loading branch information
TylerBarnes committed Feb 11, 2022
1 parent d846f89 commit 506db86
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 10 deletions.
Expand Up @@ -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.
Expand Down
14 changes: 13 additions & 1 deletion integration-tests/node-manifest/gatsby-node.js
Expand Up @@ -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: {
Expand All @@ -13,6 +13,10 @@ exports.sourceNodes = ({ actions }) => {
},
}

if (id === 5) {
node.slug = `test-slug`
}

actions.createNode(node)

actions.unstable_createNodeManifest({
Expand Down Expand Up @@ -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`),
})
}
17 changes: 17 additions & 0 deletions 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
}
}
`
14 changes: 12 additions & 2 deletions packages/gatsby-cli/src/structured-errors/error-map.ts
Expand Up @@ -641,7 +641,17 @@ const errors = {
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).`,
level: Level.WARNING,
category: ErrorCategory.USER,
},

"11805": {
// @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 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).`,
level: Level.WARNING,
category: ErrorCategory.USER,
},
Expand All @@ -651,7 +661,7 @@ const errors = {
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).`,
level: Level.WARNING,
category: ErrorCategory.USER,
},
Expand Down
Expand Up @@ -12,6 +12,7 @@ type FoundPageBy =
| `ownerNodeId`
| `filesystem-route-api`
| `context.id`
| `context.slug`
| `queryTracking`
| `none`

Expand Down
31 changes: 24 additions & 7 deletions packages/gatsby/src/utils/node-manifest.ts
Expand Up @@ -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.
Expand All @@ -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
}> {
Expand Down Expand Up @@ -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
}
Expand All @@ -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`
Expand All @@ -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 (
Expand All @@ -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
Expand All @@ -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`,
Expand All @@ -177,6 +191,7 @@ export function warnAboutNodeManifestMappingProblems({
switch (foundPageBy) {
case `none`:
case `context.id`:
case `context.slug`:
case `queryTracking`: {
logId = foundPageByToLogIds[foundPageBy]
if (verbose) {
Expand Down Expand Up @@ -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 = {
Expand Down

0 comments on commit 506db86

Please sign in to comment.