Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from Jul 18, 2022

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 <marvinjudehk@gmail.com>
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 <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)
@LekoArts LekoArts added this to Backport PR opened in Release candidate Jul 18, 2022
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 <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>
@LekoArts LekoArts moved this from Backport PR opened to Backported in Release candidate Jul 18, 2022
@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
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants