Skip to content

[Fiber] Introduce API to opt-out of batching #8661

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

Merged
merged 2 commits into from
Jan 3, 2017

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Dec 30, 2016

unbatchedUpdates reverses the effect of batchedUpdates by resetting the batching context.

This does not affect nested updates, which are always deferred regardless of whether they are inside a batch.

This change is a partial reversion of #8634, which added the ability to trigger nested updates using syncUpdates. I had thought that was the behavior we wanted for top-level mount and unmount, but it turns out all we really need is the ability to opt-out of batchedUpdates. So even though it's a deviation from how Stack works, Fiber does not allow nested updates at all. We may later decide to provide this as an escape hatch, if it helps people migrate to Fiber.

We may decide to provide this ability as an escape hatch in the future.
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change. Cleaner and easier to follow. Small problem with a test but otherwise things look good to me.


ReactNoop.syncUpdates(() => {
instance.setState({ step: 1 });
expect(ReactNoop.getChildren()).toEqual([span(2)]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This expectation won't be evaluated. It could fail and Jest won't catch it because you've already called done in componentDidUpdate above. To resolve this you could set a bool (eg componentDidUpdateCalled = true) in componentDidUpdate above, verify it here, and then call done() at the end of the sync update.

it('nested updates are always deferred, even inside unbatchedUpdates', done => {
  let instance;
  let componentDidUpdateCalled = false;
  class Foo extends React.Component {
    state = { step: 0 };
    componentDidUpdate() {
      if (this.state.step === 1) {
        ReactNoop.unbatchedUpdates(() => {
          // This is a nested state update, so it should not be
          // flushed synchronously, even though we wrapped it
          // in unbatchedUpdates.
          this.setState({ step: 2 });
        });
        expect(ReactNoop.getChildren()).toEqual([span(1)]);
        componentDidUpdateCalled = true;
      }
    }
    render() {
      instance = this;
      return <span prop={this.state.step} />;
    }
  }
  ReactNoop.render(<Foo />);
  ReactNoop.flush();
  expect(ReactNoop.getChildren()).toEqual([span(0)]);

  ReactNoop.syncUpdates(() => {
    instance.setState({ step: 1 });
    expect(componentDidUpdateCalled).toBe(true);
    expect(ReactNoop.getChildren()).toEqual([span(2)]);
    done();
  });
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, good catch!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by pushing to an array like we usually do

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Reverses the effect of batchedUpdates by resetting the current
batching context.

Does not affect nested updates, which are always deferred regardless
of whether they are inside a batch.
@acdlite acdlite force-pushed the fiberunbatchedupdates branch from 5a4bbc5 to ad7bcdf Compare January 3, 2017 18:24
@acdlite acdlite merged commit d658ee8 into facebook:master Jan 3, 2017
expect(container2.textContent).toEqual('Hello');
ReactDOM.unmountComponentAtNode(container2);
expect(container2.textContent).toEqual('');
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've changed this behavior so you had to update but you didn't replace it with an equivalent test anywhere. This is just less test coverage instead of testing the new behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants