preact icon indicating copy to clipboard operation
preact copied to clipboard

Maintain `key` compatibility with React (prepend `.$`)

Open adhirajsinghchauhan opened this issue 3 years ago • 5 comments

Describe the feature you'd love to see Copy-pasting my findings from https://github.com/vercel/next.js/issues/32944#issuecomment-1008280432: React "escapes" keys by prepending .$ while remapping children into an array. Relevant lines of code (facebook/react@a724a3b5/.../ReactChildren.js):

const SEPARATOR = '.'; // line 22
...

function escape(key: string) {
  ...
  return '$' + escapedString; // line 41
}

function getElementKey(element: any, index: number) {
    ...
    return escape('' + element.key); // line 71
    ...
}

function mapIntoArray() {
    ...
    const childKey =
      nameSoFar === '' ? SEPARATOR + getElementKey(child, 0) : nameSoFar; // line 116
    ...
}

React docs:

Note: React.Children.toArray() changes keys to preserve the semantics of nested arrays when flattening lists of children. That is, toArray prefixes each key in the returned array so that each element’s key is scoped to the input array containing it.

preact/compat passes Children.toArray over to toChildArray from the same layer, but doesn't bother adjusting keys. This leads to situations like the above linked issue. NextJS features auto-dedupe based on keys, which is great for cross-page meta tags that need updation:

To avoid duplicate tags in your head you can use the key property, which will make sure the tag is only rendered once...only the second <meta property="og:title" /> is rendered. meta tags with duplicate key attributes are automatically handled.

Since preact/compat is meant to maintain compatibility with React "as much as possible", could this be implemented? It doesn't exactly fit in with project goals, but it doesn't go against it either.

If approved, I'd be happy to create a PR.

Additional context (optional) https://github.com/preactjs/next-plugin-preact/issues/51 is the same issue; not sure if it's the correct repo though. Which is why I've created this one.

adhirajsinghchauhan avatar Jan 09 '22 12:01 adhirajsinghchauhan

I wonder whether this is something we need to handle in render to string or when does this occur? Does this happen during csr or? Just trying to wrap my head around the meat of the issue, the goal would be to have a minimal amount of size but compatability is important to us for /compat 😃

JoviDeCroock avatar Jan 09 '22 12:01 JoviDeCroock

For the project I encountered this in, it happens during SSR and remains during CSR of course.

Here's the NextJS code looking for $, and L127 is the call to React.Children.toArray: https://github.com/vercel/next.js/blob/320986a2b897bc512c21f7148f4a4c8ce749dcae/packages/next/shared/lib/head.tsx#L67.

This is Preact's current passthrough for toArray: https://github.com/preactjs/preact/blob/4f08ae4d607964ae8222e910d3b209def75626b0/src/diff/children.js#L268-L279 Unless there are assumptions within Preact on key remaining the same as what was provided initially, I believe we could just do children.key = '.$' + children.key (with existence checks) before pushing.

EDIT: React claims keys are escaped to ensure safety for reactid usages (codedoc). Not sure how that logic works within Preact.

adhirajsinghchauhan avatar Jan 09 '22 12:01 adhirajsinghchauhan

I find the check a bit odd 😅 I guess it would be better to add this here but essentially I find the check for $ to be a bit odd but yes it's part of the API spec in there I guess. We don't use keys for internal identifiers we keep the raw key. I just fear about the "breaking" factor in incorporating this into core, even compat scares me a bit 😅

JoviDeCroock avatar Jan 09 '22 13:01 JoviDeCroock

fear about the "breaking" factor in incorporating this into core, even compat scares me a bit

Assuming you meant toArray: toChildArray.map(/* prepend to keys */), your idea of putting it in Children.js would certainly mitigate any potential for a "breaking change", in case someone's using the exported toChildArray directly (which as per docs, is meant to be used if manual iteration is needed). But there's a (minor?) performance cost: mapping requires reiteration.

Others who are using React.Children.toArray would expect it to behave exactly like React.

adhirajsinghchauhan avatar Jan 10 '22 04:01 adhirajsinghchauhan

Is there any update on this? Meta tags are currently still duplicated when using Nextjs with preact/compat.

selenecodes avatar Sep 03 '22 18:09 selenecodes

Prepending .$ onto the value of my keys properly deduped everything. So I guess this is a good workaround if you're running into the Nextjs + preact/compat issue.

Example:

<link rel='canonical' key=".$canonical" href={`${hostname}${canonicalPath}`} />

Cobertos avatar Oct 25 '22 03:10 Cobertos