mobxjs / mobx-react

React bindings for MobX
https://mobx.js.org/react-integration.html
MIT License
4.85k stars 350 forks source link

is @action really necessary? #863

Closed PavanGangireddy closed 4 years ago

PavanGangireddy commented 4 years ago

As per the #505 discussion, for the below snippet when the user presses "Get user details" button,

3 update requests - 3 renders should happen

instead of

3 update requests - 2 renders are reflected

import React, { Component } from "react";
import { render } from "react-dom";
import { observable } from "mobx";
import { observer } from "mobx-react";

@observer
class UserProfile extends Component {
  @observable userDetails = null;
  @observable count = 0;
  @observable secondCount = 0;

  // Mobx level batching - 3 update requests - 2 renders
  getUserDetails = async () => {
    this.count = this.count + 1; // forceUpdate() -> render()
    this.secondCount = this.secondCount + 1; // forceUpdate() -> render()
    let promise = new Promise((resolve, reject) => {
      resolve({
        name: "Tom Preston-Werner",
        company: "Facebook"
      });
    });
    let response = await promise;
    this.userDetails = response; // forceUpdate() -> render()
  };

  render() {
    console.log("render UserProfile");
    if (this.userDetails === null) {
      return <button onClick={this.getUserDetails}>Get user details</button>;
    }
    return (
      <div>
        {this.userDetails !== null && (
          <React.Fragment>
            <p>Name: {this.userDetails.name}</p>
            <p>Company: {this.userDetails.company}</p>
          </React.Fragment>
        )}
        <div>Count: {this.count}</div>
        <div>SecondCount: {this.secondCount}</div>
        <button onClick={this.getUserDetails}>Get user details</button>
      </div>
    );
  }
}

render(<UserProfile />, document.getElementById("root"));

Why? explanation needed

danielkcz commented 4 years ago

Wait, are you really complaining that MobX is re-rendering less times? 😆 Good one.

Is something actually broken? In that case please turn this into a runnable example with eg. CodeSandbox.

PavanGangireddy commented 4 years ago

@FredyC

Sandbox

My question is regarding the observation made in #505 by @urugator

The same is true for async function, but similary to mobx, react can't batch deferred updates. Be aware it's an implementation detail and can change in the future. Btw there is another problem with async event handlers. You can't access event object in them, because events are pooled. The react will throw if you attempt to do so.

No complaining. Just trying to understand more :-).

mweststrate commented 4 years ago

React might still batch, even when mobx doesn't. The code before first await is executed during the event handler, hence batched by react.

Op za 9 mei 2020 10:19 schreef Pavan Gangireddy notifications@github.com:

@FredyC https://github.com/FredyC

Sandbox https://codesandbox.io/s/mobx5-observable-objects-inb85?file=/src/index.js

My question is regarding the observation made in #505 https://github.com/mobxjs/mobx-react/issues/505 by @urugator https://github.com/urugator

The same is true for async function, but similary to mobx, react can't batch deferred updates. Be aware it's an implementation detail and can change in the future. Btw there is another problem with async event handlers. You can't access event object in them, because events are pooled. The react will throw if you attempt to do so.

  • So as per above statement, react should not batch

this.count = this.count + 1; // forceUpdate() -> render() this.secondCount = this.secondCount + 1; // forceUpdate() -> render()

as they are inside an async function. Which is contrary in the example.

No complaining. Just trying to understand more :-).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/863#issuecomment-626135283, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBE2PGWIGLY32E7KSRLRQUN35ANCNFSM4M4WXCAA .

PavanGangireddy commented 4 years ago

Thanks @mweststrate @FredyC for the quick reply.

Also comment made by @urugator clarified it more.here

The async function runs "synchronously" until it hits the first await, so the first the 2 updates are probably batched by react (as they run from within an event handler).

Closing it.