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

Bug fix - SetState callback called before component state is updated in ReactShallowRenderer #11507

Merged
merged 17 commits into from Nov 22, 2017

Conversation

Projects
None yet
3 participants
@accordeiro
Copy link
Contributor

accordeiro commented Nov 9, 2017

This PR aims to fix the bug described in #11496

A test was created to reproduce the error, and to fix it, the ReactShallowRenderer callback calling logic was changed, so that it would stop calling the callbacks after the enqueue* functions, and would start calling them after finishing mounting or updating.

Please let me know if there any questions or if anything should be changed / improved :)

@accordeiro

This comment has been minimized.

Copy link
Contributor Author

accordeiro commented Nov 9, 2017

Hmm something is going wrong with the tests, I'm gonna check this.


_invokeCallback() {
if (typeof this._callback === 'function' && this._publicInstance) {
this._callback.call(this._publicInstance);

This comment has been minimized.

Copy link
@gaearon

gaearon Nov 9, 2017

Member

You'll probably want to immediately set it to null before calling. Since now it is outdated.

this._publicInstance = null;
}

_updateCallback(callback, publicInstance) {

This comment has been minimized.

Copy link
@gaearon

gaearon Nov 9, 2017

Member

Maybe _enqueueCallback

@accordeiro

This comment has been minimized.

Copy link
Contributor Author

accordeiro commented Nov 9, 2017

Thanks for the review @gaearon – sending a push with the changes in a bit.

BTW, I'm not sure what's going on with the CircleCI tests, all of them seem to be passing by looking at the log. Is this a CI issue, or something I should address?

@accordeiro

This comment has been minimized.

Copy link
Contributor Author

accordeiro commented Nov 9, 2017

I've just pushed the requested changes.

The CI tests are stating that I didn't run prettier for packages/react-test-renderer/src/ReactShallowRenderer.js (which I did). I ran a full yarn prettier-all and there are no suggested changes for ReactShallowRenderer.js either – what should I do?

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Nov 10, 2017

Maybe you didn't run yarn first? The Prettier version has changed.

@accordeiro

This comment has been minimized.

Copy link
Contributor Author

accordeiro commented Nov 10, 2017

Cool, thanks :)

Looks like all checks have passed now.

@@ -183,6 +184,7 @@ class ReactShallowRenderer {

if (shouldUpdate) {

This comment has been minimized.

Copy link
@gaearon

gaearon Nov 10, 2017

Member

Does ReactDOM call the callback if shouldComponentUpdate skips an update? I would expect so, but this PR doesn't. Can you verify this?

Maybe it's better to move the callback call here. Then you don't need to duplicate it in two branches.

@accordeiro

This comment has been minimized.

Copy link
Contributor Author

accordeiro commented Nov 10, 2017

OK, I've moved the callback call.

Regarding the behavior when shouldComponentUpdate skips an update, I'm not sure how I can test this. I've written this following test:

it('setState callback should be called even if update is skipped', () => {
    let stateSuccessfullyUpdated = false;

    class Component extends React.Component {
      constructor(props, context) {
        super(props, context);
        this.state = {
          hasUpdatedState: false,
        };
      }

      shouldComponentUpdate() {
        return false;
      }

      componentWillMount() {
        this.setState(
          {hasUpdatedState: true},
          () => stateSuccessfullyUpdated = this.state.hasUpdatedState,
        );
      }

      render() {
        return <div>{this.state.hasUpdatedState}</div>;
      }
    }

    const shallowRenderer = createRenderer();
    shallowRenderer.render(<Component />);
    expect(stateSuccessfullyUpdated).toBe(true);
  });

... and the callback is indeed being called, but I suspect this isn't enough for the code to even trigger a shouldComponentUpdate() check. I've thought about rendering the component, then triggering an event to start an update cycle, which would then call setState- but I'm afraid I'm overcomplicating things. Any suggestions? Thanks!

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Nov 10, 2017

OK, I've moved the callback call.

Note I don't know if that's how it works. Can you please check whether ReactDOM calls the callback in this case or not first?

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Nov 10, 2017

I've thought about rendering the component, then triggering an event to start an update cycle, which would then call setState- but I'm afraid I'm overcomplicating things.

You could call getMountedInstance() and then just call setState on it from outside.

@accordeiro

This comment has been minimized.

Copy link
Contributor Author

accordeiro commented Nov 13, 2017

Just wanted to follow up and say I'm working on this today :)

@accordeiro

This comment has been minimized.

Copy link
Contributor Author

accordeiro commented Nov 13, 2017

Note I don't know if that's how it works. Can you please check whether ReactDOM calls the callback in this case or not first?

It seems like ReactDOM does call the setState callback even if an update is skipped – I've just written some tests for both ReactShallowRenderer and ReactDOM to ensure their behavior is consistent. Do these tests make sense to you?


componentWillMount() {
setState = (newState, callback) => this.setState(newState, callback);
getState = () => this.state;

This comment has been minimized.

Copy link
@gaearon

gaearon Nov 13, 2017

Member

These helpers make it a bit hard to read what's really going on in the test. Could you please just store the instance instead, and then read state and call setState on it directly?

let setState, getState;

const div = document.createElement('div');
document.body.appendChild(div);

This comment has been minimized.

Copy link
@gaearon

gaearon Nov 13, 2017

Member

I don't think we need to add it to the body for this test.

}
}

_invokeCallback() {

This comment has been minimized.

Copy link
@gaearon

gaearon Nov 13, 2017

Member

Maybe _invokeCallbackIfNecessary? It doesn't always exist.

@accordeiro

This comment has been minimized.

Copy link
Contributor Author

accordeiro commented Nov 13, 2017

Done! Just pushed the requested changes.

expect(mockFn).not.toBeCalled();

instance.setState({hasUpdatedState: true}, () => {
expect(mockFn).toBeCalled();

This comment has been minimized.

Copy link
@gaearon

gaearon Nov 13, 2017

Member

This doesn't verify that the callback itself has been called.

@gaearon
Copy link
Member

gaearon left a comment

I'll take it from here. Thanks!

@accordeiro

This comment has been minimized.

Copy link
Contributor Author

accordeiro commented Nov 13, 2017

Awesome, thanks! :)

@gaearon
Copy link
Member

gaearon left a comment

I have a few more concerns here.

@accordeiro

This comment has been minimized.

Copy link
Contributor Author

accordeiro commented Nov 14, 2017

OK, I'll investigate this. I will probably only be able to follow up later this week – is this ok?

cvburgess added a commit to usePF/perch-data that referenced this pull request Dec 5, 2017

Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017

Bug fix - SetState callback called before component state is updated …
…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
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.