Unify ReactFiberCurrentOwner and ReactCurrentFiber #29038
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
| // Reset this to null before calling lifecycles | ||
| if (__DEV__ || !disableStringRefs) { | ||
| setCurrentOwner(null); | ||
| } |
There was a problem hiding this comment.
We weren't really initializing this to anything and we were always cleaning it up so I don't think this has been necessary for a while. However, since the life cycles also set the current debug fiber we get the resetting for free regardless - at least in DEV.
|
Comparing: 55dd0b1...599e50f Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
| @@ -2486,7 +2486,7 @@ export function commitMutationEffects( | |||
|
|
|||
| setCurrentDebugFiberInDEV(finishedWork); | |||
| commitMutationEffectsOnFiber(finishedWork, root, committedLanes); | |||
| setCurrentDebugFiberInDEV(finishedWork); | |||
| resetCurrentDebugFiberInDEV(); | |||
There was a problem hiding this comment.
This was an bug before and the layout effects just reset to previous so it left the last mutation fiber as the debug fiber after commit.
sebmarkbage
force-pushed
the
currentowner
branch
3 times, most recently
from
32f2bab
to
0812517
Compare
| @@ -2586,14 +2586,14 @@ describe('TreeListContext', () => { | |||
| utils.act(() => TestRenderer.create(<Contexts />)); | |||
There was a problem hiding this comment.
This used to log an react-test-renderer is deprecated error which shouldn't be counted towards the rendered errors. It was previously incorrectly associated to the previous tree because the "current fiber" wasn't reset after the commit phase of the previous render.
That's why the snapshots below have one less error counted.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| // @gate !disableStringRefs | ||
| it('throws an error', async () => { | ||
| // @gate !disableStringRefs && !__DEV__ | ||
| it('throws an error in prod', async () => { |
There was a problem hiding this comment.
What does it do in dev now?
There was a problem hiding this comment.
We use the same fields for dev and prod just to temporarily support string refs but in DEV it's active for the entire begin / complete phases for debugging purposes. Meaning that it just works in dev because the owner is set up but in prod it's not.
It still has a console.error in DEV so you shouldn't add any new callers as long as the warning actually shows up.
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
We previously had two slightly different concepts for "current fiber".
There's the "owner" which is set inside of class components in prod if string refs are enabled, and sometimes inside function components in DEV but not other contexts.
Then we have the "current fiber" which is only set in DEV for various warnings but is enabled in a bunch of contexts.
This unifies them into a single "current fiber".
The concept of string refs shouldn't really exist so this should really be a DEV only concept. In the meantime, this sets the current fiber inside class render only in prod, however, in DEV it's now enabled in more contexts which can affect the string refs. That was already the case that a string ref in a Function component was only connecting to the owner in prod. Any string ref associated with any non-class won't work regardless so that's not an issue. The practical change here is that an element with a string ref created inside a life-cycle associated with a class will work in DEV but not in prod. Since we need the current fiber to be available in more contexts in DEV for the debugging purposes. That wouldn't affect any old code since it would have a broken ref anyway. New code shouldn't use string refs anyway.
The other implication is that "owner" doesn't necessarily mean "rendering" since we need the "owner" to track other debug information like stacks - in other contexts like useEffect, life cycles, etc. Internally we have a separate
isRenderingflag that actually means we're rendering but even that is a very overloaded concept. So anything that uses "owner" to imply rendering might be wrong with this change.This is a first step to a larger refactor for tracking current rendering information.