feat(gatsby): allow deduplicating head elements on id (#36138) (#36159)

gatsbybot · pieh · web-flow · commit 6f0d46bffa64 · 2022-07-18T11:04:46.000+02:00
* 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 <marvinjudehk@gmail.com> * 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 <marvinjudehk@gmail.com> (cherry picked from commit b08ef18) Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
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
@@ -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
@@ -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
@@ -0,0 +1,35 @@
+import * as React from "react"
+
+export default function HeadFunctionDeduplication() {
+  return (
+    <>
+      <h1>
+        I deduplicated Head elements by their <code>id</code>
+      </h1>
+    </>
+  )
+}
+
+function SEO({ children }) {
+  return (
+    <>
+      <link rel="deduplication" id="deduplication-test" href="/foo" />
+      <link
+        rel="alternate"
+        type="application/atom+xml"
+        title="RSS Feed"
+        href="/blog/news/atom"
+      />
+      {children}
+    </>
+  )
+}
+
+export function Head() {
+  return (
+    <SEO>
+      <link rel="deduplication" id="deduplication-test" href="/bar" />
+      <link rel="alternate" hrefLang="de-DE" href="/de/" />
+    </SEO>
+  )
+}
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
@@ -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
@@ -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
@@ -0,0 +1,35 @@
+import * as React from "react"
+
+export default function HeadFunctionDeduplication() {
+  return (
+    <>
+      <h1>
+        I deduplicated Head elements by their <code>id</code>
+      </h1>
+    </>
+  )
+}
+
+function SEO({ children }) {
+  return (
+    <>
+      <link rel="deduplication" id="deduplication-test" href="/foo" />
+      <link
+        rel="alternate"
+        type="application/atom+xml"
+        title="RSS Feed"
+        href="/blog/news/atom"
+      />
+      {children}
+    </>
+  )
+}
+
+export function Head() {
+  return (
+    <SEO>
+      <link rel="deduplication" id="deduplication-test" href="/bar" />
+      <link rel="alternate" hrefLang="de-DE" href="/de/" />
+    </SEO>
+  )
+}
diff --git 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
@@ -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
@@ -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
@@ -0,0 +1,35 @@
+import * as React from "react"
+
+export default function HeadFunctionDeduplication() {
+  return (
+    <>
+      <h1>
+        I deduplicated Head elements by their <code>id</code>
+      </h1>
+    </>
+  )
+}
+
+function SEO({ children }) {
+  return (
+    <>
+      <link rel="deduplication" id="deduplication-test" href="/foo" />
+      <link
+        rel="alternate"
+        type="application/atom+xml"
+        title="RSS Feed"
+        href="/blog/news/atom"
+      />
+      {children}
+    </>
+  )
+}
+
+export function Head() {
+  return (
+    <SEO>
+      <link rel="deduplication" id="deduplication-test" href="/bar" />
+      <link rel="alternate" hrefLang="de-DE" href="/de/" />
+    </SEO>
+  )
+}
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
@@ -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
@@ -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)
+        }
       }
     }
 

Original file line number	Diff line number	Diff line change
`@@ -11,6 +11,7 @@ const page = {`
`11`	`11`	ssr: `${path}/ssr/`,
`12`	`12`	invalidElements: `${path}/invalid-elements/`,
`13`	`13`	fsRouteApi: `${path}/fs-route-api/`,
	`14`	+ deduplication: `${path}/deduplication/`,
`14`	`15`	`}`
`15`	`16`
`16`	`17`	`const data = {`
Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,7 @@ const page = {`
`7`	`7`	staticQuery: `${path}/static-query-component/`,
`8`	`8`	warnings: `${path}/warnings/`,
`9`	`9`	allProps: `${path}/all-props/`,
	`10`	+ deduplication: `${path}/deduplication/`,
`10`	`11`	`}`
`11`	`12`
`12`	`13`	`const data = {`