Opt-in variant of batched updates

[danielkcz]

Mostly just reexport of the mobxjs/mobx-react-lite#214.

In some distant future when 7.0 comes out, we can remove react-native specific bundle. Although it would be nice to somehow deprecate it now. Ideas?

[coveralls]

Coverage increased (+0.04%) to 95.628% when pulling 31b1bc8 on batched-updates into d1f7314 on master.

[auvipy]

LGTM

[danielkcz]

Well, and what do you think about the deprecation strategy for the current approach?

[mweststrate]

I think this is a cool approach, alleviating some tooling issues! As a hint, we could maybe detect and warn during the first render that no scheduler has been set at all. (and maybe even provide a noop scheduler if for some reason observer is used without react-dom or native)

[danielkcz]

I am still a bit concerned about the deprecation strategy. Or do you think that removing RN specific bundle with a next major without any sort of warning is ok? It's probably not that big change on the consumer side, that's true.

As a hint, we could maybe detect and warn during the first render that no scheduler has been set at all.

Wouldn't that be kinda obtrusive? Most people probably don't need it and bugging them to even set noop one feels like a nuisance.

[mweststrate]

imho most people does need it at some point, as not using this will lead at random to really confusion semantics / /bugs when an update causes both a parent and child should update, and they don't do it in order. So I quite disagree with the premise ("most people don't need it"). It is imho simply a requirement for mobx-react to run correctly and predictable. The fact that it doesn't cause a bug in all projects (or even in most, if you stick to causing updates from event handlers), doesn't mean it's a random opt-in optimization; its not just an optimization, its a change in semantics. Figuring out afterwards why something doesn't work here, and why components are updating in the incorrect order, is way way harder, than blindly configuring this correctly upfront.

…

On Sun, Jan 12, 2020 at 10:53 PM Daniel K. ***@***.***> wrote: I am still a bit concerned about the deprecation strategy. Or do you think that removing RN specific bundle with a next major without any sort of warning is ok? It's probably not that big change on the consumer side, that's true. As a hint, we could maybe detect and warn during the first render that no scheduler has been set at all. Wouldn't that be kinda obtrusive? Most people probably don't need it and bugging them to even set noop one feels like a nuisance. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#787?email_source=notifications&email_token=AAN4NBCYLV4V5ZT7I6NUWPDQ5ONQDA5CNFSM4JAW3RZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIXGPTA#issuecomment-573466572>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBAI64CYFH7DQNOIJ5LQ5ONQDANCNFSM4JAW3RZA> .

[danielkcz]

Alright, I seem to have misunderstood the problem at hand. I even call it optimization in mobx-react-lite docs.

It would be nice if you can create some small writeup of the implication of (not) using it. We could then link it from that hint so people can make educated decision and not just a guess.

[mweststrate]

Short summary (I think you can confirm this by disabling it in the mobx-react repo and running the unit tests, some should break). For example this test: https://github.com/mobxjs/mobx-react/blob/master/test/observer.test.js#L639

The idea is:

1. Render two observer based components <Parent><Child /></Parent> (btw, you can also reproduce with normal react state I think)
2. Make sure both parent and child depend on the same observable (in the test: store.user)
3. Make sure that child has some implicit dependency on the parent (in the test: child wouldn't be rendered by parent if store.user is not set)
4. Update that shared observable. E.g. set store.user to undefined. Now both components will signal to update. However, it is imperative that the Parent is updated before the Child. As the Parent will then remove the child and make sure it never gets updated as all. If the Child would render first, it would try to pick the name property of store.user. Which would correctly throw; the Child shouldn't even try to render if the user is not set, the Parent would have prevented that if it renders first.
5. Normally, React guarantees that Parents always render before Childs. It does this by using event handlers as batch handlers (very similar to action in MobX). So at the end of an event handler, all components scheduled for update are ordered correctly by react, and then they are actually rendered. This guarantees in most situations that Parents render before Child.
6. However, if an update does not originate from a React event handler, this batching never happens, and this guarantee disappears. For example if you put an timeout in an event handler, or update from a fetch callback, websocket message, etc etc. They might change this in a future of React, but to make sure those cases don't break now, we need to batch manually using unstable_batchedUpdates (all similar libraries basically need to do this). Luckily, since mobx has already a batching mechanism we can just combine the two, which is what this whole mechanism is doing; attaching reacts updates to our own 'event loop'. Note that this is also the reason act is needed in React unit tests.
7. So, this issue will not occur in all projects, as you need a combination of some Parent + child relationship + non-react event handlers (and even then, without batching the undeterministic order can be by chance be correct). But as you can see, without any background knowledge, it will be very tricky to figure out what goes wrong if it goes wrong and this wasn't configured by default.

Hope that helps!

[danielkcz]

Thank you for the write-up, I need to dive into it and try it out and then I will add explanation in some form to mobx-react-docz site.

However, I am thinking there might be a small flaw to this solution - UMD. Since it's not an optimization as I first thought, it's probably viable to have that included in UMD build which would not have an option to do it. But I wonder if people using UMD actually do need it. I cannot think of reasons why would someone use UMD for serious app development. What do you think?