Skip to content

Conversation

@pieh
Copy link
Contributor

@pieh pieh commented Jul 15, 2022

Description

This is to properly support cases when there is global/generic/shared elements and specific pages/templates want to override it with children setup:

// somewhere in shared components
function SharedSeo({ children }) {
  return <>
    // `id` is important for deduplication
    <link id="icon" rel="icon" href="X" />
    {children}
  </>
}
// in page template
export function Head() {
  return <SharedSeo>
    <link id="icon" rel="icon" href="icon-specific-for-this-page" />
  </SharedSeo>
}

TODO:

Documentation

This is currently listed as limitation in https://github.com/gatsbyjs/gatsby/blob/docs/gatsby-head/docs/docs/reference/built-in-components/gatsby-head.md / #3612, which will need to be adjusted when this is merged

[ch53232]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 15, 2022
@pieh pieh removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 15, 2022
imjoshin
imjoshin previously approved these changes Jul 15, 2022
…n-export/deduplication.js

Co-authored-by: Jude Agboola <[email protected]>
validHeadNodes.push(element)
seenIds.set(id, validHeadNodes.length - 1)
} else {
const indexOfPreviouslyInsertedNode = seenIds.get(id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the browser version include validHeadNodes[indexOfPreviouslyInsertedNode].remove() but SSR doesn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.remove in browser is "just in case", we likely can drop it. SSR version is not rendering elements, it's rendering html strings, so it doesn't have potential DOM element to clean up - at most some no longere referenced strings/objects that can be GCed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that makes sense, saw all the other lines seemed duplicated 1 for 1 so wanted to double check. Thanks!

@LekoArts LekoArts merged commit b08ef18 into master Jul 18, 2022
@LekoArts LekoArts deleted the feat/metadata/id-deduplication branch July 18, 2022 08:50
LekoArts pushed a commit that referenced this pull request Jul 18, 2022
* 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 <[email protected]>

* 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 <[email protected]>
(cherry picked from commit b08ef18)
LekoArts pushed a commit that referenced this pull request Jul 18, 2022
)

* 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 <[email protected]>

* 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 <[email protected]>
(cherry picked from commit b08ef18)

Co-authored-by: Michal Piechowiak <[email protected]>
@LekoArts LekoArts added this to the Metadata Management milestone Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants