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

SetState callback called before component state is updated in React Shallow Renderer #11496

Closed
bdwain opened this issue Nov 9, 2017 · 19 comments

Comments

@bdwain
Copy link

bdwain commented Nov 9, 2017

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

What is the current behavior?
When I call setState with a callback in a test using react shallow renderer (via enzyme), the callback gets called and this.state is still the old state.

EDIT: This seems limited to componentWillMount

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template for React 16: https://jsfiddle.net/Luktwrdm/, template for React 15: https://jsfiddle.net/hmbg7e9w/).

EDIT: Reproduce with this https://github.com/bdwain/setstate-callback-bug

What is the expected behavior?
When the setState callback gets called, it should have access to the new state.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
Version 16.
It worked in v15 with enzyme 2.

EDIT: I think this is because in componentWillMount, this line causes the render method in the shallow wrapper to return early, before it updates the state.

@bdwain bdwain changed the title SetState callback called before component state is updated in React Test Renderer SetState callback called before component state is updated in React Shallow Renderer Nov 9, 2017
@gaearon
Copy link
Collaborator

gaearon commented Nov 9, 2017

Thanks for the report.

Note that using setState callback in componentWillMount doesn't make a lot of sense to me. You should just use conponentDidMount instead it you can.

@bdwain
Copy link
Author

bdwain commented Nov 9, 2017

I agree. I generally never use componentWillMount. The one reason I can see to use componentWillMount is because it lets you share logic that calls setState with other parts of the component. I was just upgrading my project to react 16 and noticed some places where it was used that had this issue. While they all could be implemented a different way, I feel like it's not incorrect and makes sense to behave as expected.

@gaearon
Copy link
Collaborator

gaearon commented Nov 9, 2017

Yeah that's fair!

Wanna take a stab at adding a failing test and fixing it?

@gaearon
Copy link
Collaborator

gaearon commented Nov 9, 2017

I think what needs to be done is that instead of calling the callback in enqueue* methods immediately we should store it on a field, and then call it when we have finished mounting or updating.

@bdwain
Copy link
Author

bdwain commented Nov 9, 2017

Yea I can take a look. I may not have time to finish it today though. Not sure if it’s important to finish before 16.1. Since it’s not too hard to use the constructor or componentdidmount instead I feel like it’s not a huge deal though, especially if a patch release can be done before 16.2. I already changed the spots in my project that were breaking and it was pretty easy.

@gaearon
Copy link
Collaborator

gaearon commented Nov 9, 2017

We've already cut 16.1. Seems okay since it probably was broken in 16.0 too.

@accordeiro
Copy link
Contributor

Hi, @gaearon and @bdwain

I was taking a look at this issue earlier and I might have come up with a test to reproduce this bug and a patch to fix it.

As this is assigned @bdwain I didn’t want to step in his shoes and send a PR, but I’m happy to push the code so you guys can take a look and maybe get some insights – is this ok?

@bdwain
Copy link
Author

bdwain commented Nov 9, 2017

Not a problem. I haven’t had a chance to do anything yet. Lets just go with yours.

@accordeiro
Copy link
Contributor

Thanks @bdwain – just sent a PR :)

gaearon pushed a commit that referenced this issue Nov 22, 2017
…in ReactShallowRenderer (#11507)

* Create test to verify ReactShallowRenderer bug (#11496)

* Fix ReactShallowRenderer callback bug on componentWillMount (#11496)

* Improve fnction naming and clean up queued callback before call

* Run prettier on ReactShallowRenderer.js

* Consolidate callback call on ReactShallowRenderer.js

* Ensure callback behavior is similar between ReactDOM and ReactShallowRenderer

* Fix Code Review requests (#11507)

* Move test to ReactCompositeComponent

* Verify the callback gets called

* Ensure multiple callbacks are correctly handled on ReactShallowRenderer

* Ensure the setState callback is called inside componentWillMount (ReactDOM)

* Clear ReactShallowRenderer callback queue before actually calling the callbacks

* Add test for multiple callbacks on ReactShallowRenderer

* Ensure the ReactShallowRenderer callback queue is cleared after invoking callbacks

* Remove references to internal fields on ReactShallowRenderer test
raphamorim pushed a commit to raphamorim/react that referenced this issue Nov 27, 2017
…in ReactShallowRenderer (facebook#11507)

* Create test to verify ReactShallowRenderer bug (facebook#11496)

* Fix ReactShallowRenderer callback bug on componentWillMount (facebook#11496)

* Improve fnction naming and clean up queued callback before call

* Run prettier on ReactShallowRenderer.js

* Consolidate callback call on ReactShallowRenderer.js

* Ensure callback behavior is similar between ReactDOM and ReactShallowRenderer

* Fix Code Review requests (facebook#11507)

* Move test to ReactCompositeComponent

* Verify the callback gets called

* Ensure multiple callbacks are correctly handled on ReactShallowRenderer

* Ensure the setState callback is called inside componentWillMount (ReactDOM)

* Clear ReactShallowRenderer callback queue before actually calling the callbacks

* Add test for multiple callbacks on ReactShallowRenderer

* Ensure the ReactShallowRenderer callback queue is cleared after invoking callbacks

* Remove references to internal fields on ReactShallowRenderer test
@clemmy
Copy link
Contributor

clemmy commented Nov 28, 2017

Closed in #11507.

@clemmy clemmy closed this as completed Nov 28, 2017
Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this issue Dec 8, 2017
…in ReactShallowRenderer (facebook#11507)

* Create test to verify ReactShallowRenderer bug (facebook#11496)

* Fix ReactShallowRenderer callback bug on componentWillMount (facebook#11496)

* Improve fnction naming and clean up queued callback before call

* Run prettier on ReactShallowRenderer.js

* Consolidate callback call on ReactShallowRenderer.js

* Ensure callback behavior is similar between ReactDOM and ReactShallowRenderer

* Fix Code Review requests (facebook#11507)

* Move test to ReactCompositeComponent

* Verify the callback gets called

* Ensure multiple callbacks are correctly handled on ReactShallowRenderer

* Ensure the setState callback is called inside componentWillMount (ReactDOM)

* Clear ReactShallowRenderer callback queue before actually calling the callbacks

* Add test for multiple callbacks on ReactShallowRenderer

* Ensure the ReactShallowRenderer callback queue is cleared after invoking callbacks

* Remove references to internal fields on ReactShallowRenderer test
@Norfeldt
Copy link

I had the same issue in React Native, was forced to re-use the new state variable like this:

const newState = "I'm new"
this.setState({ value: newState }, () => console.log(newState))
>> I'm new

If I didn't i would

const newState = "I'm new"
this.setState({ value: newState }, () => console.log(this.state))
>> I'm old
"dependencies": {
    "react": "16.3.0-alpha.1",
    "react-native": "0.54.0",
...

@cuevash
Copy link

cuevash commented May 10, 2018

Using "react": "^16.3.2", I am still experiencing the same problem. The callback to setState has access to the old state..

@ovkhasch
Copy link

I am also experiencing this. setState callback doesn't work as expected. Can you reopen this?

@flq
Copy link

flq commented Jun 5, 2018

Can confirm - only adding that for whatever reason it was working before lifting to 16.4, so it feels like a regression.

@bdwain
Copy link
Author

bdwain commented Jun 5, 2018

I would open a new issue if the bug has resurfaced. Maintainers don't typically look at notifications from closed issues.

@flq
Copy link

flq commented Jun 5, 2018

After some further analysis it seems that it was an unintended interplay with "getDerivedStateFromProps". Still need to get used to the new patterns emerging

@vukers
Copy link

vukers commented Jun 8, 2018

@flq Do you mean this is working as intended, or there is indeed a regression issue?

@flq
Copy link

flq commented Jun 8, 2018

It appears to work as advertised, i.e., the sequence is setSate - getDerivedStateFromProps - setState callback. If you overwrite your state in the „derived“ static, you have to expect weirdness.

@chrisandrewca
Copy link

Anyone still experiencing this behaviour please checkout this article on SO -- https://stackoverflow.com/questions/45339038/setstate-callback-not-waiting-for-state-to-update You may be using the callback argument incorrectly.

NMinhNguyen referenced this issue in enzymejs/react-shallow-renderer Jan 29, 2020
…in ReactShallowRenderer (#11507)

* Create test to verify ReactShallowRenderer bug (#11496)

* Fix ReactShallowRenderer callback bug on componentWillMount (#11496)

* Improve fnction naming and clean up queued callback before call

* Run prettier on ReactShallowRenderer.js

* Consolidate callback call on ReactShallowRenderer.js

* Ensure callback behavior is similar between ReactDOM and ReactShallowRenderer

* Fix Code Review requests (#11507)

* Move test to ReactCompositeComponent

* Verify the callback gets called

* Ensure multiple callbacks are correctly handled on ReactShallowRenderer

* Ensure the setState callback is called inside componentWillMount (ReactDOM)

* Clear ReactShallowRenderer callback queue before actually calling the callbacks

* Add test for multiple callbacks on ReactShallowRenderer

* Ensure the ReactShallowRenderer callback queue is cleared after invoking callbacks

* Remove references to internal fields on ReactShallowRenderer test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants