From 844fd4561d7dc8964137f4a0be368420ea152d34 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Mon, 18 Jul 2022 10:50:57 +0200 Subject: [PATCH] feat(gatsby): allow deduplicating head elements on `id` (#36138) * feat: deduplicate head elements on id attrbibute in browser * feat: deduplicate head elements on id attrbibute in html gen * page with head deduplication * add test * update comments to match current code * Update e2e-tests/development-runtime/cypress/integration/head-function-export/deduplication.js Co-authored-by: Jude Agboola * Update e2e-tests/development-runtime/cypress/integration/head-function-export/deduplication.js * add test case to e2e-production * add test case to head integration tests Co-authored-by: Jude Agboola (cherry picked from commit b08ef185dc80c21bc0bb08b1cbe1deb7063cb6b0) --- .../head-function-export/deduplication.js | 19 ++++++++++ .../shared-data/head-function-export.js | 1 + .../head-function-export/deduplication.js | 35 +++++++++++++++++++ .../head-function-export/deduplication.js | 19 ++++++++++ .../shared-data/head-function-export.js | 1 + .../head-function-export/deduplication.js | 35 +++++++++++++++++++ .../__tests__/ssr-html-output.js | 19 ++++++++++ .../head-function-export/package.json | 1 + .../shared-data/head-function-export.js | 1 + .../head-function-export/deduplication.js | 35 +++++++++++++++++++ .../head/head-export-handler-for-browser.js | 15 +++++++- .../head/head-export-handler-for-ssr.js | 15 ++++++-- 12 files changed, 193 insertions(+), 3 deletions(-) create mode 100644 e2e-tests/development-runtime/cypress/integration/head-function-export/deduplication.js create mode 100644 e2e-tests/development-runtime/src/pages/head-function-export/deduplication.js create mode 100644 e2e-tests/production-runtime/cypress/integration/head-function-export/deduplication.js create mode 100644 e2e-tests/production-runtime/src/pages/head-function-export/deduplication.js create mode 100644 integration-tests/head-function-export/src/pages/head-function-export/deduplication.js diff --git a/e2e-tests/development-runtime/cypress/integration/head-function-export/deduplication.js b/e2e-tests/development-runtime/cypress/integration/head-function-export/deduplication.js new file mode 100644 index 0000000000000..1c6efdae12c39 --- /dev/null +++ b/e2e-tests/development-runtime/cypress/integration/head-function-export/deduplication.js @@ -0,0 +1,19 @@ +import headFunctionExportSharedData from "../../../shared-data/head-function-export.js" + +it(`Deduplicates multiple tags with same id`, () => { + cy.visit(headFunctionExportSharedData.page.deduplication).waitForRouteChange() + + // deduplication link has id and should be deduplicated + cy.get(`link[rel=deduplication]`).should("have.length", 1) + // last deduplication link should win + cy.get(`link[rel=deduplication]`).should("have.attr", "href", "/bar") + // we should preserve id + cy.get(`link[rel=deduplication]`).should( + "have.attr", + "id", + "deduplication-test" + ) + + // alternate links are not using id, so should have multiple instances + cy.get(`link[rel=alternate]`).should("have.length", 2) +}) diff --git a/e2e-tests/development-runtime/shared-data/head-function-export.js b/e2e-tests/development-runtime/shared-data/head-function-export.js index 30ff4de8f5e12..c40d4272a2c01 100644 --- a/e2e-tests/development-runtime/shared-data/head-function-export.js +++ b/e2e-tests/development-runtime/shared-data/head-function-export.js @@ -11,6 +11,7 @@ const page = { ssr: `${path}/ssr/`, invalidElements: `${path}/invalid-elements/`, fsRouteApi: `${path}/fs-route-api/`, + deduplication: `${path}/deduplication/`, } const data = { diff --git a/e2e-tests/development-runtime/src/pages/head-function-export/deduplication.js b/e2e-tests/development-runtime/src/pages/head-function-export/deduplication.js new file mode 100644 index 0000000000000..53302923a3462 --- /dev/null +++ b/e2e-tests/development-runtime/src/pages/head-function-export/deduplication.js @@ -0,0 +1,35 @@ +import * as React from "react" + +export default function HeadFunctionDeduplication() { + return ( + <> +

+ I deduplicated Head elements by their id +

+ + ) +} + +function SEO({ children }) { + return ( + <> + + + {children} + + ) +} + +export function Head() { + return ( + + + + + ) +} diff --git a/e2e-tests/production-runtime/cypress/integration/head-function-export/deduplication.js b/e2e-tests/production-runtime/cypress/integration/head-function-export/deduplication.js new file mode 100644 index 0000000000000..1c6efdae12c39 --- /dev/null +++ b/e2e-tests/production-runtime/cypress/integration/head-function-export/deduplication.js @@ -0,0 +1,19 @@ +import headFunctionExportSharedData from "../../../shared-data/head-function-export.js" + +it(`Deduplicates multiple tags with same id`, () => { + cy.visit(headFunctionExportSharedData.page.deduplication).waitForRouteChange() + + // deduplication link has id and should be deduplicated + cy.get(`link[rel=deduplication]`).should("have.length", 1) + // last deduplication link should win + cy.get(`link[rel=deduplication]`).should("have.attr", "href", "/bar") + // we should preserve id + cy.get(`link[rel=deduplication]`).should( + "have.attr", + "id", + "deduplication-test" + ) + + // alternate links are not using id, so should have multiple instances + cy.get(`link[rel=alternate]`).should("have.length", 2) +}) diff --git a/e2e-tests/production-runtime/shared-data/head-function-export.js b/e2e-tests/production-runtime/shared-data/head-function-export.js index 523fbc1b62ed7..a53b7edf8ee3a 100644 --- a/e2e-tests/production-runtime/shared-data/head-function-export.js +++ b/e2e-tests/production-runtime/shared-data/head-function-export.js @@ -11,6 +11,7 @@ const page = { ssr: `${path}/ssr/`, invalidElements: `${path}/invalid-elements/`, fsRouteApi: `${path}/fs-route-api/`, + deduplication: `${path}/deduplication/`, } const data = { diff --git a/e2e-tests/production-runtime/src/pages/head-function-export/deduplication.js b/e2e-tests/production-runtime/src/pages/head-function-export/deduplication.js new file mode 100644 index 0000000000000..53302923a3462 --- /dev/null +++ b/e2e-tests/production-runtime/src/pages/head-function-export/deduplication.js @@ -0,0 +1,35 @@ +import * as React from "react" + +export default function HeadFunctionDeduplication() { + return ( + <> +

+ I deduplicated Head elements by their id +

+ + ) +} + +function SEO({ children }) { + return ( + <> + + + {children} + + ) +} + +export function Head() { + return ( + + + + + ) +} diff --git a/integration-tests/head-function-export/__tests__/ssr-html-output.js b/integration-tests/head-function-export/__tests__/ssr-html-output.js index 1354d6a62b2d5..74ab79ac30afb 100644 --- a/integration-tests/head-function-export/__tests__/ssr-html-output.js +++ b/integration-tests/head-function-export/__tests__/ssr-html-output.js @@ -76,4 +76,23 @@ describe(`Head function export SSR'ed HTML output`, () => { expect(style.text).toContain(data.queried.style) expect(link.attributes.href).toEqual(data.queried.link) }) + + it(`deduplicates multiple tags with same id`, () => { + const html = readFileSync(`${publicDir}${page.deduplication}/index.html`) + const dom = parse(html) + + // deduplication link has id and should be deduplicated + expect(dom.querySelectorAll(`link[rel=deduplication]`)?.length).toEqual(1) + // last deduplication link should win + expect( + dom.querySelector(`link[rel=deduplication]`)?.attributes?.href + ).toEqual("/bar") + // we should preserve id + expect( + dom.querySelector(`link[rel=deduplication]`)?.attributes?.id + ).toEqual("deduplication-test") + + // alternate links are not using id, so should have multiple instances + expect(dom.querySelectorAll(`link[rel=alternate]`)?.length).toEqual(2) + }) }) diff --git a/integration-tests/head-function-export/package.json b/integration-tests/head-function-export/package.json index 112e6bb1e8b45..3005b8f69b4ff 100644 --- a/integration-tests/head-function-export/package.json +++ b/integration-tests/head-function-export/package.json @@ -19,6 +19,7 @@ "babel-preset-gatsby-package": "^2.4.0", "fs-extra": "^10.0.0", "jest": "^27.2.1", + "node-html-parser": "^5.3.3", "npm-run-all": "4.1.5" }, "dependencies": { diff --git a/integration-tests/head-function-export/shared-data/head-function-export.js b/integration-tests/head-function-export/shared-data/head-function-export.js index a6bfc266ead36..6945cdda70994 100644 --- a/integration-tests/head-function-export/shared-data/head-function-export.js +++ b/integration-tests/head-function-export/shared-data/head-function-export.js @@ -7,6 +7,7 @@ const page = { staticQuery: `${path}/static-query-component/`, warnings: `${path}/warnings/`, allProps: `${path}/all-props/`, + deduplication: `${path}/deduplication/`, } const data = { diff --git a/integration-tests/head-function-export/src/pages/head-function-export/deduplication.js b/integration-tests/head-function-export/src/pages/head-function-export/deduplication.js new file mode 100644 index 0000000000000..53302923a3462 --- /dev/null +++ b/integration-tests/head-function-export/src/pages/head-function-export/deduplication.js @@ -0,0 +1,35 @@ +import * as React from "react" + +export default function HeadFunctionDeduplication() { + return ( + <> +

+ I deduplicated Head elements by their id +

+ + ) +} + +function SEO({ children }) { + return ( + <> + + + {children} + + ) +} + +export function Head() { + return ( + + + + + ) +} diff --git a/packages/gatsby/cache-dir/head/head-export-handler-for-browser.js b/packages/gatsby/cache-dir/head/head-export-handler-for-browser.js index 758e6011e1ada..d4113e15e7ff2 100644 --- a/packages/gatsby/cache-dir/head/head-export-handler-for-browser.js +++ b/packages/gatsby/cache-dir/head/head-export-handler-for-browser.js @@ -22,15 +22,28 @@ const onHeadRendered = () => { removePrevHeadElements() + const seenIds = new Map() for (const node of hiddenRoot.childNodes) { const nodeName = node.nodeName.toLowerCase() + const id = node.attributes.id?.value if (!VALID_NODE_NAMES.includes(nodeName)) { warnForInvalidTags(nodeName) } else { const clonedNode = node.cloneNode(true) clonedNode.setAttribute(`data-gatsby-head`, true) - validHeadNodes.push(clonedNode) + if (id) { + if (!seenIds.has(id)) { + validHeadNodes.push(clonedNode) + seenIds.set(id, validHeadNodes.length - 1) + } else { + const indexOfPreviouslyInsertedNode = seenIds.get(id) + validHeadNodes[indexOfPreviouslyInsertedNode].remove() + validHeadNodes[indexOfPreviouslyInsertedNode] = clonedNode + } + } else { + validHeadNodes.push(clonedNode) + } } } diff --git a/packages/gatsby/cache-dir/head/head-export-handler-for-ssr.js b/packages/gatsby/cache-dir/head/head-export-handler-for-ssr.js index 0aa7d53e5dac7..2003321cf71cb 100644 --- a/packages/gatsby/cache-dir/head/head-export-handler-for-ssr.js +++ b/packages/gatsby/cache-dir/head/head-export-handler-for-ssr.js @@ -54,8 +54,10 @@ export function headHandlerForSSR({ const validHeadNodes = [] + const seenIds = new Map() for (const node of headNodes) { const { rawTagName, attributes } = node + const id = attributes.id if (!VALID_NODE_NAMES.includes(rawTagName)) { warnForInvalidTags(rawTagName) @@ -68,8 +70,17 @@ export function headHandlerForSSR({ }, node.childNodes[0]?.textContent ) - - validHeadNodes.push(element) + if (id) { + if (!seenIds.has(id)) { + validHeadNodes.push(element) + seenIds.set(id, validHeadNodes.length - 1) + } else { + const indexOfPreviouslyInsertedNode = seenIds.get(id) + validHeadNodes[indexOfPreviouslyInsertedNode] = element + } + } else { + validHeadNodes.push(element) + } } }