-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Unify ReactFiberCurrentOwner and ReactCurrentFiber #29038
Conversation
// Reset this to null before calling lifecycles | ||
if (__DEV__ || !disableStringRefs) { | ||
setCurrentOwner(null); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
32f2bab
to
0812517
Compare
@@ -2586,14 +2586,14 @@ describe('TreeListContext', () => { | |||
utils.act(() => TestRenderer.create(<Contexts />)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it do in dev now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 `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.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we missing an isRendering
check in findNodeHandle
in ReactNativePublicCompat.js
, too?
This was missed in #29038 when unifying the "owner" abstractions, causing `findNodeHandle` to warn even outside of `render()` invocations.
Full list of changes: * chore[react-devtools]: improve console arguments formatting before passing it to original console ([hoxyq](https://github.com/hoxyq) in [#29873](#29873)) * chore[react-devtools]: unify console patching and default to ansi escape symbols ([hoxyq](https://github.com/hoxyq) in [#29869](#29869)) * chore[react-devtools/backend]: remove consoleManagedByDevToolsDuringStrictMode ([hoxyq](https://github.com/hoxyq) in [#29856](#29856)) * chore[react-devtools/extensions]: make source maps url relative ([hoxyq](https://github.com/hoxyq) in [#29886](#29886)) * fix[react-devtools] divided inspecting elements between inspecting do… ([vzaidman](https://github.com/vzaidman) in [#29885](#29885)) * [Fiber] Create virtual Fiber when an error occurs during reconcilation ([sebmarkbage](https://github.com/sebmarkbage) in [#29804](#29804)) * fix[react-devtools] component badge in light mode is now not invisible ([vzaidman](https://github.com/vzaidman) in [#29852](#29852)) * Remove Warning: prefix and toString on console Arguments ([sebmarkbage](https://github.com/sebmarkbage) in [#29839](#29839)) * Add jest lint rules ([rickhanlonii](https://github.com/rickhanlonii) in [#29760](#29760)) * [Fiber] Track the Real Fiber for Key Warnings ([sebmarkbage](https://github.com/sebmarkbage) in [#29791](#29791)) * fix[react-devtools/store-test]: fork the test to represent current be… ([hoxyq](https://github.com/hoxyq) in [#29777](#29777)) * Default native inspections config false ([vzaidman](https://github.com/vzaidman) in [#29784](#29784)) * fix[react-devtools] remove native inspection button when it can't be used ([vzaidman](https://github.com/vzaidman) in [#29779](#29779)) * chore[react-devtools]: ip => internal-ip ([hoxyq](https://github.com/hoxyq) in [#29772](#29772)) * Fix #29724: `ip` dependency update for CVE-2024-29415 ([Rekl0w](https://github.com/Rekl0w) in [#29725](#29725)) * cleanup[react-devtools]: remove unused supportsProfiling flag from store config ([hoxyq](https://github.com/hoxyq) in [#29193](#29193)) * [Fiber] Enable Native console.createTask Stacks When Available ([sebmarkbage](https://github.com/sebmarkbage) in [#29223](#29223)) * Move createElement/JSX Warnings into the Renderer ([sebmarkbage](https://github.com/sebmarkbage) in [#29088](#29088)) * Set the current fiber to the source of the error during error reporting ([sebmarkbage](https://github.com/sebmarkbage) in [#29044](#29044)) * Unify ReactFiberCurrentOwner and ReactCurrentFiber ([sebmarkbage](https://github.com/sebmarkbage) in [#29038](#29038)) * Dim `console` calls on additional Effect invocations due to `StrictMode` ([eps1lon](https://github.com/eps1lon) in [#29007](#29007)) * refactor[react-devtools]: rewrite context menus ([hoxyq](https://github.com/hoxyq) in [#29049](#29049))
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.