Bug: devtools may crash when custom hook has the name "useState"

[avkonst]

React version: 17.0.1

Steps To Reproduce

1. clone repo: https://github.com/avkonst/react-devtools-crash-demo
2. yarn start
3. open the browser of the started app
4. open devtools
5. click Counter component in the components tab of the development tools
6. Watch it not loading anything and unhandled exception dumped to the console log

Link to code example: https://github.com/avkonst/react-devtools-crash-demo

The current behavior

Devtools crash when I click on the following component in the development tools:

import React from 'react';

function useState() {
    React.useState(0);
    React.useEffect(() => () => {});
}

function Counter() {
	useState();
	React.useState(0);
	return <div>Open React Dev Tools Components panel,
		click on Counter component and
		observe the crash in the logging console.</div>;
}

export default Counter;

The expected behavior

No crash and the development tools show hooks used.

[rickhanlonii]

@avkonst thanks for submitting, continuing the discussion from https://github.com/facebook/react/issues/20609.

It's very common for stack traces to point to React internals where the issue is sourced in user code. Since you're the expert of the way your library works, can you strip down the example to use vanilla React?

[avkonst]

Have you seen the code in the demo repo? I stripped down the example to just that:

export function useState() {
    React.useState(0);
    React.useEffect(() => {
        return () => {}
    });
}

This is vanilla react, there is nothing left from Hookstate really. And this hook causes the crash if used twice in a component. I can not make it smaller than this. Is it good enough?

[avkonst]

Here is the complete reproducer in 16 lines of vanilla react code:

import React from 'react';

function useState() {
    React.useState(0);
    React.useEffect(() => () => {});
}

function Counter() {
	useState();
	React.useState(0);
	return <div>Open React Dev Tools Components panel,
		click on Counter component and
		observe the crash in the logging console.</div>;
}

export default Counter;

[avkonst]

I have updated the original description too. There is nothing left from Hookstate in the above 16 lines of vanilla react code.

[avkonst]

Committed the same code sample to the demo repository now.

[dai-shi]

Issue reproduced, and it's a naming conflict? Renaming the custom hook to useMyState seems to work. https://codesandbox.io/s/ecstatic-shockley-g1usv

[avkonst]

Maybe naming. But devtools should not influence how I name my functions, right? Compiler is happy with this name, so why not...

[rickhanlonii]

@avkonst thanks a ton for following up and reducing the code down.

The issue is that DevTools errors when a custom hook is named useState. If you rename the custom hook in your example to useStateCustom, DevTools does not error.

[avkonst]

@rickhanlonii, sure, but I can not rename the hook as it is the interface of the public library and I could not find better name originally. Hookstate.useState is in the different module and naming space than React.useState. It does not cause any conflict for the compiler and bundler, why should it for devtools. Hookstate.useState is the supercharged version of React.useState, hence named identically as it is nearly plug and play replacement.

[rickhanlonii]

Yeah, I agree, it probably shouldn't error but I don't have a much context as @bvaughn so let's wait to see what he says.

In the meantime, I've created a failing test here https://github.com/facebook/react/pull/20614.

Thanks again for reporting the issue and stripping down the issue so I could debug it quickly.

[rickhanlonii]

And thanks @dai-shi for debugging this as well, nice find!

[bvaughn]

Thanks for the bug report and concise repo. I'll take a look this morning.

[bvaughn]

I can reproduce this failure in a unit test btw:

it('should gracefully handle custom hooks that have the same name as primitive hooks', async done => {
  let inspectedElement = null;
  function Suspender({target}) {
    inspectedElement = useInspectedElement(target);
    return null;
  }

  const container = document.createElement('div');

  function useState() {
    React.useState(0);
    React.useEffect(() => {});
  }

  function HooksNamingConflict() {
    useState();
    React.useState(0);
    return null;
  }

  await utils.actAsync(() =>
    ReactDOM.render(<HooksNamingConflict />, container),
  );

  const id = ((store.getElementIDAtIndex(0): any): number);
  await utils.actAsync(
    () =>
      TestRenderer.create(
        <Contexts
          defaultSelectedElementID={id}
          defaultSelectedElementIndex={0}>
          <React.Suspense fallback={null}>
            <Suspender target={id} />
          </React.Suspense>
        </Contexts>,
      ),
    false,
  );
  expect(inspectedElement.hooks).toMatchInlineSnapshot(`
    Array [
      Object {
        "id": 0,
        "isStateEditable": true,
        "name": "State",
        "subHooks": Array [],
        "value": 1,
      },
    ]
  `);

  done();
});

Seems like a few things are required:

• Custom hook with the same name as built-in hook.
• Custom hook calls both the same name built-in hook and at least one other built-in hook.
• React component calls both the custom hook and at least one other built-in hook.

Anyway now that I have a repro should be easy enough to track down and fix between meetings today. 😄

Edit Can repro this with an even smaller test:

it('should gracefully handle custom hooks that have the same name as primitive hooks', () => {
  function useState() {
    React.useState(0);
    React.useEffect(() => {});
  }

  function HooksNamingConflict() {
    useState();
    React.useMemo(() => 0, []);
    return null;
  }

  ReactDebugTools.inspectHooks(HooksNamingConflict, {});
});

[rickhanlonii]

@bvaughn in your smaller version, I don't think you need the useEffect

[bvaughn]

Seems like (maybe among other things) this check is not great: https://github.com/facebook/react/blob/master/packages/react-debug-tools/src/ReactDebugHooks.js#L538-L539

[bvaughn]

@bvaughn in your smaller version, I don't think you need the useEffect

You do, if you want to repro the runtime error. Without it, the error isn't thrown (although the inspected tree is missing info I think)

[bvaughn]

I think the core of this problem is here: https://github.com/facebook/react/blob/a511dc7090523ee49ce21a08e55c41917d8af311/packages/react-debug-tools/src/ReactDebugHooks.js#L439-L444

This check in findPrimitiveIndex is what causes the "primitive index" to advance too far if a custom hook name matches the current built-in hook.

Comments above suggest that this is intentional, to handle wrappers added by packagers with the same name: https://github.com/facebook/react/blob/a511dc7090523ee49ce21a08e55c41917d8af311/packages/react-debug-tools/src/ReactDebugHooks.js#L431-L432

Unfortunately we don't have any test coverage for that case 😦 I can disable the lines above and my newly added tests pass (as well as all previous tests) but I suspect this just means we don't have a good integration test case.

I'm not sure how to differentiate between a packager-added wrapper and a user-added custom hook. Is there even a way?

[bvaughn]

I need to sit this task down for now because I have something more pressing to work on. If anyone else feels like picking it up, that would be great. We have pretty good test coverage in react-debug-tools.

[bvaughn]

Note that I think we could prevent the runtime error by adding a check before pushing to levelChildren:

if (levelChildren !== undefined) {
  levelChildren.push({
    id,
    isStateEditable,
    name: primitive,
    value: hook.value,
    subHooks: [],
  });
}

But this would have two downsides:

• It would mask the underlying error.
• It would hide some of the hooks information.

Consider the following component example:

function useState() {
  React.useState(0);
  React.useEffect(() => {});
}

function HooksNamingConflict() {
  useState();
  React.useMemo(() => 0, []);
  return null;
}

We should see a hooks tree of:

• State (custom)
  • State (actual)
  • Effect
• Memo

With the above fix we would only see a reported tree of:

• State (actual)
• Memo

Stepping up the stack too far in findPrimitiveIndex would cause us to miss the custom useState as well as any other primitive hooks it calls inside of it.

[avkonst]

Brian, would it be possible to apply and merge the solution you proposed in the latest comment above? The downside you mentioned is actually a good feature for hookstate support because it would hide internal hooks used by hookstate and would show only the actual state hook. I think it is the best outcome we could get really...

On Thu, 21 Jan 2021, 05:28 Brian Vaughn, [email protected] wrote:

Note that I think we could prevent the runtime error by adding a check before pushing to levelChildren:

if (levelChildren !== undefined) { levelChildren.push({ id, isStateEditable, name: primitive, value: hook.value, subHooks: [], });}

But this would have two downsides:

• It would mask the underlying error.
• It would hide some of the hooks information.

Consider the following component example:

function useState() { React.useState(0); React.useEffect(() => {});} function HooksNamingConflict() { useState(); React.useMemo(() => 0, []); return null;}

We should see a hooks tree of:

• State (custom)
  • State (actual)
  • Effect
• Memo

With the above fix we would only see a reported tree of:

• State (actual)
• Memo

Stepping up the stack too far in findPrimitiveIndex would cause us to miss the custom useState as well as any other primitive hooks it calls inside of it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/20613#issuecomment-763763249, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6JSVK5NS6P2KTAVEZA7HLS24AENANCNFSM4WIB72XA .

[bvaughn]

That's not the way custom hook support is supposed to work in DevTools, and I don't think it would be what you wanted anyway (e.g. Effect hook would be invisible in this scenario).

[avkonst]

Brian, yes, I want the effect hook invisible when it is inside of custom hook named useState. I understand you proposal does not have the downside for all other custom hooks, right?

On Thu, 21 Jan 2021, 08:52 Brian Vaughn, [email protected] wrote:

That's not the way custom hook support is supposed to work in DevTools, and I don't think it would be what you wanted anyway (e.g. Effect hook would be invisible in this scenario).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/20613#issuecomment-763892483, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6JSVJCBZQ2PD6RXF7GRLTS24YAHANCNFSM4WIB72XA .

[bvaughn]

It just happens to be the effect hook because of the way this example code is structured. It could be other arbitrary hooks that were shown or hidden if the code were structured differently. That's not a design that I think we would be able to land.

[avkonst]

Are you planning to return to this back again? We need more ideas :) I certainly have got no idea and zero knowledge of this code...

On Thu, 21 Jan 2021, 09:00 Brian Vaughn, [email protected] wrote:

It just happens to be the effect hook because of the way this example code is structured. It could be other arbitrary hooks that were shown or hidden if the code were structured differently. That's not a design that I think we would be able to land.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/20613#issuecomment-763897062, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6JSVMSVOLC4AEIKT5HSP3S24Y65ANCNFSM4WIB72XA .

[bvaughn]

I wrote this comment only a few moments ago.

I need to sit this task down for now because I have something more pressing to work on. If anyone else feels like picking it up, that would be great. We have pretty good test coverage in react-debug-tools.

It still holds.

[avkonst]

Guys, any plans or ideas when/how to progress this?

[avkonst]

Sorry, I missed the pull request progress. Seems like React team is getting close to resolve it. Great. Very much appreciated.

[avkonst]

Hi guys. What is the plan for this issue? There was some pull request, but it is unclear where it is gone...

[avkonst]

Could you please give an update? Has it been fixed?