-
Notifications
You must be signed in to change notification settings - Fork 46.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
Support editable useState hooks in DevTools #14906
Conversation
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: 69060e1...d450ead react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
react-debug-tools
Generated by 🚫 dangerJS |
} | ||
if (currentHook !== null) { | ||
let updatedState = copyWithSet(currentHook.memoizedState, path, value); | ||
currentHook.queue.dispatch(updatedState); |
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 will work for useState
but not useReducer
, which I assume is intentional, correct?
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 really should support useReducer if we support useState since that’s the recommended one. Don’t want people choosing useState over useReducer based on DevTools.
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.
Interesting. I thought a little (not much) about useReducer
support but didn't think it was necessary.
Edit for clarity: DevTools only supports edit functionality (currently) for useState
but I could go back to the drawing board if we think it's important to support useReducer
too!
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 see two possible semantics for editing state. One option is that we actually edit the application’s internal state. The other option is that we permanently shadow the value as it is returned to the render function until something forces it to release. Like we meant to do with props.
What are the tradeoffs for these approaches?
I don't think we necessarily meant to do this with props. It's something we've discussed but never committed to (at least not that I recall, although I'm forgetful). I think it could be nice in some cases, but I'm personally not convinced that it's useful often enough to justify the added complexity of building it to be honest. At least not at this stage of the rewrite. |
I’d be happy to add the infra myself to help out adding this to props. I’d rather have an approach that is conceptually consistent than relies on subtle details of rerendering that breaks during refactors. We’re dealing with the exact problem for setNativeProps on Fabric now and it’s a pain. The difference with state is that conceptually you can update it to anything and it is persistent. So we don’t have to do it for state but I haven’t thought much about the tradeoffs yet. |
I remain unconvinced that the added complexity of masking is worth it as a feature. Based on my recent Twitter poll (which isn't necessarily conclusive– but it did have 1,500 votes so I think it's at least worth consideration) only 19% of people use the feature regularly to begin with, and most of them only use it for toggling booleans to e.g. test visual state. Maybe you and I can talk about this today over a coffee or something and come to a consensus that unblocks me for now @sebmarkbage. |
Okay. I've updated the implementation so that both I also added an explicit |
15b3e6e
to
6423e94
Compare
I've pushed a version of the DevTools built with this React change to https://react-devtools-experimental.now.sh/ if you'd like to test out the editable |
@@ -368,6 +369,27 @@ if (__DEV__) { | |||
return copyWithSetImpl(obj, path, 0, value); | |||
}; | |||
|
|||
// Support DevTools editable values for useState and useReducer. | |||
overrideHook = ( |
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.
Let's call this something specific to state since there are many other hooks. E.g. might want to override changed bits etc. for debugging context not updating. For Focal we might want to flip the booleans which are not state.
overrideHookState
?
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 guess the isEditable
just indicates that the "value" can be edited as is in whatever form it is. Maybe overrideHookDebugValue
whatever the representation of debug tools' value is?
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 suggesting I rename the injected overrideHook
method to overrideHookDebugValue
? I think that sounds confusingly close to useDebugValue
even though they aren't actually related. Maybe I'm misunderstanding you.
overrideHookState
seems okay though. Probably more clearly indicates its purpose.
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 think I've complied with part of your request (renaming overrideHook
to overrideHookState
) but I'm still a little fuzzy on the other comment. Would you like me to also rename isEditable
to something like isStateEditable
?
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.
Chatted offline and the consensus was this:
- Replace
index
andisEditable
properties with a single property, e.g.overrideState
that is either a function ornull
. - DevTools will pass through the renderer-injected values to
react-debug-tools
when inspecting a fiber. - For hooks that are editable,
react-debug-tools
will add anoverrideState
function that closes over the necessary info.
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.
Actually implementing the changes above is proving to be kind of complicated, and I'm having second thoughts about it being a good idea. It requires maintaining and coordinating several data structures, all of which are updated async.
Inspecting hooks
React itself has the hooks list stored in memoizedState
. These hooks are what we actually need to update.
ReactDebugHooks
has its own inspected hooks object, consisting of a pointer to the "native" hook, the name (e.g. "State"), value, array of sub-hooks (if it's a custom hook), and an overrideState
function that closes over things needed to e.g. modify a state or reducer hook.
The frontend needs its own representation, but since this is sent across the Bridge– we need to replace the overrideState
function with something that can be serialized. So we need an ID that lets us map back to the original closure function. This means a third structure– with an id, name, value, sub-hooks array, and an isStateEditable
boolean indicating whether there's an overrideState
function.
Overriding state
The frontend starts by sending an "overrideHookState" message with the hook id, path, override value, and the renderer ID (so the backend can find the right injected internals).
The agent uses the renderer ID to pass this info along to the appropriate renderer interface, which needs to be able to find the original inspected hook (from ReactDebugHooks
, using our ID) so it can call the overrideState
method.
Then the overrideState
method needs a way to find the current fiber, and to call the renderer's injected overrideHookState
method with that fiber along with the "native" hook, path, and value.
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.
@sebmarkbage Let's talk about this again. I'm not convinced that this change is a positive one after looking into how I would implement it. I think it adds a lot of complexity without adding enough value to offset it.
6423e94
to
dc53d9c
Compare
Looks like the approach I was using of updating Edit - It looks like the approach had settled on for overriding both state and reducer hook values was broken by the change to bailout reducer bailout (PR #14569). This change makes supporting editable reducer hooks difficult, since I can't add an update to the queue (since there's no action for DevTools overrides). I can cheat around this by shallow-cloning This definitely highlights the need for unit tests covering this functionality to avoid future regressions. |
For now I'm just pushing a fix to the bailout issue mentioned in my previous comment and a rename of the inspected hook |
dc53d9c
to
1c6b5b3
Compare
…e bool to hooks metadata
Expose a new
overrideHookState
to be injected into DevTools to enable state and reducer hooks to be editable in DevTools. Also updateReactDebugHooks
package to support this functionality.We should release the
react-debug-tools
package soon.