facebook/react

useCallback doesn't memoize callback in shallow renderer

chenesan opened this issue · 8 comments

Do you want to request a feature or report a bug?

bug

What is the current behavior?

In shallow renderer, The returned callback from useCallback is not the same one between two rerendering even though the dependencies are the same. But useMemo will work as expected.

For example, the following test won't work:

    function SomeComponent() {
      const noop = React.useCallback(() => {}, []);

      return (
        <div onClick={noop} />
      );
    }

    const shallowRenderer = createRenderer();
    let firstResult = shallowRenderer.render(<SomeComponent />);
    let secondResult = shallowRenderer.render(<SomeComponent />);

    expect(firstResult).toEqual(secondResult);

If useCallback returned the same callback between two rendering (I think) the assertion should pass.

but the same (almost) logic will work with useMemo, in the shallow renderer test suite (See https://github.com/facebook/react/blob/master/packages/react-test-renderer/src/__tests__/ReactShallowRendererHooks-test.js#L273-L291).

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

I created a branch in https://github.com/chenesan/react/tree/usecallback-not-work-properly-in-shallow-renderer and you can run the failed shallow renderer test in chenesan@b2dff28#diff-d9a78422c03941578ae9ba487e8132cb .

What is the expected behavior?

useCallback should return the memoized callback when the dependencies unchanged even in shallow renderer.

I tried to look into this and I found out that in shallow renderer useCallback just returns the original callback argument (See https://github.com/facebook/react/blob/master/packages/react-test-renderer/src/ReactShallowRenderer.js#L365-L371) but useMemo will compare the dependencies between rendering. I'm not sure if it's intended (So it's expected that useCallback will not memoize callback in shallow renderer). If it's triaged as a bug I'm glad to send a pr for this :)

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

react@16.8.6

yup, this looks like a miss on our part. Would you like to send a PR? :)

@threepointone thanks for reply :-) I'd try to make a PR in a few days.

I don't think it's a miss on our part. It was an intentional decision since it's mostly an optimization, and unclear why it would be important for shallow rendering to do that. Can you explain why it matters?

It’s important for shallow rendering to match production as much as possible, since shallow rendering is a testing tool.

Agree to @ljharb. Even it's optimization I think there might be still case that we want to make sure we've really do the optimization (though not guaranteed in production) in code.

@ljharb @chenesan @gaearon Is this still open to be worked on?

For this code and statement:

    function SomeComponent() {
      const noop = React.useCallback(() => {}, []);

      return (
        <div onClick={noop} />
      );
    }

    const shallowRenderer = createRenderer();
    let firstResult = shallowRenderer.render(<SomeComponent />);
    let secondResult = shallowRenderer.render(<SomeComponent />);

    expect(firstResult).toEqual(secondResult);

If useCallback returned the same callback between two rendering (I think) the assertion should pass.

Isn't it creating two instances, and then rendering each of them independently? AFAIK this is not two renderings of the same, but instead two different elements.

If I have a component rendered in different places those are independent of each other. They keep the same callback between re-renders, but not across all the instances.

The shallow renderer has moved to https://github.com/NMinhNguyen/react-shallow-renderer. If this is still relevant, please feel free to file this issue there. Thanks!