-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Conversation
e2e-tests/development-runtime/cypress/integration/head-function-export/deduplication.js
Outdated
Show resolved
Hide resolved
e2e-tests/development-runtime/cypress/integration/head-function-export/deduplication.js
Show resolved
Hide resolved
…n-export/deduplication.js Co-authored-by: Jude Agboola <marvinjudehk@gmail.com>
e2e-tests/development-runtime/cypress/integration/head-function-export/deduplication.js
Outdated
Show resolved
Hide resolved
validHeadNodes.push(element) | ||
seenIds.set(id, validHeadNodes.length - 1) | ||
} else { | ||
const indexOfPreviouslyInsertedNode = seenIds.get(id) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
* 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)
) * 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>
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:TODO:
e2e-tests/production-runtime
( https://github.com/gatsbyjs/gatsby/pull/36138/files/3157d104ca1d3b9796f6dd652ab4c4bfef60e40f#diff-b719bec3cafff0f512dc6daff16ee4b0d94965be3bdcd3d2f97fd908e8d3ef3a )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]