Unify ReactFiberCurrentOwner and ReactCurrentFiber #29038
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
sebmarkbage
commented
May 10, 2024
| // 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
|
sebmarkbage
commented
May 10, 2024
| @@ -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
sebmarkbage
commented
May 10, 2024
| @@ -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 ↗︎
|
eps1lon
reviewed
May 21, 2024
| // @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>
eps1lon
approved these changes
May 21, 2024
gnoff
approved these changes
May 21, 2024
rickhanlonii
approved these changes
May 23, 2024
github-actions bot
pushed a commit
that referenced
this pull request
May 23, 2024
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 `isRendering` flag 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. --------- Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com> DiffTrain build for commit 2e3e6a9.
github-actions bot
pushed a commit
that referenced
this pull request
May 23, 2024
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 `isRendering` flag 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. --------- Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com> DiffTrain build for [2e3e6a9](2e3e6a9)
yungsters
reviewed
May 28, 2024
yungsters
added a commit
that referenced
this pull request
May 29, 2024
This was missed in #29038 when unifying the "owner" abstractions, causing `findNodeHandle` to warn even outside of `render()` invocations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.