Opt-in variant of batched updates

[danielkcz]

Kudos to @eps1lon for an idea facebook/react#15080 (comment)

Instead of creating two bundles per environment (dom, react-native), this solution is opt-in and as such should be causing less trouble for people who are not concerned by it. The bundles still exist although they are the same. The 2.0 will consolidate that.

Ultimately, the mobx-react could utilize the same approach. It could be just added in V6 and somehow deprecate native bundle.

Fixes #153

[coveralls]

Coverage decreased (-0.9%) to 98.649% when pulling d300e58 on batched-updates into c8febca on master.

[danielkcz]

I tried to recover (in this PR) commented out test mentioned in #191 (comment) and it passes even without batched updates. I have probably misunderstood something about that use case or is it possible it actually works now without any hacks?

[danielkcz]

@mweststrate Do you have any objections about this solution? Also please see my last comment, something doesn't add up.

[mweststrate]

@FredyC, It seems there is still an react-dom dep in src/optimizeScheduler.ts ?

But yes, I think the general approach is great! I think just exposing a setReactBatchScheduler(fn) would already do the trick, if it is documented that either unstableBatchedUpdates from react-dom or react-native should be passed in there before starting the React rendering.

It could be indeed that the test passes already, but that is mere lucky chance in that case about in which other the listeners are established, rather than that it is guaranteed to work. The general outline of the problem is:

1. two components, a parent and a child, listen to the same observable
2. the observable is changed outside a react event handler / act block
3. parent must re-render before child

Now the notification system might by mere change already notify in the right order (I think there is a Set nowaydays storing the listeners). However, it is not guaranteed. BatchedUpdates guarantees that even if a child is notified to render before a parent, the parent renders first nonetheless.

[danielkcz]

It seems there is still an react-dom dep in src/optimizeScheduler.ts ?

It's only for the type (which is the same). It's stripped away in a bundle.

I am exporting optimizeScheduler from the bundle so it can be used directly if someone wants. Those separate import files are for convenience, but it's true I can document the existence of that function too. The dependency on react-dom typings might a problem in that case though, so I guess I will have to copy that type declaration.

2. the observable is changed outside a react event handler / act block

Yea, that's probably what I messed up, will try to fiddle with that.

[xaviergonz]

what if it first try-catch require react-dom, if that failed try-catch require react-native and if that failed threw an exception?

import { configure } from "mobx"

let domPackage: any

try {
    domPackage = require("react-native")
}
catch {
    try {
        domPackage = require("react-dom")
    }
    catch {
        throw new Error("neither react-native nor react-dom packages could be found")
    }
}

const unstable_batchedUpdates = domPackage.unstable_batchedUpdates
if (!unstable_batchedUpdates) {
    throw new Error("unstable_batchedUpdates could not be found in either react-native or react-dom packages")
}
configure({ reactionScheduler: unstable_batchedUpdates })

[xaviergonz]

Just checked, it does work, but it shows a warning on the console and a warning on compilation with create-react-app that such module cannot be found (e.g. react-native)

[mweststrate]

Yeah, I'd expect bundlers (and other analysis tools) to trip over it

…

On Wed, Sep 11, 2019 at 9:32 PM Javier Gonzalez ***@***.***> wrote: Just checked, it does work, but it shows a warning on the console and a warning on compilation with create-react-app that such module cannot be found — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#214?email_source=notifications&email_token=AAN4NBEODKHSPEPQB3AAEKTQJFBTTA5CNFSM4IPFW3RKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6PUIVI#issuecomment-530531413>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBEF4ZKQH22GOGNOU2DQJFBTTANCNFSM4IPFW3RA> .

[danielkcz]

Ok, I tried to break that test to stay as close to original as possible, but nada. It's kinda rare trying to break something and it doesn't want to be :)

Oh well, no point delaying it because of that. Let's leave it for another day when it becomes an actual problem.