-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
fix(core): allow overriding ssr/dev template meta tags #7952
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
Size Change: +494 B (0%) Total Size: 814 kB
ℹ️ View Unchanged
|
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.
I like this!
I think the same. I think it's possible to promote certain tags in react-helmet? |
The problem is our plugin lifecycle uses a totally different code path, using plain strings and not injectHtmlTags?: (args: {content: Content}) => {
headTags?: HtmlTags;
preBodyTags?: HtmlTags;
postBodyTags?: HtmlTags;
}; For example all the PWA plugin metadata also can't be overridden (no "data-rh" attribute) |
Important note: apparently the order has an impact on SEO: https://github.com/staylor/react-helmet-async#prioritizing-tags-for-seo "generator" meta is not in the list (PR staylor/react-helmet-async#180) |
Ah I see. That's not a bad thing to do—smaller JS bundle. Maybe we can keep |
I'd like later to provide extra tooling on top of Docusaurus to make it even easier, in which case overriding the generator might be a use-case For example a simple CLI command |
👍 Should definitely work as well, but that's maybe a separate issue (if not created already) since it already doesn't work? And not a blocking issue for this PR which is more realistic to get implemented? I assume that will be a larger rewrite and should it require a different approach for
Some users might also want to hide it. Not familiar with security risks in SSR, but company security policies may require it - especially version. |
Going to keep "generator" (see staylor/react-helmet-async#180) and "charSet" (see staylor/react-helmet-async#181) outside of Helmet system for now |
Also reverting the usage of see https://github.com/facebook/docusaurus/runs/7917709859?check_suite_focus=true |
Also found out some limitations when using This leads to these metas being automatically removed (if not found in For now, let's keep it simple: charSet and generator won't be overrideable, but viewport and robots will be |
working as I want it to be for now 👍 |
Wow, that was fast. Thanks! 🙂 |
Motivation
Related to #7944
Move
robots
andviewport
meta to<Head>
so that we can more easily override themKeep
charSet
andgenerator
in the static templates for now, until a solution is found (we want them at the very top)Test Plan
Manual 😅 Inspecting preview metadata, comparing to prod
Test page for overrides: https://deploy-preview-7952--docusaurus-2.netlify.app/tests/pages/head-metadata/
Test links
Deploy preview:
Former solution attempt (moving everything to use Head)
Our eta (prod) / ejs (dev) templates have metadata:
But it's not possible to override it using react-helmet-async (and thus not either using
themeConfig.metadata
because those are static.Apparently our
<Head>
component is capable to de-duplicate meta tags with the same namePossible alternative: keep metadata in the template, but add
data-rh='true'
apparently allows react-helmet-async to kick in and update the existing tags instead of creating new ones. (see staylor/react-helmet-async#108)
For consistency that looks better to always use
<Head>
and move out 100% of those metadata out of the templates.Note: this works but I find this not very convenient to debug. I like the fact title, generator etc are at the very top, it makes it easy to inspect if a site is using Docusaurus or not. Maybe it's better to use the alternative solution?