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

shallow renderer: wrong initial state passed to getDerivedStateFromProps #15075

Closed
ljharb opened this Issue Mar 9, 2019 · 11 comments

Comments

Projects
None yet
4 participants
@ljharb
Copy link
Contributor

ljharb commented Mar 9, 2019

Demonstrated in airbnb/enzyme#2027 - when gDSFP returns a state change, cDU does not seem to be called by the shallow renderer. I'm not sure how to make a simple test case; but the identical enzyme tests pass for mount (ie, when using React itself) but fail for shallow (when using the shallow renderer). enzyme doesn't have any logic for calling or interacting with getDerivedStateFromProps, so I suspect it's not an issue with enzyme (but it might be!)

@miraage

This comment has been minimized.

Copy link
Contributor

miraage commented Mar 10, 2019

Could you please setup a sandbox?

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Mar 12, 2019

Happy to take a bugfix if you think it's a bug. (See existing shallow renderer tests)

@ljharb

This comment has been minimized.

Copy link
Contributor Author

ljharb commented Mar 15, 2019

I'm actually seeing two issues here:

https://codesandbox.io/s/m7nn4v428 shows the behavior of React itself:

  1. Demo's gDSFP is called, returns null
    NOTE: called with second argument as Demo's initial this.state
  2. Demo component renders (via SetProps component)
  3. Demo's props are updated (by SetProps)
  4. Demo's gDSFP is called, returns null
  5. rerenders
  6. cDU is called

https://codesandbox.io/s/mqm4o7m7nx shows the behavior of the shallow renderer:

  1. Demo's gDSFP is called, returns null
    NOTE: called with second argument as undefined
  2. Demo component renders (via shallow renderer render call)
  3. Demo's props are updated (by another shallow renderer render call)
  4. Demo's gDSFP is called, returns null
  5. rerenders
  6. cDU is NOT called

(I'm not sure where to start with a PR for these fixes; if someone can help by either implementing the fix, or sharing a branch with failing tests so i can build a fix on top of that, or a direct link to where the tests and fix would go, that would be great)

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Mar 18, 2019

Here's an example of a recent gDSFP fix, it shows where to change the code and where the tests go.
https://github.com/facebook/react/pull/14613/files

@peanutenthusiast

This comment has been minimized.

Copy link

peanutenthusiast commented Mar 18, 2019

I'll look into this.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Mar 19, 2019

Looking at it again — this works as intended.

Shallow renderer has never fired componentDidUpdate at all, similar to how it never fired componentDidMount. The reason is that most componentDidMount and componentDidUpdate methods have side effects or deal with refs, and so they break shallow rendering assumptions anyway.

There is a bigger discussion to have about the future status of shallow renderer in and its design, but the reported issue is not a bug.

@gaearon gaearon closed this Mar 19, 2019

@ljharb

This comment has been minimized.

Copy link
Contributor Author

ljharb commented Mar 19, 2019

@gaearon note as well the discrepancy in the first call to gDSFP - should i file a new issue, or can this be reopened?

@gaearon gaearon reopened this Mar 19, 2019

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Mar 19, 2019

Oops, missed that. Thanks.

@gaearon gaearon changed the title shallow renderer: when getDerivedStateFromProps changes state, componentDidUpdate is not called. shallow renderer: wrong initial state passed to getDerivedStateFromProps Mar 19, 2019

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Mar 19, 2019

Hmm, I can't repro with https://codesandbox.io/s/mqm4o7m7nx:

Screen Shot 2019-03-19 at 10 15 01 PM

I don't see undefined.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Mar 19, 2019

Is it only if you omit this.state assignment in constructor? This normally warns. (Although not in shallow.) I see a difference in null vs undefined for this case but I don't know how to repro {} vs undefined.

@ljharb

This comment has been minimized.

Copy link
Contributor Author

ljharb commented Mar 24, 2019

Hmm - now I can't reproduce it even on my original sandboxes :-/ I see the same minor discrepancy as you when I comment out the state assignment in the Demo constructor, and while that might be nice to fix it's probably not worth the effort.

I'm going to close this - I'll comment here, or file a new issue, if I see anything else.

@ljharb ljharb closed this Mar 24, 2019

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