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

Add test to repro problem with mocking out 'getDerivedStateFromProps' #6000

Closed

Conversation

flarnie
Copy link
Contributor

@flarnie flarnie commented Apr 16, 2018

what is the change?:
Soon folks will be adopting the new 'getDerivedStateFromProps' in their
React components, and re-running existing tests.

If those components are mocked in existing tests, then React will throw
errors currently.

Why? Because React validates that any component defining this method has
state and returns the right thing from 'gDSFP', and when an auto-mock is
created it doesn't fit these invariants.

Talking through this with @mael and @mjesun we determined an easy
solution is to add a small conditional to either blacklist mocking
functions with this name, so that automocks just don't have this method
and the invariant is never checked in React.

An alternative solution would be to make the mocked 'gDSFP' method
return 'null' and define 'state' on the mocked component.

why make this change?:
Let's work together to make Jest and React work together incredibly
smoothly, like a sea of butter. ^_^

test plan:
This test will throw errors right now - tagging @mjesun to tweak Jest so
that the new test will pass correctly.

screen shot 2018-04-15 at 6 35 52 pm

**what is the change?:**
Soon folks will be adopting the new 'getDerivedStateFromProps' in their
React components, and re-running existing tests.

If those components are mocked in existing tests, then React will throw
errors currently.

Why? Because React validates that any component defining this method has
state and returns the right thing from 'gDSFP', and when an auto-mock is
created it doesn't fit these invariants.

Talking through this with @mael and @mjesun we determined an easy
solution is to add a small conditional to either blacklist mocking
functions with this name, so that automocks just don't have this method
and the invariant is never checked in React.

An alternative solution would be to make the mocked 'gDSFP' method
return 'null' and define 'state' on the mocked component.

**why make this change?:**
Let's work together to make Jest and React work together incredibly
smoothly, like a sea of butter. ^_^

**test plan:**
This test will throw errors right now - tagging @mjesun to tweak Jest so
that the new test will pass correctly.

(Flarnie will insert screenshot of the errors thrown)
@mjesun
Copy link
Contributor

mjesun commented Apr 16, 2018

Whoa, congrats on #6000 PR! We'll take it from here :)

@mjesun
Copy link
Contributor

mjesun commented Apr 16, 2018

Hi! Tried pushing a commit to your branch but got permission denied 😔

Aside from the linting errors and a small typo proccess ➡️ process, the rest of the test passes (at least on my machine). Can you double-check that?

@mjesun
Copy link
Contributor

mjesun commented Apr 16, 2018

Nevermind, i was able to reproduce it. Can you enable pushes to your branch, though? Thanks! :)

@flarnie
Copy link
Contributor Author

flarnie commented Apr 16, 2018

I had thought checking 'enable edits from maintainers' would allow you to push to the branch.
screen shot 2018-04-16 at 1 05 10 pm

Invited you as a collaborator on my fork also for good measure, and thanks for your help!

@codecov-io
Copy link

Codecov Report

Merging #6000 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6000   +/-   ##
=======================================
  Coverage   64.32%   64.32%           
=======================================
  Files         217      217           
  Lines        8312     8312           
  Branches        3        4    +1     
=======================================
  Hits         5347     5347           
  Misses       2964     2964           
  Partials        1        1

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 70e7ac2...ac26a1a. Read the comment docs.

@mjesun mjesun mentioned this pull request Apr 17, 2018
@cpojer
Copy link
Member

cpojer commented Apr 17, 2018

Closing for #6011

@cpojer cpojer closed this Apr 17, 2018
@SimenB SimenB reopened this Apr 17, 2018
@SimenB
Copy link
Member

SimenB commented Aug 15, 2018

@mjesun do we still need to do something here?

@mjesun
Copy link
Contributor

mjesun commented Aug 15, 2018

Don't think so; we agreed on fixing this somewhere else.

@mjesun mjesun closed this Aug 15, 2018
@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 12, 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