Add support for unstable_batchedUpdates when used with react-dom / react-native

[gsabanov]

The issue:
I suspect mobx-react-lite observer is handling actions/transactions and subsequent re-rendering differently to mobx-react. I don't know if the behaviour is intended and i am doing something wrong, or if its a bug so i would like to clarify this.

Expected behaviour (the mobx-react way):

• When an action is modifying the state of one or more models, all changes should be executed before an update (and re-render) is triggered on any affected components

export const ParentModel = types.model({
    title: "Parent",
    toggler: false,
    date: "",
    child: types.optional(ChildModel, {})
}).actions(self => ({
    toggleWithSideEffect(){
        self.child.date = new Date().toString();
        self.date = new Date().toString();
        self.toggler = !self.toggler; 
        // no rendering should have been done until this point  
        console.log("i toggle")
    },
}));

Actual behavior (the mobx-react-lite way)

• Certain combinations of state modifications cause updates to the state and re-render components in the middle of an action execution (transaction)

export const ParentModel = types.model({
    title: "Parent",
    toggler: false,
    date: "",
    child: types.optional(ChildModel, {})
}).actions(self => ({
    toggleWithSideEffect(){
        // child component referencing this value gets re-rendered after this statement
        self.child.date = new Date().toString();       
        self.date = new Date().toString();
        self.toggler = !self.toggler;
        // no rendering should have been done until this point
        console.log("i toggle")        
    },
}));

I created a (misspelled) repo that demonstrates the issue.

If you run the app in that repo, clicking on the toggle button will work the first time, but will crash the second time because the child model is using a computed value dependent on the toggler value in the parent model, that becomes null on the second iteration, and the child component re-renders before the parent. Running the same code with mobx-react observer behaves as expected, the child component doesn't even try to render when toggler is false.

The issue got weirder while i was composing the repo to test this behaviour.
The idea was to have two react apps next to each other, one with mobx-react one with mobx-react-lite and compare the difference, but loading mobx-react next to mobx-react-lite seems to correct the behaviour, even by just referencing a component that uses the mobx-react observer, see here
I threw in a useContext hook in my test as well to make sure the mobx-react observer isn't overwriting the mobx-react-lite observer when referenced.
Its as if loading mobx-react next to mobx-react-lite enchances/corrects transactional behavior of mobx-react-lite.

The example uses mobx-state-tree, but i was able to produce the same error with a couple of regular mobx nested models.

Here are two branches of the same repo with almost identical code, one with mobx-react and one with mobx-react-lite.

[danielkcz]

For the convenience, I've imported repo to CodeSandbox: https://codesandbox.io/s/github/gsabanov/mobscontext

I will have a look sometimes later, thanks for the report.

[mweststrate]

This is in essence not a MobX problem, but a React problem. By default, react only batches component updates when the updates are triggered from a React event handler. However, you creating your own event listener, bypassing the React event handling.With batching, React does *not* guarantee that parents and children run in the correct order (they render in sync, so whoever gets the update signal first, gets the update first). Using a normal React event on the button probably fixes it. However, that doesn't fix it for all cases, for example, processing incoming messages from a websocket or fetch response has the same problem. The reason it doesn't cause an issue in mobx-react is because we are using a 'hack' to hook into the unofficial API (but stable, many libraries use it for the same reason) to do batching in React, as found here: https://github.com/mobxjs/mobx-react/blob/1e8d6b5009377badcc5ab11ef8a605a071bb60e3/src/index.js#L1-L10 mobx-react-lite doesn't apply that trick atm.

…

On Wed, May 8, 2019 at 1:39 PM Daniel K. ***@***.***> wrote: For the convenience, I've imported repo to CodeSandbox: https://codesandbox.io/s/github/gsabanov/mobscontext I will have a look sometimes later, thanks for the report. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#153 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBF7HTUJT5RGQ547EX3PUK3VJANCNFSM4HLQZG4Q> .

[gsabanov]

This is in essence not a MobX problem, but a React problem. By default, react only batches component updates when the updates are triggered from a React event handler. However, you creating your own event listener, bypassing the React event handling.With batching, React does not guarantee that parents and children run in the correct order (they render in sync, so whoever gets the update signal first, gets the update first). Using a normal React event on the button probably fixes it. However, that doesn't fix it for all cases, for example, processing incoming messages from a websocket or fetch response has the same problem. The reason it doesn't cause an issue in mobx-react is because we are using a 'hack' to hook into the unofficial API (but stable, many libraries use it for the same reason) to do batching in React, as found here: https://github.com/mobxjs/mobx-react/blob/1e8d6b5009377badcc5ab11ef8a605a071bb60e3/src/index.js#L1-L10 mobx-react-lite doesn't apply that trick atm.
…
On Wed, May 8, 2019 at 1:39 PM Daniel K. @.***> wrote: For the convenience, I've imported repo to CodeSandbox: https://codesandbox.io/s/github/gsabanov/mobscontext I will have a look sometimes later, thanks for the report. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#153 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAN4NBF7HTUJT5RGQ547EX3PUK3VJANCNFSM4HLQZG4Q .

Ah perfect, that is exactly it, in our application we actually managed to get this effect without own event listeners (this was just in this demo where i was going to have 2 react apps rendered originally), it was instead as a part of flow() generator so maybe that can get issues too.

Adding parts of the highlighted code directly into the application initialization seems to resolve the issue :) Its great to know whats going on and we will see if we will restructure our code or add the:

if (typeof rdBatched === "function") configure({ reactionScheduler: rdBatched })

Thanks for a super fast reply @mweststrate and @FredyC. I am happy with the explanation and solution/workaround, can i close this now or is this something that you would like in mobx-react-lite?

[danielkcz]

can i close this now or is this something that you would like in mobx-react-lite?

I am kinda curious about that as well. Do we need batching in lite? I am grasping half of the stuff that has been said in here, so I am not really sure.

[mweststrate]

I think it should. Problem is that external tooling often doesn't like having unused imports (e.g bundlephobia might now count react and react native to the bundle size). There might be a cleaner solution than used in mobx-react (concerning the imports). Not sure. Let's at least leave this issue open for now. Op wo 8 mei 2019 18:46 schreef Daniel K. <notifications@github.com>:

…

can i close this now or is this something that you would like in mobx-react-lite? I am kinda curious about that as well. Do we need batching in lite? I am grasping half of the stuff that has been said in here, so I am not really sure. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#153 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBD3LZR2Y6TYQ7YWUZTPUL7XHANCNFSM4HLQZG4Q> .

[mweststrate]

N.B. this issue can also be fixed by configuring mobx manually when starting the app, as described here

[danielkcz]

I want to include this in the upcoming 2.0. I guess we will use the same solution as in mobx-react as there really doesn't seem to be anything better currently.

A better solution has a PR now - #214