Add documentation for ref cleanup functions #6770
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Size changes📦 Next.js Bundle Analysis for react-devThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
Used this test scenario to clarify how callback refs work when detached based on the availability of a cleanup function to update documentation in reactjs/react.dev#6770 Checking it in for additional test coverage and test-based documentation
There was a problem hiding this comment.
the content of this looks good to me overall, but considering versioning, maybe it would be best to update just the reference/ page, with all the changes within a CanaryBadge (that is, the information about cleanup functions / ref callbacks called with null would also be broken out to a canary badge)
then, after release, we'd update thelearn/ page as well? it seems like so far learn/ does not make reference to the canary, and that's probably not relevant information for that audience until a release.
|
What I would do is inline the reference similar to |
|
Updated reference changes to be inside a Canary UI block. Also reverted the null argument change in the list example, but kept the other changes since the old image service is broken and the updates make it easier for us to drop in the cleanup function usage later. |
| // Remove from the Map | ||
| map.delete(cat.id); | ||
| } | ||
| // Add to the map |
There was a problem hiding this comment.
This doesn't indicate canary? I also don't understand why we'd need to add/remove cats?
There was a problem hiding this comment.
For the Canary thing, I would keep the example the same and add a block.
There was a problem hiding this comment.
Ah thanks I missed this spot. Updating to a Canary block.
For the add/remove, this was to make it more obvious how/when the cleanups are fired if you want to extend the example with logs or other behavior. Was helpful when putting it together but I'll update to simplify here.
| {catList.map(cat => ( | ||
| {catList.map((cat) => ( | ||
| <li | ||
| key={cat.id} | ||
| key={cat} | ||
| ref={(node) => { | ||
| const map = getMap(); | ||
| if (node) { | ||
| map.set(cat.id, node); | ||
| map.set(cat, node); | ||
| } else { | ||
| map.delete(cat.id); | ||
| map.delete(cat); | ||
| } | ||
| }} | ||
| > | ||
| <img | ||
| src={cat.imageUrl} | ||
| alt={'Cat #' + cat.id} | ||
| /> | ||
| <img src={cat} /> |
There was a problem hiding this comment.
should we leave all this as is? and keep the { id, imageUrl } structure in setupCatList? while this change is probably what i would prefer in actual code, i think the object makes things a bit more immediately grokkable in the context of examples
There was a problem hiding this comment.
whoops, didn't see you already merged. i don't feel strongly about this.
No description provided.