-
Notifications
You must be signed in to change notification settings - Fork 1.8k
MVP hooks support #1272
MVP hooks support #1272
Conversation
…ce "opaque node" just points to whichever fiber happened to be created first for the pair)
Should the inputs array given to useCallback (2nd argument) also be part of the inspection? It is part of the hook node at least. |
No, we aren't currently planning on surfacing that info in DevTools. |
8ce4a2e
to
b5ac442
Compare
Given I have component like below:
In devtools it will display
So there will be mutiple "state"-objects in the list if I do not use "custom hooks". Is what I am describing correct? |
@TryingToImprove most likely it will look like this
|
I should clarify my previous comment. Yes, you will see multiple "State" hooks– because you are calling So with the code example you provided, it would actually look like this:
|
4cc286d
to
f1a92de
Compare
Okay, I think this is ready for review. |
agent/Agent.js
Outdated
} | ||
if (this._prevInspectedHooks !== inspectedHooks) { | ||
this._prevInspectedHooks = inspectedHooks; | ||
this.emit('inspectedHooks', inspectedHooks); |
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.
Doing this (storing data, having Fiber-specific code) in the Agent sucks. I'm open to suggestions for a better place for this logic to live, but at the same time– I didn't stress about it too much because I think this version of DevTools is short lived (without either a rewrite or a refactor).
agent/Agent.js
Outdated
if (internals && internals.currentDispatcherRef) { | ||
// HACK: This leaks Fiber-specific logic into the Agent which is not ideal. | ||
// $FlowFixMe | ||
const currentFiber = data.state === internalInstance.memoizedState ? internalInstance : internalInstance.alternate; |
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 is gross for obvious reasons, but it needs to live somewhere. Happy to move it out of the Agent if anyone thinks it's too gross even for a temporary solution.
Does it still show "undefined" for Hooks that don't have values? Is that necessary/useful? Can be just empty? |
Good call. I added a guard for this. |
backend/attachRendererFiber.js
Outdated
var containsHooks = | ||
(tag === FunctionComponent || | ||
tag === SimpleMemoComponent || | ||
tag == ForwardRef) && |
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 you wanting to use ==
for the FowardRef
comparison here?
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.
Typo. Doesn't actually matter in this case but I'll fix it.
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.
I am a merciful god.
(I'll test this later tonight, but lgtm!)
theme: Theme, | ||
}; | ||
|
||
function HooksNodeView({ hooksNode, index, inspect, path, theme }: HooksNodeViewProps) { |
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.
yo dawg I heard you like hooks
@@ -251,4 +270,9 @@ const noPropsStateStyle = (theme: Theme) => ({ | |||
padding: '0.5rem', | |||
}); | |||
|
|||
const hooksTreeLoadingStyle = { |
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.
any reason this isn't in a stylesheet? (low pri, ignore)
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.
Just following the (super gross) convention within the file. Old DevTools are pretty gross.
Updated Profiler UI to not show a "State" pane for Fibers using hooks. Unfortunately, we can't show the hooks either– because we can't inspect stale Fibers. |
Just to confirm. If I specifically call |
There is no way for DevTools to differentiate between the two states of "I explicitly called |
We can't show hooks values either, unfortunately, since we can't use the ReactDebugHooks util for stale fibers because their props will have changed.
73ff54c
to
f456a68
Compare
I moved the fiber+hooks specific hacks inside of |
Support (read-only) inspectable hooks.
Open questions
How should we handle production mode?
How should we handle hooks inspection in production mode? The
react-debug-hooks
package relies on parsing function names to identify custom hooks. If these names are mangled, they won't be identifiable, and as a result– the hooks tree will look strange.Development:
Production: