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

useCallback doesn't memoize callback in shallow renderer #15774

Open
chenesan opened this issue May 30, 2019 · 6 comments · May be fixed by #15843
Open

useCallback doesn't memoize callback in shallow renderer #15774

chenesan opened this issue May 30, 2019 · 6 comments · May be fixed by #15843

Comments

@chenesan
Copy link
Contributor

@chenesan chenesan commented May 30, 2019

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

@threepointone

This comment has been minimized.

Copy link
Contributor

@threepointone threepointone commented May 30, 2019

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

@chenesan

This comment has been minimized.

Copy link
Contributor Author

@chenesan chenesan commented May 30, 2019

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

ljharb added a commit to chenesan/enzyme that referenced this issue May 31, 2019
`shallow`: Note that the `get same callback when using `useCallback` and rerender with same prop in dependencies` is skipped because react shallow renderer doesn't memoize callback function value. Pending facebook/react#15774
ljharb added a commit to chenesan/enzyme that referenced this issue May 31, 2019
`shallow`: Note that the `get same callback when using `useCallback` and rerender with same prop in dependencies` is skipped because react shallow renderer doesn't memoize callback function value. Pending facebook/react#15774
@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Jun 8, 2019

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?

@chenesan chenesan mentioned this issue Jun 9, 2019
12 of 14 tasks complete
@ljharb

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Jun 9, 2019

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

@chenesan

This comment has been minimized.

Copy link
Contributor Author

@chenesan chenesan commented Jun 9, 2019

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.

@jamesgeorge007

This comment has been minimized.

Copy link
Contributor

@jamesgeorge007 jamesgeorge007 commented Jun 23, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.