Support configurable labels for custom hooks#14559
Conversation
|
ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: f290138...fa8d3d1 react-dom
react-art
react-native-renderer
react-test-renderer
react-debug-tools
Generated by 🚫 dangerJS |
|
When I first saw the name “label” I thought it would be a string to name the field itself - which I don’t think we should do. But perhaps others could be confused by the name as I was. How about |
|
|
||
| export function useDebugValueLabel(valueLabel: string) { | ||
| const dispatcher = resolveDispatcher(); | ||
| return dispatcher.useDebugValueLabel(valueLabel); |
There was a problem hiding this comment.
Can we wrap this in if (__DEV__) {? That would make this function a noop which can be trivially eliminated by a bundler. At least with ES modules. So that would let us remove the callsite itself in prod bundles.
|
The function it calls in the dispatcher itself is already a noop, unless
devtools are installed.
|
|
Not all dispatchers are noops so there still needs to be a lookup of which dispatcher is active and that thing called. A call is not a noop. I.e. a bundler doesn’t know that it will be a noop so it can’t delete it. A VM can’t neither because any dispatcher can be installed that isn’t a noop. |
|
Yeah, I dig |
5bf400e to
597af0f
Compare
1. Renamed useDebugValueLabel hook to useDebugValue 2. Wrapped useDebugValue internals in if-DEV so that it could be removed from production builds.
597af0f to
5d7b4be
Compare
| export function useDebugValue(valueLabel: string): void { | ||
| // This hook is normally a no-op. | ||
| // The react-debug-hooks package injects its own implementation | ||
| // so that e.g. DevTools can display customhook values. |
| }); | ||
| } | ||
|
|
||
| // Associate custom hook values (useInpect() hook entries) with the correct hooks |
There was a problem hiding this comment.
No longer called useInspect
| return rootChildren; | ||
| } | ||
|
|
||
| function rollupDebugValues(hooksNode: HooksNode): void { |
There was a problem hiding this comment.
What does this function do exactly? I struggle to understand just from reading the code — maybe because of filter with a mutation and recursion inside. Not saying it needs to be changed but a brief comment about what it does to the tree would help.
| ]); | ||
| }); | ||
|
|
||
| describe('useDebugValue', () => { |
There was a problem hiding this comment.
Should we also have a test for useDebugValue on top level of component? Outside a Hook.
There was a problem hiding this comment.
Also should we add tests for updates? To verify inspectHooksOfFiber can find latest version. (In case it matters — I don't understand this enough to tell yet.)
Ah nvm, I see you pass current fiber explicitly.
There was a problem hiding this comment.
Sure, I can add a test for that. I'm not sure what the expected behavior should be. A warning? Just ignore it entirely?
For now I'll just remove them.
gaearon
left a comment
gaearon
left a comment
There was a problem hiding this comment.
I don't understand how rollupDebugValues works but overall I think this makes sense.
sebmarkbage
left a comment
sebmarkbage
left a comment
There was a problem hiding this comment.
What happened to the transform function as second arg? Do we expect that we don't need that?
| function rollupDebugValues(hooksNode: HooksNode): void { | ||
| let useInpectHooksNodes: Array<HooksNode> = []; | ||
| hooksNode.subHooks = hooksNode.subHooks.filter(subHooksNode => { | ||
| if (subHooksNode.name === 'DebugValue') { |
There was a problem hiding this comment.
Maybe we should tag these nodes with something more unique to the built-in one so that it doesn't happen with custom hooks that happen to share the same name. E.g. You could also check that this is the terminal node. children.length === 0
There was a problem hiding this comment.
Good point. I meant to do this but forgot. 😁
There was a problem hiding this comment.
This seems trickier than I thought because of how we're parsing the stack for function names and then comparing them to the primitive labels.
For example, I can name the local function something more unique (e.g. useDebugValue__REACT_INTERNAL) but if it's assigned to the dispatcher as useDebugValue– then function name that's being parsed and passed to isReactWrapper is useDebugValue.
Maybe I'm overlooking something obvious. I'll come back to this.
Or maybe I'll just use the subHooks.length heuristic.
|
|
||
| function rollupDebugValues(hooksNode: HooksNode): void { | ||
| let useInpectHooksNodes: Array<HooksNode> = []; | ||
| hooksNode.subHooks = hooksNode.subHooks.filter(subHooksNode => { |
There was a problem hiding this comment.
Maybe just make this into a for-loop. Like Dan, I find it confusing if something is both functional style and imperative. I assume this to be pure if it's using filter.
I forgot about it 😄 I'll add it. |
|
Okay, I think I've applied all of the suggested feedback. |
1. Fixed some minor typos 2. Added inline comment explaining the purpose of rollupDebugValues() 3. Refactored rollupDebugValues() to use a for loop rather than filter() 4. Improve check for useDebugValue hook to lessen the chance of a false positive 5. Added optional formatter function param to useDebugValue
4a2d812 to
683907b
Compare
|
|
|
What about partial renderer (server), I don't see it in this PR? Should be a no-op too, otherwise it'll crash on use. |
|
Rats. It's easy to forget about that use case. Thanks for the catch! 😄 I'll pick this up tomorrow with another PR! Follow up PR: #14597 |
Summary: Add definitions for new debug hook, `useDebugValue` (react/react#14559) I'm sorry for not updating tests 😦`make` and `yarn install` both still failing for me. cc jbrown215 Pull Request resolved: #7356 Reviewed By: bvaughn Differential Revision: D13672750 Pulled By: jbrown215 fbshipit-source-id: a48bd161f001ecfb770f9d708e762ffa3ed5b733
* react-debug-tools accepts currentDispatcher ref as param * ReactDebugHooks injected dispatcher ref is optional * Support custom values for custom hooks * PR feedback: 1. Renamed useDebugValueLabel hook to useDebugValue 2. Wrapped useDebugValue internals in if-DEV so that it could be removed from production builds. * PR feedback: 1. Fixed some minor typos 2. Added inline comment explaining the purpose of rollupDebugValues() 3. Refactored rollupDebugValues() to use a for loop rather than filter() 4. Improve check for useDebugValue hook to lessen the chance of a false positive 5. Added optional formatter function param to useDebugValue * Nitpick renamed a method
* react-debug-tools accepts currentDispatcher ref as param * ReactDebugHooks injected dispatcher ref is optional * Support custom values for custom hooks * PR feedback: 1. Renamed useDebugValueLabel hook to useDebugValue 2. Wrapped useDebugValue internals in if-DEV so that it could be removed from production builds. * PR feedback: 1. Fixed some minor typos 2. Added inline comment explaining the purpose of rollupDebugValues() 3. Refactored rollupDebugValues() to use a for loop rather than filter() 4. Improve check for useDebugValue hook to lessen the chance of a false positive 5. Added optional formatter function param to useDebugValue * Nitpick renamed a method
* react-debug-tools accepts currentDispatcher ref as param * ReactDebugHooks injected dispatcher ref is optional * Support custom values for custom hooks * PR feedback: 1. Renamed useDebugValueLabel hook to useDebugValue 2. Wrapped useDebugValue internals in if-DEV so that it could be removed from production builds. * PR feedback: 1. Fixed some minor typos 2. Added inline comment explaining the purpose of rollupDebugValues() 3. Refactored rollupDebugValues() to use a for loop rather than filter() 4. Improve check for useDebugValue hook to lessen the chance of a false positive 5. Added optional formatter function param to useDebugValue * Nitpick renamed a method
6 participants
Builds on top of PR #14556 to add support for configurable labels for custom hooks (for DevTools) by way of a new, debug-only hook named
useDebugValue.Using the debounce hook from usehooks.com as an example:
This would render like so:
