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

React 16 shallow renderer batched updates discussion #10355

Closed
bvaughn opened this Issue Aug 2, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@bvaughn
Contributor

bvaughn commented Aug 2, 2017

React 16 comes with a new, completely rewritten shallow renderer. The new renderer doesn't currently provide an unstable_batchedUpdates method. This issue is for discussion about whether it should implement that method.

cc @flarnie and @lelandrichardson who recently discussed wrt its potential impact on Enzyme.

cc @koba04 who initially added the integration with Enzyme and who also provided an example implementation.

@gaearon gaearon referenced this issue Aug 2, 2017

Closed

React 16 Umbrella ☂️ (and 15.5) #8854

117 of 120 tasks complete
@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Aug 3, 2017

Contributor

Thank you for creating this issue!

I've added it to emulate batchedUpdates in lifecycle methods and event handlers.

#10294 (comment)

This motivation was to support the following case.

class App extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      count: 1
    };
  }
  onClick() {
    this.setState({count: this.state.count + 1});
    // :
    this.setState({count: this.state.count + 1});
  }
  render() {
    return <button onClick={() => this.onClick()}>click</button>;
  }
}

I guess supporting batched updates is not hard work. koba04@ca78c92
But it can't support the case calling ReactDOM.unstable_batchedUpdates directly.
I'm not sure but we might be able to support the case by using ReactGenericBatching.

I wonder if we should support consistent behaviors between shallow renderer and other fiber renderers.
Currently, shallow renderer isn't supporting ref and some lifecycle methods.
Should shallow renderer support componentDidCatch and ReactDOM.activeUpdates?

I think keeping consistent behaviors between the renderers will become harder after released Fiber.
So I don't think we should support them.
(It might be possible if we could implement shallow renderer based on test renderer because test renderer is based on FiberReconciler, which seems to be able to support features Fiber provides easily)

But enzyme also does that works like batched updates and supporting all lifecycle methods and many developers use shallow renderer through shallow enzyme provides.
So it makes sense to expose unstable_batchedUpdates for backward compatibilities of enzyme.

Contributor

koba04 commented Aug 3, 2017

Thank you for creating this issue!

I've added it to emulate batchedUpdates in lifecycle methods and event handlers.

#10294 (comment)

This motivation was to support the following case.

class App extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      count: 1
    };
  }
  onClick() {
    this.setState({count: this.state.count + 1});
    // :
    this.setState({count: this.state.count + 1});
  }
  render() {
    return <button onClick={() => this.onClick()}>click</button>;
  }
}

I guess supporting batched updates is not hard work. koba04@ca78c92
But it can't support the case calling ReactDOM.unstable_batchedUpdates directly.
I'm not sure but we might be able to support the case by using ReactGenericBatching.

I wonder if we should support consistent behaviors between shallow renderer and other fiber renderers.
Currently, shallow renderer isn't supporting ref and some lifecycle methods.
Should shallow renderer support componentDidCatch and ReactDOM.activeUpdates?

I think keeping consistent behaviors between the renderers will become harder after released Fiber.
So I don't think we should support them.
(It might be possible if we could implement shallow renderer based on test renderer because test renderer is based on FiberReconciler, which seems to be able to support features Fiber provides easily)

But enzyme also does that works like batched updates and supporting all lifecycle methods and many developers use shallow renderer through shallow enzyme provides.
So it makes sense to expose unstable_batchedUpdates for backward compatibilities of enzyme.

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Aug 3, 2017

Contributor

This is off topic, but I'd like to add all lifecycle methods support in a future release even though it is a breaking change.

Contributor

koba04 commented Aug 3, 2017

This is off topic, but I'd like to add all lifecycle methods support in a future release even though it is a breaking change.

@bvaughn

This comment has been minimized.

Show comment
Hide comment
@bvaughn

bvaughn Aug 3, 2017

Contributor

Thanks for the feedback! 😁

But it can't support the case calling ReactDOM.unstable_batchedUpdates directly.

I'm a little confused by the mention of the ReactDOM renderer. The shallow renderer is totally decoupled from DOM renderer.

I wonder if we should support consistent behaviors between shallow renderer and other fiber renderers. Currently, shallow renderer isn't supporting ref and some lifecycle methods.

It doesn't make sense for shallow renderer to support things like refs or componentDidMount since nothing is actually mounted and no underlying views are created. Shallow renderer has a narrower use-case.

Should shallow renderer support componentDidCatch and ReactDOM.activeUpdates?

Shallow renderer only renders one level deep. componentDidCatch only applies to errors thrown by lifecycle methods on children. That means componentDidCatch is a no-op for shallow renderer. (This test does a good job of illustrating this.)

(It might be possible if we could implement shallow renderer based on test renderer because test renderer is based on FiberReconciler, which seems to be able to support features Fiber provides easily)

I think this is prohibitively difficult (to make the fiber reconciler behave as "shallow"). I tried it initially before just writing shallow renderer as a stand-alone implementation.

This is off topic, but I'd like to add all lifecycle methods support in a future release even though it is a breaking change.

I think this may cause more harm than good. Calling componentDidMount is likely to trigger code that expects to be able to use things like findDOMNode or refs- neither of which will work with the new shallow renderer. Better to not call that lifecycle method at all and avoid adding potential pitfalls or complication to user component code.

Contributor

bvaughn commented Aug 3, 2017

Thanks for the feedback! 😁

But it can't support the case calling ReactDOM.unstable_batchedUpdates directly.

I'm a little confused by the mention of the ReactDOM renderer. The shallow renderer is totally decoupled from DOM renderer.

I wonder if we should support consistent behaviors between shallow renderer and other fiber renderers. Currently, shallow renderer isn't supporting ref and some lifecycle methods.

It doesn't make sense for shallow renderer to support things like refs or componentDidMount since nothing is actually mounted and no underlying views are created. Shallow renderer has a narrower use-case.

Should shallow renderer support componentDidCatch and ReactDOM.activeUpdates?

Shallow renderer only renders one level deep. componentDidCatch only applies to errors thrown by lifecycle methods on children. That means componentDidCatch is a no-op for shallow renderer. (This test does a good job of illustrating this.)

(It might be possible if we could implement shallow renderer based on test renderer because test renderer is based on FiberReconciler, which seems to be able to support features Fiber provides easily)

I think this is prohibitively difficult (to make the fiber reconciler behave as "shallow"). I tried it initially before just writing shallow renderer as a stand-alone implementation.

This is off topic, but I'd like to add all lifecycle methods support in a future release even though it is a breaking change.

I think this may cause more harm than good. Calling componentDidMount is likely to trigger code that expects to be able to use things like findDOMNode or refs- neither of which will work with the new shallow renderer. Better to not call that lifecycle method at all and avoid adding potential pitfalls or complication to user component code.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 3, 2017

Member

I do wish for some consistency though. It’s odd we call componentDidUpdate but don’t call componentDidMount. Not calling either would make more sense to me.

Member

gaearon commented Aug 3, 2017

I do wish for some consistency though. It’s odd we call componentDidUpdate but don’t call componentDidMount. Not calling either would make more sense to me.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Aug 3, 2017

Member

I don't think we should call componentDidUpdate.

Member

sophiebits commented Aug 3, 2017

I don't think we should call componentDidUpdate.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 3, 2017

Member

We still have time to break this. 😄

Member

gaearon commented Aug 3, 2017

We still have time to break this. 😄

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 3, 2017

Member

I sent a PR for this: #10372

Member

gaearon commented Aug 3, 2017

I sent a PR for this: #10372

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Aug 4, 2017

Contributor

@bvaughn
Thank you for the reply!
I agree with you.

I do wish for some consistency though. It’s odd we call componentDidUpdate but don’t call componentDidMount. Not calling either would make more sense to me.

It makes sense.
I think we should add this into the documentation. Are you gonna work on updating the documentation for v16 before the release?

We are already having test renderer, I can use it if I need ref and all lifecycle methods.
It's a good way to use shallow renderer for a narrower use-case.
Shallow renderer is already convenient for testing behaviors of render function of a component.

When shallow renderer was released, it's an only way to test components without DOM.
I depended on it too heavy, but now I can use test renderer for that case.
I think it would be nice if there is a section for test renderer in documentation as well as shallow renderer.

Contributor

koba04 commented Aug 4, 2017

@bvaughn
Thank you for the reply!
I agree with you.

I do wish for some consistency though. It’s odd we call componentDidUpdate but don’t call componentDidMount. Not calling either would make more sense to me.

It makes sense.
I think we should add this into the documentation. Are you gonna work on updating the documentation for v16 before the release?

We are already having test renderer, I can use it if I need ref and all lifecycle methods.
It's a good way to use shallow renderer for a narrower use-case.
Shallow renderer is already convenient for testing behaviors of render function of a component.

When shallow renderer was released, it's an only way to test components without DOM.
I depended on it too heavy, but now I can use test renderer for that case.
I think it would be nice if there is a section for test renderer in documentation as well as shallow renderer.

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Aug 4, 2017

Contributor

I'm a little confused by the mention of the ReactDOM renderer. The shallow renderer is totally decoupled from DOM renderer.

This is my misunderstanding. You are right.

Contributor

koba04 commented Aug 4, 2017

I'm a little confused by the mention of the ReactDOM renderer. The shallow renderer is totally decoupled from DOM renderer.

This is my misunderstanding. You are right.

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Sep 15, 2017

Contributor

Shallow renderer does not implement unstable_batchedUpdates() anymore.

#10294

This issue can be closed.

Contributor

koba04 commented Sep 15, 2017

Shallow renderer does not implement unstable_batchedUpdates() anymore.

#10294

This issue can be closed.

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