-
Notifications
You must be signed in to change notification settings - Fork 34
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
RFC: Managing tags without extra markup #97
Comments
I think I'm a fan of Option 2. What are your thoughts @CanRau @TrySound @rdsedmundo @TrejoCode @JoshuaToenyes ? Any other options you can think of? Would love your thoughts! EDIT: I added an Option 4 above, the nuclear option 💣 😁 |
For my use, option 2 favors me and in general, it seems like a simple solution to implement. |
Very nicely described. Here are my thoughts on each option: Option 1 - Manually created whitelist Specifically on the "cons" list:
In general, I like the explicit approach Option 1 provides. Option 2 - Auto-Whitelist
After being enlightened by @CanRau, (and reading the React Helmet issue linked above more carefully), I'm revising my opinion of option 2. This way ahead could actually be the best approach. One possible solution to the overhead of including the tag's content would be to perform a very simple hash on the tag and/or attribute values. We use this algorithm for string hashing on both client and server (although we usually convert it to a string, like
On the client side, it would simply re-hash each tag checking for a match. If it matches, it's managed. To make debugging easier, it would make sense to add an attribute or comment someplace indicating which tags are managed when One other note... should the Option 3 - Individually opt-in whitelist I don't prefer this option since it requires "global" thinking at each component. If I'm writing a component that modifies the head tags, I don't really want to be thinking about how those head tags will be rendered. In other words, I think the "whitelist" behavior should be abstracted away at the individual component level. Option 4 - Nuclear! I honestly like this approach:
There's one case where I can imagine this wouldn't work well: If you're building some hybrid React application where React is only responsible for some portion of the page and not the entire page. For example, if I have a WordPress site and embed some React powered widget that uses react-head to manage head tags of the outer page. In this case, if you enable the Proposal: Option 5 - Nuclear + Whitelist What if we had a hybrid Option 1 + Option 4 approach, with a In this case, it should be an error to specify both the I like this approach because I think it provides a way to solve the problems presented and is incremental: existing behavior is preserved, users may allow react head to manage some or all of the head tags. |
I've to add that that CSS like whitelist won't work like that at the moment and I don't know a simple solution to get it working as mentioned here #84 (comment) and possible workarounds here #84 (comment) I'm actually open to any of those 😁I like the use case of a fallback a lot as well 👍 Prop. 5: Nuke + whitelist sounds good but might be overkill to add both I don't know The @JoshuaToenyes just a side note, maybe you already know(?!), it's not only about beauty but fear of SEO issues due to some software not properly scraping meta tags with additional attributes, that's at least what many are talking about in the above linked react-helmet issue |
@CanRau Ah, I wasn't aware of the SEO concern or scraping problems on the data attributes. I will read through that issue more carefully! Our software powers quite a few sites (~100) most using react-head and a few running react-helmet. I haven't noticed an SEO impact using either... but I'm also not doing any detailed SEO analysis on head tags either... I would be very interested if there's any data on that. |
Me too actually, yet until that exists I think it’s desirable to take the safe route if possible 😉
|
Thank you for pointing that out @CanRau. I revised my opinion on Option 2 above :) |
I know it's christmas just wanted to check for any updates on this? |
Hey @CanRau thanks for following-up. Apologies but I haven't made progress on this, however, I think based on the discussion here we might at least have a path forward that I'd be open to a PR to add by someone, or I will try and get to myself if I get time. Anyway, here's the key factors of what I think to be the best approach:
I think a sort of super-hybrid of the Nuclear option and Auto-whitelist might work, sort of in line with the hybrid approach that @JoshuaToenyes suggested but combined to a single prop interface. Let me know what you think: <HeadProvider headTags={headTags} manageTags={option}> Where
What do you all think? Is this interface confusing? Does it cover all bases? |
Think that looks good 👍Not sure when I can find time to look into it though 😿
|
Yea, agreed, “types” is kind of a weird name. Open to other suggestions as well... |
Ran into this issue at my company - I'm glad to help but not sure what's the best avenue. For now planning to just wipe out the existing head tags on JS load to simulate the Thought for the "inferred"/"types" option (maybe If a direction can be set then would be glad to try to contribute an implementation. Thanks! |
Thanks for your interest, @osdiab ! Please feel free to submit a PR on this if you're up for it.
I'm fine with
<HeadProvider headTags={headTags} manageTags="infer">
<App>
// ... within any component, head tags are rendered as normal
<Meta name="blah-long-name" content="lots of content here" />
<Title>Title of page</Title>
</App>
</HeadProvider> And your HTML template looked something like this: <head>
<title>Fallback title</title>
<link rel="stylesheet" type="text/css" href="theme.css">
</head> Then the resultant tags on the page, after client rehydration would look like this: <head>
<meta name="blah-long-name" content="lots of content here" />
<title>Title of page</title>
<link rel="stylesheet" type="text/css" href="theme.css">
</head> Note that Let me know if that's still unclear. Thanks! |
I need to help :( |
Problem
Currently, react-head utilizes
data-rh
attributes in order to manage the tags that it creates. By default, react-head avoids touching tags that it doesn't know about (that don't havedata-rh
attributes). This is so that static tags that are part of a static HTML template are not touched.There's been a few issues (both in this project and react-helmet) from folks who are interested in:
While these two items are certainly separate, they are closely related and we might be able to kill two birds with one stone depending on our approach.
Potential Solutions
Option 1: Manually created whitelist definition
One solution, as proposed in #84 is to define an explicit whitelist prop:
This solution has the following benefits:
While this works, it has the following drawbacks:
[foo^=bar]
)Implementation Details
The implementation can be explored in the WIP PR by @CanRau
Option 2: Auto-Whitelist
Expanding on Option 1, we could auto-generate the necessary "selector set" as you go (opt-in to this behavior with
autoWhitelist
prop onHeadProvider
):Implementation Details
On the server, as we render head tags, if any of them include a
whitelist
prop, we build up the selector set necessary to identify which tags were inserted by react-head (basically the selector set that was manually created in Option 1 above). We return this as a renderable DOM element as part of theheadTags[]
array with some unique ID that we grab from the DOM and provide as context during client rendering:CONs:
meta[name^="blah"][content^="lots"]
PROs:
title
, instead of also including the full text content). Alternatively we could force opt-in to mangling by requiring an additional specific prop,<title mangle>Title of page</title>
but not crazy about that idea.Option 3: Individually opt-in to whitelist
As a tweak to Option 2, we could individually require opt-in, if we wanted to support whitelisting only individual elements, we could support an explicit
whitelist
prop on each head tag instead of anautoWhitelist
prop on theHeadProvider
.In this scenario you'd have a mix of
data-rh
and#react-head-whitelist
selectors.Not sure how popular this option would be and it complicates the implementation slightly so I'd almost rather have it be all or nothing. Thoughts?
PRO:
CON:
Option 4: The Nuclear Option
The above assume that folks don't want react-head to manage tags that it hasn't explicitly rendered. There is a "nuclear" option of allowing folks to opt-in to having react-head mangle all tags:
If
manageAllTags
is set, we can avoid renderingdata-rh
attributes and just have a very greedy selector on the client, something like:head > title, head > meta
... which would select all tags in<head />
whether or not they were created by react-head. This would work but would require that users express all their tags in the React tree and not just rely on static HTML templates. This is okay if we make this opt-in and explain thoroughly in the documentation.PROs:
CONs:
The text was updated successfully, but these errors were encountered: