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

Add information about ref handling in strict mode #6777

Merged
merged 5 commits into from May 9, 2024

Conversation

noahlemen
Copy link
Collaborator

No description provided.

Copy link

vercel bot commented Apr 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
19-react-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 9, 2024 5:41pm
react-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 9, 2024 5:41pm

Copy link

github-actions bot commented Apr 24, 2024

Size changes

📦 Next.js Bundle Analysis for react-dev

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 104.24 KB (🟡 +89 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Five Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/404 82.3 KB (🟡 +1.2 KB) 186.54 KB
/500 82.29 KB (🟡 +1.2 KB) 186.53 KB
/[[...markdownPath]] 84.14 KB (🟡 +1.2 KB) 188.38 KB
/errors 82.51 KB (🟡 +1.2 KB) 186.74 KB
/errors/[errorCode] 82.48 KB (🟡 +1.2 KB) 186.72 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 10% or more, there will be a red status indicator applied, indicating that special attention should be given to this.


For [`ref` callbacks](/reference/react-dom/components/common#ref-callback), React will call the callback function with the DOM node as its argument. It will then call the callback's [cleanup function](reference/react-dom/components/common#returns) before calling the `ref` callback function again with the DOM node as its argument.

Similarly, React will detach DOM refs that were created via `useRef` by setting `ref.current` to `null` before setting it back to the DOM node.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useRef is probably more common than callback refs. Maybe just flip it to put this statement before the line above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was debating doing this, but that's a good justification - thanks

@@ -825,7 +827,19 @@ Without Strict Mode, it was easy to miss that your Effect needed cleanup. By run
[Read more about implementing Effect cleanup.](/learn/synchronizing-with-effects#how-to-handle-the-effect-firing-twice-in-development)

---
### Fixing bugs found by detaching and re-attaching DOM refs in development {/*fixing-bugs-found-by-cleaning-up-and-re-attaching-dom-refs-in-development*/}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the <Canary> component should start above this line since this category is only applicable in canary

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like the header doesn't show up properly in the outline if i do that. maybe can fix that though

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal if it messes up the formatting

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just say "refs" instead of "DOM refs". Double invoking works for all of them now: https://ccp74m.csb.app/

Copy link
Collaborator Author

@noahlemen noahlemen Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't currently have terminology to differentiate clearly between refs as a general store for data (eg: a ref object created via useRef which points to some arbitrary value) versus refs as passed to an element. we currently seem to conceptualize this in the docs as being centered around interation with DOM elements (example) even though this doesn't clearly encompass the kinds of refs like your example shows. open to suggestions though – perhaps i'm overthinking this and just saying "refs" is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about "detaching and re-attachin refs to components" to make it clear we're not talking about refs as datastores like useRef?

Copy link
Collaborator Author

@noahlemen noahlemen Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think that makes sense. i wasn't sure if "components" would be inclusive of DOM elements, but it looks like elsewhere in the reference docs we refer to these as "common components", so i think that works.

@AleksandrHovhannisyan
Copy link

AleksandrHovhannisyan commented May 7, 2024

See also this issue that I opened a while back: #6123. In it, I proposed that we update the React docs to discourage developers from relying on useRef to run useEffect logic once on mount in strict mode.

Given that React 19 is now fixing refs so they run twice in strict mode (facebook/react#24670), I think it's worth flagging the "useRef to run useEffect once on mount" hack in the docs as an anti-pattern. Our code base adopted this widespread pattern back when we were upgrading to React 18, and now it seems like we've only delayed the inevitable.

For example, search up "React 18 strict mode run useEffect once" or something along those lines, and nearly every answer that comes up suggests using useRef as a workaround. For example:

https://stackoverflow.com/questions/72238175/why-useeffect-running-twice-and-how-to-handle-it-well-in-react

@rickhanlonii
Copy link
Member

@AleksandrHovhannisyan note that this only impacts element refs like DOM nodes and refs with useImperitiveEffect, refs created with useRef will not be destroyed, so this seems like a separate thing from your concern.

I added docs for the pattern you suggested here: #6846

@noahlemen noahlemen merged commit b51108e into reactjs:v19 May 9, 2024
5 of 6 checks passed
@noahlemen noahlemen deleted the strictmode-dom-refs branch May 9, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants