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

accordeiro · 2017-11-09T19:12:57Z

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 :)

…#11496)

accordeiro · 2017-11-09T19:24:25Z

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

gaearon · 2017-11-09T19:27:24Z

packages/react-test-renderer/src/ReactShallowRenderer.js

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


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

gaearon · 2017-11-09T19:27:57Z

packages/react-test-renderer/src/ReactShallowRenderer.js

+    this._publicInstance = null;
+  }
+
+  _updateCallback(callback, publicInstance) {


Maybe _enqueueCallback

accordeiro · 2017-11-09T19:34:50Z

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 · 2017-11-09T20:05:17Z

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 · 2017-11-10T09:23:48Z

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

accordeiro · 2017-11-10T12:50:04Z

Cool, thanks :)

Looks like all checks have passed now.

gaearon · 2017-11-10T13:45:42Z

packages/react-test-renderer/src/ReactShallowRenderer.js

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

    if (shouldUpdate) {


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 · 2017-11-10T15:42:30Z

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 · 2017-11-10T15:56:10Z

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 · 2017-11-10T15:58:01Z

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 · 2017-11-13T12:26:17Z

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

…Renderer

accordeiro · 2017-11-13T13:27:03Z

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?

gaearon · 2017-11-13T14:26:27Z

packages/react-dom/src/__tests__/ReactDOM-test.js

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


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?

gaearon · 2017-11-13T14:26:39Z

packages/react-dom/src/__tests__/ReactDOM-test.js

+    let setState, getState;
+
+    const div = document.createElement('div');
+    document.body.appendChild(div);


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

gaearon · 2017-11-13T14:27:10Z

packages/react-test-renderer/src/ReactShallowRenderer.js

+    }
+  }
+
+  _invokeCallback() {


Maybe _invokeCallbackIfNecessary? It doesn't always exist.

accordeiro · 2017-11-13T15:11:39Z

Done! Just pushed the requested changes.

gaearon · 2017-11-13T15:23:29Z

packages/react-dom/src/__tests__/ReactDOM-test.js

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


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

gaearon

I'll take it from here. Thanks!

accordeiro · 2017-11-13T15:32:29Z

Awesome, thanks! :)

gaearon

I have a few more concerns here.

This doesn't seem to handle multiple callbacks correctly. Can this situation happen in practice? I'm not sure. This is how the old Stack reconciler (which is somewhat similar to the shallow renderer) used to handle it: https://github.com/facebook/react/blob/15-stable/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js#L134-L138
These lines from the Stack reconciler also surprised me: https://github.com/facebook/react/blob/15-stable/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js#L139-L143. I vaguely remember reading about this issue. Can you research this? Did React not call the setState callbacks scheduled from componentWillMount at all in 15? Did it start to do it in 16? Because if we don't do it in 16 either, we should probably change the shallow renderer to ignore them. Otherwise we should change the ReactDOM test to verify this specific case.

accordeiro · 2017-11-14T12:33:56Z

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

Patch this React bug: - facebook/react#1740 -facebook/react#11507

…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

…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

accordeiro added 2 commits November 9, 2017 16:41

Create test to verify ReactShallowRenderer bug (facebook#11496)

035eb3d

Fix ReactShallowRenderer callback bug on componentWillMount (facebook…

856cb2b

…#11496)

facebook-github-bot added the CLA Signed label Nov 9, 2017

gaearon reviewed Nov 9, 2017

View reviewed changes

accordeiro added 2 commits November 9, 2017 17:45

Improve fnction naming and clean up queued callback before call

a269747

Merge branch 'master' into shallow-renderer-bug

a13a3d6

Run prettier on ReactShallowRenderer.js

11b931d

gaearon reviewed Nov 10, 2017

View reviewed changes

Consolidate callback call on ReactShallowRenderer.js

ae88abd

Ensure callback behavior is similar between ReactDOM and ReactShallow…

87bf19f

…Renderer

gaearon reviewed Nov 13, 2017

View reviewed changes

Fix Code Review requests (facebook#11507)

120b399

gaearon reviewed Nov 13, 2017

View reviewed changes

Move test to ReactCompositeComponent

cfa6552

gaearon approved these changes Nov 13, 2017

View reviewed changes

Verify the callback gets called

ffa5565

gaearon requested changes Nov 13, 2017

View reviewed changes

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

Fix for setState callback

1deeacf

Patch this React bug: - facebook/react#1740 -facebook/react#11507

renovate bot mentioned this pull request Feb 22, 2018

Update react monorepo packages to v16.2.0 mozilla/ensemble#28

Closed

This was referenced Mar 1, 2018

Update react-dom in / from 16.0.0 to 16.2.0 pcardune/redux-firebase-mirror#27

Open

Update react-test-renderer in / from 16.0.0 to 16.2.0 pcardune/redux-firebase-mirror#28

Open

dependabot-preview bot mentioned this pull request Mar 15, 2018

Update react requirement to 16.2.0 shankarshastri/GreenKeeperTest#4

Closed

This was referenced Mar 21, 2018

Update react to 16.3.2-abandoned sbardian/DebtTrackerAPI#15

Closed

Update react-dom to ^16.0.0-abandoned sbardian/DebtTrackerAPI#18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

accordeiro commented Nov 9, 2017

accordeiro commented Nov 9, 2017

gaearon Nov 9, 2017

gaearon Nov 9, 2017

accordeiro commented Nov 9, 2017

accordeiro commented Nov 9, 2017

gaearon commented Nov 10, 2017

accordeiro commented Nov 10, 2017

gaearon Nov 10, 2017

accordeiro commented Nov 10, 2017

gaearon commented Nov 10, 2017

gaearon commented Nov 10, 2017

accordeiro commented Nov 13, 2017

accordeiro commented Nov 13, 2017

gaearon Nov 13, 2017

gaearon Nov 13, 2017

gaearon Nov 13, 2017

accordeiro commented Nov 13, 2017

gaearon Nov 13, 2017

gaearon left a comment

accordeiro commented Nov 13, 2017

gaearon left a comment

accordeiro commented Nov 14, 2017

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

		if (shouldUpdate) {

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

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

Conversation

accordeiro commented Nov 9, 2017

accordeiro commented Nov 9, 2017

gaearon Nov 9, 2017

Choose a reason for hiding this comment

gaearon Nov 9, 2017

Choose a reason for hiding this comment

accordeiro commented Nov 9, 2017

accordeiro commented Nov 9, 2017

gaearon commented Nov 10, 2017

accordeiro commented Nov 10, 2017

gaearon Nov 10, 2017

Choose a reason for hiding this comment

accordeiro commented Nov 10, 2017

gaearon commented Nov 10, 2017

gaearon commented Nov 10, 2017

accordeiro commented Nov 13, 2017

accordeiro commented Nov 13, 2017

gaearon Nov 13, 2017

Choose a reason for hiding this comment

gaearon Nov 13, 2017

Choose a reason for hiding this comment

gaearon Nov 13, 2017

Choose a reason for hiding this comment

accordeiro commented Nov 13, 2017

gaearon Nov 13, 2017

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

accordeiro commented Nov 13, 2017

gaearon left a comment

Choose a reason for hiding this comment

accordeiro commented Nov 14, 2017