[flags] remove enableOwnerStacks #32426
Conversation
github-actions
bot
added
the
React Core Team
rickhanlonii
force-pushed
the
rh/remove-ff-owner-stacks
branch
from
77946d4 to
4a1c5b4
Compare
rickhanlonii
force-pushed
the
rh/remove-ff-owner-stacks
branch
from
4a1c5b4 to
4395e47
Compare
rickhanlonii
force-pushed
the
rh/remove-ff-owner-stacks
branch
from
4395e47 to
c4da3fd
Compare
| @@ -135,6 +135,7 @@ export function supportsConsoleTasks(fiber: Fiber): boolean { | |||
| return !!fiber._debugTask; | |||
| } | |||
|
|
|||
| // TODO: clean this up? | |||
There was a problem hiding this comment.
@sebmarkbage is it safe for me to delete this? Not sure about devtools versioning
There was a problem hiding this comment.
No it's not safe to delete because this is the feature detection whether this version of React supports owner stacks or not.
| @@ -1102,6 +1102,7 @@ export function attach( | |||
|
|
|||
| function getComponentStack( | |||
| topFrame: Error, | |||
| // TODO: is this needed now that owner stacks have landed? e.g. for backwards compat of devtools? | |||
There was a problem hiding this comment.
@sebmarkbage same here - can I remove the enableOwnerStacks value returned?
There was a problem hiding this comment.
No, because it feature detects the version.
There was a problem hiding this comment.
Yeah, all changes in React DevTools files should maintain compatibility with older versions of React and its internals.
| @@ -917,7 +917,7 @@ export function createProfilingHooks({ | |||
| // Creating a cache of component stacks won't help, generating a single stack is already expensive enough. | |||
| // We should find a way to lazily generate component stacks on demand, when user inspects a specific event. | |||
| // If we succeed with moving React DevTools Timeline Profiler to Performance panel, then Timeline Profiler would probably be removed. | |||
| // If not, then once enableOwnerStacks is adopted, revisit this again and cache component stacks per Fiber, | |||
| // Now that owner stacks are adopted, revisit this again and cache component stacks per Fiber, | |||
There was a problem hiding this comment.
@sebmarkbage relevant comment, not sure if this is still planned
There was a problem hiding this comment.
I'm not sure about this one. This is @hoxyq's work.
There was a problem hiding this comment.
This is not planned, just an idea for the future, when I was triaging why RDT profiling is slow - #31154.
One of the reasons why it was that slow is because we were recording component (parent) stack eagerly for each state update. With owner stacks we could re-use the information thats already on Fiber / Virtual instance.
| if (__DEV__) { | ||
| const getCurrentStack = ReactSharedInternals.getCurrentStack; | ||
| if (getCurrentStack === null) { | ||
| // TODO: when is this true? |
There was a problem hiding this comment.
@sebmarkbage is this null check needed now?
There was a problem hiding this comment.
Yes, it's when you're outside an active renderer. The idea is that when you're outside a React render you get null. When you're inside a React render but we don't have a stack you get ''. That way you can decide to display it differently.
rickhanlonii
force-pushed
the
rh/remove-ff-owner-stacks
branch
from
c4da3fd to
66aa6a4
Compare
rickhanlonii
force-pushed
the
rh/remove-ff-owner-stacks
branch
from
66aa6a4 to
886812d
Compare
| @@ -356,6 +356,7 @@ export type OnErrorOrWarning = ( | |||
| ) => void; | |||
| export type GetComponentStack = ( | |||
| topFrame: Error, | |||
| // TODO: is this needed now that owner stacks have landed? e.g. for backwards compat of devtools? | |||
Bassed off: #32425
Wait to land internally.
Commit to review.
This has landed everywhere