facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.1k stars 46.87k forks source link

Named hooks: MVP support #16474

Closed bvaughn closed 3 years ago

bvaughn commented 5 years ago

Note this issue is outdated. The current thinking is that the alternative, "load source code (with source maps) and parse for name", is probably the best course of action.


The problem

One common piece of feedback about DevTools hooks integration is that hooks have no name and can be confusing. Consider the following example:

function useSomeCustomHook() {
  const [foo, setFoo] = useState(true);
  const [bar, setBar] = useState(false);

  // ...
}

function Example() {
  const baz = useSomeCustomHook();

  // ...
}

Currently in DevTools the above component would be displayed as follows:

SomeCustomHook:
  State: true
  State: false

This information isn't as rich as we would prefer. ☹️

The next question is often: "can you use the name of the variable the hook return value is assigned to?" but this is tricky because DevTools doesn't actually have any way to access that variable. (Even if DevTools has a handle on the Example function above, how would it access the useSomeCustomHook function?)

The proposal

The solution to this would be some form of user-defined metadata (preferably generated by a code transform). Building on the precedent of the useDebugValue hook (https://github.com/facebook/react/pull/14559), we might introduce a new no-op hook e.g. useDebugName.

The above example could make use of this hook like so:

function useSomeCustomHook() {
  const [foo, setFoo] = useState(true);
  useDebugName("foo"); // injected by Babel transform
  const [bar, setBar] = useState(false);
  useDebugName("bar"); // injected by Babel transform

  // ...
}

function Example() {
  const baz = useSomeCustomHook();

  // ...
}

DevTools could then display something like:

SomeCustomHook:
  State (foo): true
  State (bar): true

Implementation details

The new useDebugName hook might be a noop hook provided by React (similar to useDebugValue) or it could even be an export from the (soon to be released react-debug-hooks package). The key concerns would be that:

  1. It has no effect (and adds no overhead) when DevTools is not present.
  2. Not calling it at all (or only calling it for some hooks) should not break or corrupt anything.

DevTools could override the no-op useDebugName implementation before inspecting a component and automatically associate the provided name with the most recently called native hook.

For example, the following code should only result in one named hook (the second useState call).

const [foo, setFoo] = useState(true);
const [bar, setBar] = useState(false);
useDebugName("bar"); // injected by Babel transform
const [baz, setBaz] = useState(true);

Being able to support sparse name metadata would be important for third party code (that might not be transformed to supply the metadata).

A code transform would be ideal for this scenario because manual annotation would probably be cumbersome. This could also be marketed as a DEV-only transform so as not to bloat production bundles with display names. We might even try to detect the env and throw if it isn't DEV (like https://github.com/facebook/react/pull/15939).

Further considerations

Custom hooks?

In some cases, custom hooks might also be ambiguous. Consider the useSubscription hook (https://github.com/facebook/react/pull/15022):

function Example() {
  const foo = useSubscription(...);
  const bar = useSubscription(...);

  // ...
}

Currently in DevTools the above component would be displayed as follows:

Subscription: "some value"
  State: Object
Subscription: "some other  value"
  State: Object

Maybe the value alone (provided by useDebugValue) could be enough to uniquely identify the hook, but I suspect in many cases it might not be sufficient. Should we then use useDebugName for custom hooks as well?

I think it would be more fragile given the way our custom hooks detection logic is implemented. Custom hooks are not identified until after a component has finished rendering. In order for us to associate names with custom hooks, we would need to maintain a stack of names. This could lead to potential mismatches though in the event that useDebugName was called more (or fewer) times than there are custom hooks.

For example, consider the following code:

function useSomeCustomHook() {
  const [foo, setFoo] = useState(true);
  useDebugName("foo");
  useDebugName("effectively ignored");
  const [bar, setBar] = useState(false);
  const [baz, setBaz] = useState(false);
  useDebugName("baz");

  // ...
}

The proposed implementation of useDebugName would be robust enough to handle naming "foo" and "baz" states and leaving "bar" as anonymous state hook. If we were maintaining a stack of names however, this discrepency would be more difficult to manage.

Perhaps there is a clever solution to this problem. I would probably suggest leaving it out of the initial implementation though and only revisiting if we determine it's a necessary feature.

Alternatives considered

Pass debug name as an additional (unused) parameter

An alternative approach to calling a separate hook for naming purposes would be to pass the display name as an additional parameter to the native hook, e.g.:

function useSomeCustomHook() {
  const [foo, setFoo] = useState(true, "foo");
  const [bar, setBar] = useState(false, "bar");

  // ...
}

function Example() {
  const baz = useSomeCustomHook();

  // ...
}

Pros:

Cons:

Load source code (with source maps) and parse for name

We could use an extension API like Resource.getContent to load the source code (including custom hooks) and parse it determine the hook/variable names. Essentially this would work like the proposed transform above, but at runtime.

Pros:

Cons:

Call toString on the function component and parse for name

A possible 80/20 variant of the above proposal would be to simply calltoString on the function component and parse any top-level hooks.

Pros:

Cons:

Use a Babel transform to leave an inline comment (and call toString to search for it)

Rather than inserting a call to a new custom hook, our code transform could just insert an inline comment with the name. We could then parse the code to find the inline comment, e.g.:

function Example() {
  /* hook:foo:Example.react.js:3 */
  const foo = useSubscription(...);
  /* hook:bar:Example.react.js:5 */
  const bar = useSubscription(...);

  // ...
}

Pros:

Cons:


Originally reported via https://github.com/bvaughn/react-devtools-experimental/issues/323

bvaughn commented 5 years ago

Comment by @Jessidhia https://github.com/bvaughn/react-devtools-experimental/issues/323#issuecomment-507925810


One thing that useDebugName could do is, create an exception and record the stack depth. renderWithHooks itself would also record its own stack depth. If the stack depth is recorded along with the name argument, it should be possible to tell whether the call came from a custom hook or from the component.

The problem is that renderWithHooks would have to unconditionally measure its own depth before rendering the component, whether or not the component has hooks, which is a performance cost. The alternative is to only measure it if the component called useDebugName at least once, but at that point a second pass over the hook list would be needed to bind the names to the correct depth.


This could potentially break with strange setups like the regenerator transform, but I don't think any component or custom hook is allowed to be async or a generator function.

bvaughn commented 5 years ago

Comment by @audiolion https://github.com/bvaughn/react-devtools-experimental/issues/323#issuecomment-509862917


This is really well thought-out! I think my issue with the useDebugName hook, even though it follows precedent, is that it seems you -always- want to have this hook defined for future devs who need to debug the code. If this is the case, making this a separate hook that you need to learn how to combine with your normal hooks seems more error prone.

I believe adding an -arity argument to the original react provided hooks to name them, and then a useDebugName hook for custom hooks (because at this point if you are making custom hooks we can assume you have the requisite knowledge to incorporate this correctly), would be the best way forward.

Then docs can reference:

const [count, setCount] = React.useState(0, { name: 'count' });

and people will catch on that the name is for debugging purposes in the DevTools.

bvaughn commented 5 years ago

Comment by @sompylasar https://github.com/bvaughn/react-devtools-experimental/issues/323#issuecomment-509943298


  1. I think it might be more future-proof to name it useDebugInfo() and take the hook function name (which is always required) as one of possible parameters and the assignment as another, for example, you might add source address (line, column) to jump to in the code editor.
  2. Have you considered calling this debug before the hook that's being named, not after?

For custom hooks it will naturally build a stack:

function Component() { 
  useDebugInfo({fn:"useSubscription",lhs:"const {something}"});
  const {something} = useSubscription();
}
bvaughn commented 5 years ago

Comment by @audiolion https://github.com/bvaughn/react-devtools-experimental/issues/323#issuecomment-510065137


After some more thought, if we did have a useDebugName hook, someone could just release a package with wrappers around useState and other built-in react hooks that include useDebugName and useDebugValue. I changed my mind and vote on the custom hook.

dpgalloway commented 5 years ago

This useDebugName hook would be very useful. Any time there are a few useState hooks within a component it gets very hard to efficiently debug. Any update on this or any suggestion for a workaround?

inoyakaigor commented 4 years ago

So, what's the decision? Are there any hook names in the plans, and if so, what way will be chosen and is there a specific date or version number?

bvaughn commented 4 years ago

There is no decision on this. If there were, that info would be on the issue.

muescha commented 4 years ago

FYI:

i found an workaround for state with label at stack overflow https://stackoverflow.com/a/58579953/1930509 but i also would prefer an included solution

const [item, setItem] = useStateWithLabel(2, "item");
function useStateWithLabel(initialValue, name) {
    const [value, setValue] = useState(initialValue);
    useDebugValue(`${name}: ${value}`);
    return [value, setValue];
}
danielvanc commented 4 years ago

Just reading about useDebugValue on react docs and they don't recommend it with every custom hook. Maybe it's expensive? Would that then mean that you should use useStateWithLabel with caution?

muescha commented 4 years ago

I think is a workaround

if you see the code for code with useDebugValue it is only available in Dev mode otherwise an noOp:

https://github.com/facebook/react/blob/6c00c5bba4d2272de6975c124ab6dc29abb629c8/packages/react/src/ReactHooks.js#L140-L148

outerlook commented 4 years ago

Hi! With named hooks, wouldn't be useful to be able to filter to find currently components that are using a specific custom hook on searchbar?

I came to this issue after searching if it is somehow implemented. My use case is:

I'm trying Relay Hooks (experimental) I'm making some queries reusable, to prevent passing too much props, and I'm focusing on grouping components that use the same information on a view.

I could do this faster if I could go to the page and filter components that are using useCurrentUserQuery( ) for example, then implement a useCurrentUserQueryHomePage() for that group viewed on home page.

pdevito3 commented 4 years ago

Any chance we could revive this? Moving to react from vue and this is a majorly annoying part of working with react dev tools vs vue.

Personally prefer an additional param on useState to not clog up my code, but whatever works honestly.

leidegre commented 3 years ago

This is a minor detail but some of this is alleviated by simply doing.

function useMyHook(initialValue) {
  return React.useState(initialValue)
}

function MyComponent() {
  const [value, setValue] = useMyHook(0)

  return "..."
}

The developer tools will display this as a MyHook with a State hook nested under it. It allows you to create a heading by simply grouping hooks in function names.

image

If you have a really big and complex component this might not be possible to do but you probably want to break down that big complex component into something smaller anyway...

andrewchen5678 commented 3 years ago

+1 just starting to use react hooks and encountered this issue which makes debugging much harder than classes, I guess I'll stick with classes for now for most of my works until this is solved.

Qwertie- commented 3 years ago

@andrewchen5678

In my hooks I just do

const [state, setState] = useState({
  thing: value,
  thing2: value2
});

This way they show up in the dev tools with names. Then when you want to update you can do

setState((previousState) => {
  return { ...previousState, thing2: value };
});
Lagicrus commented 3 years ago

@andrewchen5678

In my hooks I just do

const [state, setState] = useState({
  thing: value,
  thing2: value2
});

This way they show up in the dev tools with names. Then when you want to update you can do

setState((previousState) => {
  return { ...previousState, thing2: value };
});

Got a screenshot to show this in action?

Qwertie- commented 3 years ago

@Lagicrus

Looks like this in the dev tools

Screenshot from 2021-03-23 15-43-37

JimmayVV commented 3 years ago

All hooks, not just the state hook, would benefit from some mechanism to display the underlying values in dev tools. Generally I agree it'd be best to inspect the actual values these hooks produce in source maps or the like but if we really want to name these hooks in dev tools, we should be able to opt into declaring named functions instead of anonymous arrow functions.

This isn't such a great experience in my humble opinion: image

bvaughn commented 3 years ago

All hooks, not just the state hook, would benefit from some mechanism to display the underlying values in dev tools.

DevTools already does this for all built-in hooks.

The example you show above contains custom hooks (.wrappedHook) for which we provide the useDebugValue API.

JimmayVV commented 3 years ago

All hooks, not just the state hook, would benefit from some mechanism to display the underlying values in dev tools.

DevTools already does this for all built-in hooks.

The example you show above contains custom hooks (.wrappedHook) for which we provide the useDebugValue API.

Without giving away the entire business logic, this is what the useMemo's are in my example (which they themselves are within a custom hook, which is what the CarPaintState is):

const aiDriver = React.useMemo(() => aiRoster?.rosterDrivers[aiRoster?.rowIndex], [aiRoster]);

If there's a way to name the reference to that hook by using a named function I'm not seeing it work.

Of course for all I know I might not be on a recent enough version of dev tools

This is what I've tried, that wasn't working:

    const aiDriver = React.useMemo(
        function aiDriver() {
            return aiRoster?.rosterDrivers[aiRoster?.rowIndex];
        },
        [aiRoster]
    );

image

bvaughn commented 3 years ago

I think you're conflating things now. This issue is strictly about showing names for built-in hooks. The previous comment (the one I was replying to) was about showing values for built-in hooks. DevTools already does the latter (and supports a solution for custom hooks too).

Showing names for built in hooks isn't supported yet. That's why this issue is open. 😄

We have a work in progress solution but it's not finished yet.

JimmayVV commented 3 years ago

sorry, you're right, I did conflate the term name and value - I was intending to stick to the original topic; the only reason I even commented was because there were previous comments insinuating that topic wasn't receiving attention and wanted to 👍 that there's an appetite for this still without risking this issue going stale.

By the way, thanks for all you do to make the web a better place for everyone 😄

bvaughn commented 3 years ago

Check out this PR for work in progress on this: https://github.com/MLH-Fellowship/react/pull/115#event-4127480301

It hasn't been dropped or forgotten. We're just a small team so we have to prioritize larger efforts like this fairly aggressively.

bvaughn commented 3 years ago

Closed via #21641

Note that, initially, this feature is only enabled for internal builds of the extension. I'd like to get people trying the feature out and reporting bugs for a bit before I release it to the larger browser stores.

Worth keeping in mind that this feature can be slow for large apps, which is why it's off by default, but the follow up task #21782 should hopefully help with this.

https://user-images.githubusercontent.com/29597/124169487-4b573500-da74-11eb-9a47-690e4a82413b.mp4

MetaMmodern commented 3 years ago

Hello @bvaughn , sorry for writing into closed issue but what should I change in my React code for this to work? Cause I've done the thing you showed in gif(video) but it didn't work, no names appeared. I'm using webpack 5 with typescipt and babel.

bvaughn commented 3 years ago

@MetaMmodern Share a repro?

MetaMmodern commented 3 years ago

@bvaughn I can share screenshots: Here is the code: image

And what I see in react tools: image

Do I need to share my weback config?

bvaughn commented 3 years ago

@MetaMmodern Need a full repro that I can run. Screenshots don't help.

MetaMmodern commented 3 years ago

@MetaMmodern Need a full repro that I can run. Screenshots don't help.

Ok, that will take some time to create, I'll ping you here in a couple of days or on weekend then)

bvaughn commented 3 years ago

Do I need to share my weback config?

Actually, there is a thing that could be relevant here: If your Webpack is using one of the cheap eval map variants, we don't support that b'c we can't load the underlying source code. (The source URL given is something like webpack-internal://... which isn't a real protocol.

We could only access this if we used internal browser APIs via the "debugger" permission, but that permission is not something we'll want to add to DevTools. It's super powerful, and as a counter balance to that, Chrome shows a banner that says something like, "an extension is currently controlling your browser".

inoyakaigor commented 3 years ago

@bvaughn

cheap eval map variants

devtools: 'eval' also?

MetaMmodern commented 3 years ago

Do I need to share my weback config?

Actually, there is a thing that could be relevant here: If your Webpack is using one of the cheap eval map variants, we don't support that b'c we can't load the underlying source code. (The source URL given is something like webpack-internal://... which isn't a real protocol.

We could only access this if we used internal browser APIs via the "debugger" permission, but that permission is not something we'll want to add to DevTools. It's super powerful, and as a counter balance to that, Chrome shows a banner that says something like, "an extension is currently controlling your browser".

Well, for devtool I use inline-source-map. Is that okay or you can suggest anything better?

MetaMmodern commented 3 years ago

@bvaughn

cheap eval map variants

devtools: 'eval' also?

Just tried eval, it gives hooks parse failed error.

bvaughn commented 3 years ago

Yes, anything with "eval" doesn't work.

If you're using inline, it should work. Le'ts get a repro :)