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

Maintain key compatibility with React (prepend .$) #3403

Open
adhirajsinghchauhan opened this issue Jan 9, 2022 · 6 comments
Open

Maintain key compatibility with React (prepend .$) #3403

adhirajsinghchauhan opened this issue Jan 9, 2022 · 6 comments

Comments

@adhirajsinghchauhan
Copy link

adhirajsinghchauhan commented Jan 9, 2022

Describe the feature you'd love to see
Copy-pasting my findings from vercel/next.js#32944 (comment): 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)
preactjs/next-plugin-preact#51 is the same issue; not sure if it's the correct repo though. Which is why I've created this one.

@JoviDeCroock
Copy link
Member

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 😃

@adhirajsinghchauhan
Copy link
Author

adhirajsinghchauhan commented Jan 9, 2022

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:

preact/src/diff/children.js

Lines 268 to 279 in 4f08ae4

export function toChildArray(children, out) {
out = out || [];
if (children == null || typeof children == 'boolean') {
} else if (Array.isArray(children)) {
children.some(child => {
toChildArray(child, out);
});
} else {
out.push(children);
}
return out;
}

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.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Jan 9, 2022

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 😅

@adhirajsinghchauhan
Copy link
Author

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.

@selenecodes
Copy link

selenecodes commented Sep 3, 2022

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

@Cobertos
Copy link

Cobertos commented Oct 25, 2022

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}`} />

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants