React keys are explicitly unoptimized for re-rendering #703
Comments
github-actions
bot
added
👋 phase/new
|
Definitely open to ideas on improving keying for faster re-renders. 👍 |
|
semi-related to #466 and some of the discussion there may be relevant |
|
Yes, using the index as a key is a well-known anti-pattern. The key should uniquely identify the data it is rendering inside the list. It’s about providing a unique stable identity in the array, not about making it as unique as possible. This allows React to prevent unmounting and mounting components causing unnecessary rerenders. Note that the current solution, and both alternative proposals all use the index. My proposal doesn’t literally use the array index, but it also relies on the array position. The current situation is very close to this pitfall
Using an index is an well known anti-pattern, because it’s simplist way to silence React warnings and most of the time there’s a a better solution, i.e. using an id. However, unist nodes don’t have ids. Unist nodes have two things that identify them: their type (or tag name for hast nodes) and their index. The positional information doesn’t make them more unique, because the index already always makes them different. Let’s say we make the following modification to the example above: # Title
This is a paragraph
+ which now spans multiple lines
This is another paragraphWith the current algorithm, this means every React element generated from the markdown below the added line receives a new key. This means React will remove all that content from the DOM, and generate new HTML elements to add to the DOM. With pure index-based keys or my proposal to use the nth occurrence of the tag name, React would only update the changed paragraph content in the DOM. Where my proposal to use the nth occurrence differs from pure index-based keys, is when elements are swapped, deleted, or inserted. # Title
+
+ 
This is a paragraph
-
- 
This is another paragraphIn this situation pure index-based keys would notice that the React element type of the 2nd and 3rd elements have changed, so it would delete those DOM nodes and create new ones. With my proposal, the keys for the image and paragraph would stay the same, meaning React will swap these existing nodes in the DOM. # Title
This is a paragraph
+
+ This paragraph is inserted
This is another paragraphWhen inserting an element, only elements of the same type are affected. In this example, pure index-based keys would trigger rerenders of all elements below the inserted content, so image element would be replaced with a new paragraph. The old last paragraph would be replaced with a new image element, and a new paragraph is created. <h1 key="0">Title</h1>
<p key="1">This is a paragraph</p>
- <img key="2" src="./image.png" /> {/* key 2 updated its tag name to `p` */}
- <p key="3">This is another paragraph</p> {/* key 2 updated its tag name to `img` */}
+ <p key="2">This paragraph is inserted<p>
+ <img key="3" src="./image.png" />
+ <p key="4">This is another paragraph<p> {/* key 4 is new */}With my proposal, React would see this as a swap of what was the last paragraph <h1 key="h1-0">Title</h1>
<p key="p-0">This is a paragraph</p>
- <img key="img-0" src="./image.png" /> {/* key img-0 was relocated, but not altered */}
- <p key="p-1">This is another paragraph</p> {/* key p-1 text contents changed */}
+ <p key="p-1">This paragraph is inserted<p>
+ <img key="img-0" src="./image.png" />
+ <p key="p-2">This is another paragraph<p> {/* key p-1 is new */} |
|
But how far should this be taken? Such as, we could also hash content, e.g., you could generate ids based on the first 3 and last 3 words or so? As far as I understand it, keys make sense for when you are generating elements with code, but not really as much when compiling markdown and similar like we’re doing? I’m very open to improve the situation, but because what the proper solution is vague, I’d like to see some performance results? |
|
Hi. Thank you for your hard work and providing such a awesome library. We have developed weseek/growi : an app that can edit markdown and show HTML preview side-by-side, and now working on to migrate to remark from markdown-it. ProblemVideo.mp4
Why this problem happens?The key is fixed by react-markdown/lib/ast-to-react.js Lines 217 to 222 in a80dfde and there is no way to overwrite that. WorkaroundFortunately, the react-markdown/lib/ast-to-react.js Lines 201 to 204 in a80dfde So you can work around this by deleting import { Node } from 'unist';
export const remarkPlugin: Plugin = function() {
return (tree) => {
visit(tree, (node, index) => {
if (node.type === 'code') {
if (isDrawioBlock(node.lang)) {
rewriteNode(node, index ?? 0); // method for drawio feature
// omit position to fix the key regardless of its position
delete node.position;
}
}
});
};
};This will change Discussion / ProposalI'm not good at English, so I apologize if I'm rude.
Exactly. But I think react-markdown need not to care that. As with comments like #466 (comment), most developers would like to manage "when the component should be re-rendered" in the component own. Therefore, I think it makes sense that Proposal 1. #466 wayThe first idea is implementing a new option like following that are suggested in #466: const key = typeof options.keyGenerator === 'function ' ? options.keyGenerator(node, pos, index) : [node.type, pos.line, pos.column, index].join('-')or const key = node.key ?? [node.type, pos.line, pos.column, index].join('-')Proposal 2. Specify by
|
|
@yuki-takei Hey, thanks for your patience. Can you explain what your algorithm would look like? That’s the thing #466 stalled on: people have a problem X, they think this solution Y solves it, but after more thinking we find out that solution Y doesn’t solve problem X. @remcohaszing What’s a bit more complex in your examples is that you use paragraphs vs images. However, those have different content types in markdown/mdast. Images are always in a paragraph (or heading). So these examples you show are all paragraphs. It’s a bit of a hard problem to think about IMO. Perhaps some actual checking of how the editor responds to different keys might be a good idea. |
|
IMO, there is no XY problem.
1. #466@dang1412 said:
I agree with him. My opinion is:
2. @ChristianMurphy on #466 (comment)He said:
I don't think so. The default So only the simple "Y problem" exists from the beginning and there is no complex "X problem". 3. @ChristianMurphy on #466 (comment)
I also don't think the custom key generator always provides better performance than the default 4. @baumandm on #466 (comment)He said the same thing with @dang1412 and me. He explained his proposal as follows:
By allowing the keys of a component to be customized, you can give programmers a way to reduce re-rendering. 5. @ChristianMurphy on #466 (comment)
This would be wrong. react-markdown/lib/ast-to-react.js Lines 217 to 222 in a80dfde And proposal 2 on #703 (comment) make us to be able to overwrite it. 6. #703@remcohaszing reported:
The above is why I treat #703 and #466 as identical problems. He shows the benefits of at least line/column agnostic keys. My workaround written in #703 (comment) stems from the same idea as his comment. 7. @wooorm on #703 (comment)
It's hard to find a solution that generates hashes from content and performs well in all cases. Rather, I think the approach needed now is to provide a way for programmers to be able to set the key that they already know or claim to be the best to reduce re-rendering. |
|
Thanks for all your responses. Can you please explain what your algorithm would look like?
What will you provide? |
|
Index as key is a well known anti-pattern in React. Often datasets contain something that uniquely identifies a key, such as an id or username. Using such an identifier helps React to know what to rerender and how to track state. However, this anti-pattern is so well-known people try to avoid The pattern used by A significant improvement would be in - properties.key = [
- name,
- position.start.line,
- position.start.column,
- index
- ].join('-')
+ properties.key = indexIf someone adds a line to a paragraph, that means the We could stop here. This would already be a significant improvement. But we can go further. Above uses index as a way to uniquely identify a unist child. However, there is another way to uniquely identify them. Given the following hast tree, we can identify the last element not just as “the fourth node”, but also as “The third element”, or “The second element whose tagName is {
"type": "root",
"children": [
{ "type": "text" },
{ "type": "element", "tagName": "p" },
{ "type": "element", "tagName": "div" },
{ "type": "element", "tagName": "p" }
]
}My idea is to use this last way of identification: “The second element whose tagName is {
"type": "root",
"children": [
{ "type": "text" },
{ "type": "element", "tagName": "p" },
{ "type": "element", "tagName": "p" },
{ "type": "element", "tagName": "div" }
]
}Now this identification still works. This means React would be able to identify this swapping of elements, and avoid deleting and creating DOM nodes and state of custom components. In export function childrenToReact(context, node) {
/** @type {Array<ReactNode>} */
const children = []
let childIndex = -1
let tagCounter = Object.create(null)
while (++childIndex < node.children.length) {
const child = node.children[childIndex]
if (child.type === 'element') {
const tagName = child.tagName
if (tagName in tagCounter) {
tagCounter[tagName]++
} else {
tagCounter[tagName] = 0
}
const key = tagName + '-' + tagCounter[tagName]
children.push(toReact(context, child, key, node))
} else {
// …
}
}
return children
}Re: hashing The goal of a key in React is not to prevent React from reusing DOM nodes. It’s to aid React determine what changed. Using a hash based on content would be deliberately preventing React to update the DOM efficiently if one character changes. In addition the hashing itself on every render is resource intensive. |
@remcohaszing Thanks for sharing the idea! I think your suggestion sounds really good. In case we insert a Edit: Wait! Sorry but what if we need it to re-render, adding text to the node will not cause the key to change, so react will think its content is unchanged and not update it. Update: |
|
@remcohaszing this seems like a reiteration of your earlier arguments. Still very good points. One of the points made there was that this project is about markdown, so there will be a lot of Additionally, on your new I think it would be very good to check these ideas in a browser to investigate their actual performance
This is the case for hashes that are meant to, indeed, produce different results if one character changes. function hash(node, counts) {
const words = toString(node).split(/\W+/)
const headAndTail = [...words.slice(0, 3), ...words.slice(-3)]
const hash = headAndTail.map(d => d.charAt(0).toLowerCase()).join('')
const count = counts.get(hash)
if (count === undefined) {
count = 0
} else {
count++
}
counts.set(hash, count)
hash += '-' + count
return hash
}You could go much further with this, but we want to keep it fast.
Correct. As so often in programming, I think we have to decide between trade-offs here. |
Here you are. Usage
ExampleBoth of the plugins I wrote below rewrite keys of import type { Element } from 'hast-util-select';
import type { Plugin } from 'unified';
import { visit } from 'unist-util-visit';
export const remarkPlugin: Plugin = () => {
return (tree) => {
visit(tree, (node, index) => {
const { type, position } = node;
// filtering
if (type !== 'code') {
return;
}
const data = node.data ?? (node.data = {});
data.hProperties = {
key: `${type}-${position?.start.line}-${position?.start.column}-${index}`,
};
});
};
};
export const rehypePlugin: Plugin = () => {
return (tree) => {
visit(tree, 'element', (node: Element, index) => {
const { tagName, position } = node;
// filtering
if (tagName !== 'code') {
return;
}
const properties = node.properties ?? {};
properties.key = `${tagName}-${position?.start.line}-${position?.start.column}-${index}`;
node.properties = properties;
});
};
};import * as customKeygenerator from './custom-keygenerator';
...
<ReactMarkdown children={markdown} remarkPlugins={[customKeygenerator.remarkPlugin]} />How does it work?
Sure! It is the "Overwrite algorithm". |
|
You show the existing algorithm to generate keys. |
|
My proposal consists of two steps of which I am certain they will decrease rerenders
Re paragraphs vs images: It’s true images are wrapped by paragraps. This example was poorly chosen. They could still be code blocks, block quotes, or transformed content though. While it’s true there are a lot of Re hashing algorithm: I now better understand the hashing algorithm provided. It’s not more unique than stages 1 or 2. However, it might be a better identifier. Perhaps it would be slightly simpler to just use the first 6 characters. I imagine a user is more likely to change the last characters thant the first, but that’s a detail and a hunch. I honestly don’t know if this hashing would help. I imagine this could be optimal in specific situations, but not others. I.e. it seems more optimal when moving paragraphs around, but less so when editing the first or last character of a paragraph. I am confident that all of the proposed formulas are better than the current approach, mostly because none of them contain the positional information. I was hesitant about a custom key function, but maybe it’s actually not that bad. We could even export two functions that implement such a key generator function. Although I’m not sure what its API should look like. |
That’s my point. Adding or removing a paragraph means all paragraphs after it get a new index. Because paragraphs are so common, that is likely significant.
It should be, in your last example, because all other nodes will remain the same. Only one node has a different key.
Same, presumably they are. That’s why I am very interested in actual performance metrics.
Can you provide reasons for why you changed your mind? |
|
TLDR: Alright, I tested this out. There is no noticable improvement with any of these methods. How I checked this
The algorithms I checked:
a), b), and c) all perform at about ±100ms between rerenders at the top, ±90ms at the bottom. Then I tested b) and c) some more by keeping CMD+V pressed to add short new paragraphs as fast as possible at the top and the bottom. I guess the thing is:
So, I propose going with |
|
I attempted a bit of profiling using the React devtools, but didn’t find very noteworthy results. I expected more significant results. It’s not something I use a lot, so maybe I did it wrong, or maybe my test case was too simple. I used the following code: import { useEffect, useState } from "react"
import { createRoot } from "react-dom/client"
import { ReactMarkdown } from "react-markdown/lib/react-markdown.js"
const components = {
p({ children, ...props }) {
const [count, setCount] = useState(0)
useEffect(() => {
setInterval(() => {
setCount((old) => old + 1)
}, 500)
}, [])
return (
<p {...props}>
{children}
{count}
</p>
)
},
}
function App() {
const [markdown, setMarkdown] = useState("")
useEffect(() => {
let count = 0
setInterval(() => {
setMarkdown((old) => {
count += 1
return (count % 2 ? "# asd\n\n" : "zxc\n\n") + old
})
}, 1000)
}, [])
return <ReactMarkdown components={components}>{markdown}</ReactMarkdown>
}
createRoot(document.getElementById("app")).render(<App />)What is noteworthy is that option c) leads to fewer DOM updates and reused state of custom components compared to b). I just don’t really see this show up in the React profiler. Still, I’m 100% for just using |
|
In our case (#703 (comment)), However, if it is fixed this time, I think |
|
I really want to understand what you want to do with that ability. See #703 (comment) for example. |
This comment has been minimized.
This comment has been minimized.
|
Released in |
With custom keys, users could have opted for |
|
Anyway, I'm glad that line/column agnostic keys were adopted. Good job. |
|
That is not a use case or a problem. That is a solution to something that isn’t a problem. |
|
Hmm.. you really don't know? What if some users say, "I want to make keys with hashes, that gives the best performance"? Are you going to discuss it again for a few months? |
|
It has been 2 years. Perhaps you can answer the question instead of dodging it? |
|
I think the idea here is that we shouldn't do premature optimisations. We shouldn't ship a feature because there may or may not be a use case in the future. Besides that, in my opinion there are certain things you don't want to expose to users, and key algorithms to improve re-renders definitely fits that category. We should leave that to knowledgeable people who are willing to proof it's better and let users have it for free as an internal change. I think we can stop the discussion here for now. Thanks for all the detailed answers. |
OK, I'll try.
You need not to know. Because no key algorithm is perfect for all use cases and problems. What I really wanted was line/column agnostic keys. So I'm satisfied with
Because I would like ASAP. |
|
Sorry, I went too far about "wasting time". OK, let's finish. Thanks for the great changes. |
Initial checklist
Problem
As can be seen here, React keys are generated using the name, line, column, and node index. This means that:
This means the key is explicitly unoptimized for React re-rendering.
Solution
The key should be calculated as the nth instance if that tag name.
I.e. the following markdown:
should be represented as:
This way if a paragraph changes, only that paragraph is changed. If another paragraph is inserted or removed, only all paragraphs that come after the edited paragraph are rerendered.
Alternatives
The text was updated successfully, but these errors were encountered: