Skip to content
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

Merged
merged 10 commits into from
Feb 28, 2019

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Feb 20, 2019

Expose a new overrideHookState to be injected into DevTools to enable state and reducer hooks to be editable in DevTools. Also update ReactDebugHooks package to support this functionality.

We should release the react-debug-tools package soon.

@sizebot
Copy link

sizebot commented Feb 20, 2019

ReactDOM: size: 0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: 69060e1...d450ead

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.2% +0.2% 773.15 KB 774.33 KB 175.77 KB 176.14 KB UMD_DEV
react-dom.production.min.js 0.0% 0.0% 105.31 KB 105.33 KB 33.91 KB 33.92 KB UMD_PROD
react-dom.profiling.min.js 0.0% 0.0% 108.22 KB 108.25 KB 34.8 KB 34.8 KB UMD_PROFILING
react-dom.development.js +0.2% +0.2% 767.79 KB 768.97 KB 174.26 KB 174.63 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 105.52 KB 105.55 KB 33.41 KB 33.42 KB NODE_PROD
react-dom.profiling.min.js 0.0% 0.0% 108.6 KB 108.62 KB 34.22 KB 34.23 KB NODE_PROFILING
ReactDOM-dev.js +0.2% +0.2% 790.78 KB 791.97 KB 175.42 KB 175.79 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 0.0% 322.24 KB 322.27 KB 58.72 KB 58.73 KB FB_WWW_PROD
ReactDOM-profiling.js 0.0% 0.0% 328.82 KB 328.85 KB 60.2 KB 60.21 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js +0.2% +0.2% 773.5 KB 774.68 KB 175.91 KB 176.28 KB UMD_DEV
react-dom-unstable-fire.production.min.js 0.0% 0.0% 105.32 KB 105.34 KB 33.92 KB 33.93 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 108.24 KB 108.26 KB 34.8 KB 34.81 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.2% +0.2% 768.13 KB 769.31 KB 174.4 KB 174.77 KB NODE_DEV
react-dom-unstable-fire.production.min.js 0.0% 0.0% 105.54 KB 105.56 KB 33.42 KB 33.43 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 108.61 KB 108.63 KB 34.23 KB 34.24 KB NODE_PROFILING
ReactFire-dev.js +0.2% +0.2% 789.99 KB 791.18 KB 175.38 KB 175.75 KB FB_WWW_DEV
ReactFire-prod.js 0.0% 0.0% 310.67 KB 310.7 KB 56.38 KB 56.39 KB FB_WWW_PROD
ReactFire-profiling.js 0.0% 0.0% 317.28 KB 317.31 KB 57.8 KB 57.81 KB FB_WWW_PROFILING
react-dom-test-utils.development.js 0.0% -0.0% 47.06 KB 47.06 KB 12.99 KB 12.99 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 10.27 KB 10.27 KB 3.8 KB 3.8 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.61 KB 60.61 KB 15.92 KB 15.92 KB UMD_DEV
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.28 KB 60.28 KB 15.78 KB 15.79 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10.75 KB 10.75 KB 3.71 KB 3.71 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.63 KB 3.63 KB 1.44 KB 1.44 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.21 KB 1.21 KB 704 B 705 B UMD_PROD
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.2% 1.1 KB 1.1 KB 666 B 667 B NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% +0.3% 549.23 KB 550.41 KB 119.02 KB 119.38 KB UMD_DEV
react-art.production.min.js 0.0% 0.0% 97.31 KB 97.33 KB 29.85 KB 29.86 KB UMD_PROD
react-art.development.js +0.2% +0.3% 480.32 KB 481.5 KB 101.75 KB 102.1 KB NODE_DEV
react-art.production.min.js 0.0% 🔺+0.1% 62.4 KB 62.42 KB 19.01 KB 19.02 KB NODE_PROD
ReactART-dev.js +0.2% +0.4% 489.56 KB 490.75 KB 100.98 KB 101.34 KB FB_WWW_DEV
ReactART-prod.js 0.0% 0.0% 195.46 KB 195.49 KB 33.04 KB 33.05 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.3% 616.48 KB 617.67 KB 131.7 KB 132.05 KB RN_FB_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 246.83 KB 246.86 KB 43.07 KB 43.08 KB RN_FB_PROD
ReactNativeRenderer-profiling.js 0.0% 0.0% 253.18 KB 253.21 KB 44.62 KB 44.63 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.2% +0.3% 616.39 KB 617.58 KB 131.67 KB 132.02 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 246.84 KB 246.87 KB 43.06 KB 43.07 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js 0.0% 0.0% 253.19 KB 253.22 KB 44.61 KB 44.62 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.2% +0.3% 607.33 KB 608.52 KB 129.42 KB 129.77 KB RN_FB_DEV
ReactFabric-prod.js 0.0% 0.0% 239.17 KB 239.2 KB 41.59 KB 41.6 KB RN_FB_PROD
ReactFabric-profiling.js 0.0% 0.0% 245.4 KB 245.43 KB 43.12 KB 43.13 KB RN_FB_PROFILING
ReactFabric-dev.js +0.2% +0.3% 607.24 KB 608.43 KB 129.37 KB 129.72 KB RN_OSS_DEV
ReactFabric-prod.js 0.0% 0.0% 239.18 KB 239.21 KB 41.58 KB 41.59 KB RN_OSS_PROD
ReactFabric-profiling.js 0.0% 0.0% 245.41 KB 245.44 KB 43.12 KB 43.13 KB RN_OSS_PROFILING

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.2% +0.3% 489.42 KB 490.6 KB 103.51 KB 103.86 KB UMD_DEV
react-test-renderer.production.min.js 0.0% 🔺+0.1% 63.5 KB 63.52 KB 19.34 KB 19.35 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.3% 484.93 KB 486.12 KB 102.36 KB 102.71 KB NODE_DEV
react-test-renderer.production.min.js 0.0% 🔺+0.1% 63.12 KB 63.15 KB 19.07 KB 19.08 KB NODE_PROD
ReactTestRenderer-dev.js +0.2% +0.4% 494.98 KB 496.18 KB 101.97 KB 102.33 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% 0.0% 37.2 KB 37.2 KB 9.5 KB 9.5 KB UMD_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.2% +0.4% 477.67 KB 478.85 KB 100.11 KB 100.48 KB NODE_DEV
react-reconciler.production.min.js 0.0% 🔺+0.1% 63.56 KB 63.59 KB 18.79 KB 18.8 KB NODE_PROD
react-reconciler-persistent.development.js +0.2% +0.4% 475.86 KB 477.05 KB 99.4 KB 99.77 KB NODE_DEV
react-reconciler-persistent.production.min.js 0.0% 🔺+0.1% 63.57 KB 63.6 KB 18.79 KB 18.8 KB NODE_PROD

react-debug-tools

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-debug-tools.development.js +3.1% +3.6% 18.59 KB 19.17 KB 5.51 KB 5.71 KB NODE_DEV
react-debug-tools.production.min.js 🔺+2.2% 🔺+2.3% 5.67 KB 5.8 KB 2.3 KB 2.35 KB NODE_PROD

Generated by 🚫 dangerJS

bvaughn pushed a commit to bvaughn/react-devtools-experimental that referenced this pull request Feb 20, 2019
bvaughn pushed a commit to bvaughn/react-devtools-experimental that referenced this pull request Feb 20, 2019
bvaughn pushed a commit to bvaughn/react-devtools-experimental that referenced this pull request Feb 20, 2019
}
if (currentHook !== null) {
let updatedState = copyWithSet(currentHook.memoizedState, path, value);
currentHook.queue.dispatch(updatedState);
Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

@bvaughn bvaughn Feb 21, 2019

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!

Copy link
Collaborator

@sebmarkbage sebmarkbage left a 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?

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 21, 2019

Like we meant to do with props.

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.

@sebmarkbage
Copy link
Collaborator

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 21, 2019

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 21, 2019

Okay. I've updated the implementation so that both useState and useReducer values can be edited. This approach won't handle certain pending updates edge cases with pending updates, but I'm not convinced that's important enough to need handling as I mentioned above.

I also added an explicit isEditable param to the hook metadata (which isn't strictly necessary but felt more appropriate). I could combine the index and isEditable values, but I kind of like the more explicit approach.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 21, 2019

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 useState and useReducer values.

@@ -368,6 +369,27 @@ if (__DEV__) {
return copyWithSetImpl(obj, path, 0, value);
};

// Support DevTools editable values for useState and useReducer.
overrideHook = (
Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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 and isEditable properties with a single property, e.g. overrideState that is either a function or null.
  • 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 an overrideState function that closes over the necessary info.

Copy link
Contributor Author

@bvaughn bvaughn Feb 27, 2019

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.

Copy link
Contributor Author

@bvaughn bvaughn Feb 27, 2019

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 27, 2019

Looks like the approach I was using of updating baseState and memoizedState and then scheduling sync work stopped working between 16.8 alpha 1 and 16.8.0. The work now doesn't flush until something else triggers an update. Need to do some digging to figure out what changed between the two releases.

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 props– but that feels pretty dirty.

This definitely highlights the need for unit tests covering this functionality to avoid future regressions.

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 27, 2019

For now I'm just pushing a fix to the bailout issue mentioned in my previous comment and a rename of the inspected hook isEditable field to isStateEditable.

This was referenced Mar 10, 2020
bestlucky0825 added a commit to bestlucky0825/react-devtools-experimental that referenced this pull request Jun 1, 2022
bestlucky0825 added a commit to bestlucky0825/react-devtools-experimental that referenced this pull request Jun 1, 2022
whimsy-cat pushed a commit to whimsy-cat/relay-devtools that referenced this pull request Feb 22, 2023
whimsy-cat pushed a commit to whimsy-cat/relay-devtools that referenced this pull request Feb 22, 2023
tigrevol8888 added a commit to tigrevol8888/devtools-experimental that referenced this pull request Jul 5, 2024
tigrevol8888 added a commit to tigrevol8888/devtools-experimental that referenced this pull request Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants