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

fix(pretty-format): Render custom displayName of memoized components #8546

Merged
merged 2 commits into from
Jun 11, 2019
Merged

fix(pretty-format): Render custom displayName of memoized components #8546

merged 2 commits into from
Jun 11, 2019

Conversation

gustavovnicius
Copy link
Contributor

@gustavovnicius gustavovnicius commented Jun 10, 2019

Summary

Currently, the way jest and enzyme handle memoized components is out of sync. This leads to some snapshot tests rendering the incorrect memoized component's displayName. e.g.: while using the latest react-redux version, some connected components are memoized and they respect the custom displayName, it can be checked here: https://github.com/reduxjs/react-redux/blob/master/src/components/connectAdvanced.js#L416.

enzyme has fixed it here: https://github.com/airbnb/enzyme/pull/2137/files, notice that it checks for the type's displayName, before going to the type's type displayName.

Test plan

Prior to this, when a memoized/connected React.Node is passed as another component's props, the snapshot tests of that component would return <Memo(ConnectFunction)>, now they return the correct component displayName instead: <Memo(Connect(MyComponent))>

You can check the wrong case here: https://github.com/gustavovnicius/enzyme-display-name-bug/blob/master/src/__snapshots__/App.test.js.snap#L6

In the snapshot, it should be body={<Memo(Connect(Body)) />} instead of body={<Memo(ConnectFunction) />}

@codecov-io
Copy link

codecov-io commented Jun 10, 2019

Codecov Report

Merging #8546 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8546   +/-   ##
=======================================
  Coverage   60.57%   60.57%           
=======================================
  Files         269      269           
  Lines       11054    11054           
  Branches     2696     2696           
=======================================
  Hits         6696     6696           
  Misses       3772     3772           
  Partials      586      586
Impacted Files Coverage Δ
packages/pretty-format/src/plugins/ReactElement.ts 86.48% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ac8ca7...474fc07. Read the comment docs.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, can you add a changelog entry? :)

@pedrottimark
Copy link
Contributor

@gustavovnicius Thank you. To make sure that I understand the situation:

  • the test (above yours) from Support React.memo in pretty-format #7891 is correct for React.memo by itself?
  • the test that you wrote uses Object.assign to emulate the displayName property when a connected component uses React.memo as an implementation detail? that is, to test the correct behavior without requiring react-redux package as a devDependency? If the answer is yes, then super craftsmanship, and I think a short comment in the test will help our future selves :)

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing changelog, otherwise LGTM!

@gustavovnicius
Copy link
Contributor Author

gustavovnicius commented Jun 11, 2019

@gustavovnicius Thank you. To make sure that I understand the situation:

  • the test (above yours) from Support React.memo in pretty-format #7891 is correct for React.memo by itself?
  • the test that you wrote uses Object.assign to emulate the displayName property when a connected component uses React.memo as an implementation detail? that is, to test the correct behavior without requiring react-redux package as a devDependency? If the answer is yes, then super craftsmanship, and I think a short comment in the test will help our future selves :)

@pedrottimark >
1 - Yes, however the test from #7891 doesn't consider a custom displayName property
2 - Mostly it's a shorthand alternative for:

const MemoFoo = React.memo(Foo);
MemoFoo.displayName = 'MyDisplayName(Foo)';

Thinking back now, I believe it's better written without Object.assign, less cognitive burden :) I'll change it.
Also, there's no need to import react-redux, this fix is for any component decorated with React.memo and a custom displayName.

@gustavovnicius gustavovnicius changed the title fix(pretty-format): Render displayName of memoized components fix(pretty-format): Render custom displayName of memoized components Jun 11, 2019
@gustavovnicius
Copy link
Contributor Author

I added some more test cases, so I hope now it's a bit clearer

@SimenB SimenB merged commit be1dda3 into jestjs:master Jun 11, 2019
@SimenB
Copy link
Member

SimenB commented Jun 11, 2019

Thanks!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants