mobx icon indicating copy to clipboard operation
mobx copied to clipboard

Support concurrent features React / React 18

Open mweststrate opened this issue 5 years ago • 48 comments

Support concurrent mode React. Primary open issue, don't leak reactions for component instances that are never committed.

A solution for that is described in: https://github.com/mobxjs/mobx-react-lite/issues/53

But let's first wait until concurrent mode is a bit further; as there are no docs yet etc, introducing a workaround in the code base seems to be premature at this point (11-feb-2019)

mweststrate avatar Feb 11 '19 11:02 mweststrate

That is: assuming no one is using concurrent mode in production atm

mweststrate avatar Feb 11 '19 11:02 mweststrate

For anyone interested mobx-react-lite@next has experimental support for Concurrent mode. Feel free to test it out.

Personally, I don't think it's worth the hassle supporting Concurrent mode in class components.

danielkcz avatar Jun 08 '19 08:06 danielkcz

They just published a post getting further into details about Concurrent Mode: https://reactjs.org/docs/concurrent-mode-intro.html

bsmith-cycorp avatar Oct 24 '19 20:10 bsmith-cycorp

I believe we are ready for Concurrent mode with mobx-react-lite@next. Of course, at some point, we shall try against "experimental" version if tests really do pass just to be sure. PR for that is surely welcome :)

danielkcz avatar Oct 24 '19 20:10 danielkcz

I hadn't heard about mobx-react-lite.

Is it intended to supercede mobx-react? That would be hugely disappointing for me, as I take serious philosophical issue with hooks, especially given MobX's emphasis on (and synergy with) classes.

bsmith-cycorp avatar Oct 24 '19 21:10 bsmith-cycorp

Um no, mobx-react@6 is built on top of mobx-react-lite :)

danielkcz avatar Oct 25 '19 05:10 danielkcz

Ok, FWIW, using suspense with observables is pretty natural if you throw when(() => expr) :)

https://codesandbox.io/s/dry-microservice-qnle3

mweststrate avatar Nov 08 '19 22:11 mweststrate

@mweststrate That's a certainly nice feat, although I never liked much to have fetching logic inside the stores. But I guess with Suspense it will make more sense to have it separated from components for a proper preloading.

I also love that it will re-render on further updates to those observables without any extra magic. That makes it really powerful.

That fetchingPosts extra state is awkward though, considering Suspense should kinda remove the worry about any sort of loading state, it shouldn't be needed. I do understand why it's there in this case, but it's not ideal for sure.

danielkcz avatar Nov 08 '19 22:11 danielkcz

Suspense doesn't make any decisions on who triggers rendering. So I didn't do either in the example.

THe first is triggered 'externally', the second is triggered by rendering, just to demonstrate both approaches.

The loading state again is a design decision. Should every render trigger a new fetch? Only if not pending? Or only at mostly once? The loading state was introduced to demonstrate you can express either solution you want.

On Fri, Nov 8, 2019 at 10:57 PM Daniel K. [email protected] wrote:

@mweststrate https://github.com/mweststrate That's a certainly nice feat, although I never liked much to have fetching logic inside the stores. But I guess with Suspense it will make more sense to have it separated from components for a proper preloading.

I also love that it will re-render on further updates to those observables without any extra magic. That makes it really powerful.

That fetchingPosts extra state is awkward though, considering Suspense should kinda remove the worry about any sort of loading state, it shouldn't be needed. I do understand why it's there in this case, but it's not ideal for sure.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/645?email_source=notifications&email_token=AAN4NBD6TCLMJWCTOFZSIXDQSXVFBA5CNFSM4GWQEUKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDTTHCQ#issuecomment-552022922, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBFSKUHEMVSFMNZL3WLQSXVFBANCNFSM4GWQEUKA .

mweststrate avatar Nov 08 '19 23:11 mweststrate

@mweststrate It's certainly a big mind-shift for everyone.

I have modified your example to utilize the useLocalStore which drags along a scary story it won't work with Concurrent mode. In this contrived example, it seems to be working just fine. Can you perhaps show what you had in mind back then?

https://codesandbox.io/s/mobx-suspense-e3rz4

danielkcz avatar Nov 09 '19 07:11 danielkcz

Yes, I'd expect useLocalStore not to work with suspense, at least the model would be very hard to reason about even if it worked. As stated before I recommend using Reacts primitives for local components to benefit from Suspense, and mobx for the external 'domain state' which should, unlike UI state, never be in flux (forked). I'll update the mobx.js.org observer documentation to reflect that make that more clear :) But that is one of the reason I really didn't emphasize the mobx-react hooks on https://mobx.js.org/refguide/observer-component.html

mweststrate avatar Nov 09 '19 11:11 mweststrate

My wires must be crossed, but it's same old cryptic "it won't work" without explaining what is supposed to be broken :)

as this can theoretically lock you out of some features of React's Suspense mechanism.

And practically it means what? :)

For me the main reason to utilize useLocalStore is essentially so I can pass that store down the tree and have it coupled with actions/computeds. So yea, I don't use it strictly as the local state, but more like decentralized state management :) Is there some reason why should Concurrent/Suspense cause problems? I am failing to see it.

It would be great to have some real explanation in https://mobx-react.js.org/ as well since it's more future-oriented and I believe people are slowly using it more often as a source of information.

danielkcz avatar Nov 09 '19 12:11 danielkcz

I never build anything to prove it, but if I understand suspense correctly, it will fork render trees when something is suspended, and something else isn't. That means that a component can exists twice at the same moment in time. That means that if you use useLocalStore, that store can be created twice, which can easily lead, at least, to a lot of confusion. Furthermore, when a tree is finished, suspense will (again, didn't see a concrete example of that) rebase the state updates (that is why the function version of setState is so important). However, rebasing observable updates is not really generically solvable. 

I think the interesting thing to test in your example is what would happen if that localStore is created somewhere in a component inside a suspense boundary, will you get two stores in that case (at least temporarily)? Now it is created outside, so I'd expect that case to work flawless indeed.

So, I expect, for useLocalStore to work properly, you'd need something that 'shares' the state across all instances of that component instance, but afaik React doesn't really offer a primitive for that (although it could maybe created by using a dedicated context for that an a special hook that stores them in there, if instances can be uniquely identified from within the component)

Beyond that, a very simple example where things get weird immediately is that useDeferredValue wouldn't work with a store created by useLocalStore, because both the deferred value and the current value would point to the same object (since mutations don't create new references to the store) 

mweststrate avatar Nov 11 '19 19:11 mweststrate

Relevant thread: https://twitter.com/acemarke/status/1230687035045896192

mweststrate avatar Feb 21 '20 19:02 mweststrate

Might be not relevant anymore too, so let's close it till there is someone who wants to investigate this.

danielkcz avatar Oct 18 '20 15:10 danielkcz

Reopening this as we're getting closer to a React 18 release. Progress can be followed in #3005.

At this moment, as far as initial test projects point out, mobx-react(-lite) plays fine with React 18 in Strict mode. No warnings, errors and features like startTransaction seem to work correctly.

At this moment we don't offer MobX specific concurrent features yet. Some tearing could (theoretically) still happen. And MobX might put React in de-opt mode (theoretically) in cases as well.

That being said, there are no known problems at this moment that would block mobx-react(-lite) from upgrading or using specific React features.

That conclusion is based on some pretty flimsy research, but at least it is some status update :). No known limitations or blockers for a React 18 upgrade at this moment.

mweststrate avatar Jun 24 '21 20:06 mweststrate

Ok, FWIW, using suspense with observables is pretty natural if you throw when(() => expr) :)

https://codesandbox.io/s/dry-microservice-qnle3

I had read the code and tried the demo.

but i think its a little tricky. Not all scenarios are applicable.

any good idea for a common use utility functions or pattern?

franckchen avatar Aug 17 '21 04:08 franckchen

Reopening this as we're getting closer to a React 18 release. Progress can be followed in #3005.

At this moment, as far as initial test projects point out, mobx-react(-lite) plays fine with React 18 in Strict mode. No warnings, errors and features like startTransaction seem to work correctly.

At this moment we don't offer MobX specific concurrent features yet. Some tearing could (theoretically) still happen. And MobX might put React in de-opt mode (theoretically) in cases as well.

That being said, there are no known problems at this moment that would block mobx-react(-lite) from upgrading or using specific React features.

That conclusion is based on some pretty flimsy research, but at least it is some status update :). No known limitations or blockers for a React 18 upgrade at this moment.

Are there plans to further research and if necessary, improve mobx-react(-lite) to better support suspense features hopefully without de-opting? Or, at a minimum, a deeper write up that summarizes how mobx should and should not be used in React 18?

ericmasiello avatar Nov 17 '21 12:11 ericmasiello

Reopening this as we're getting closer to a React 18 release. Progress can be followed in #3005.

At this moment, as far as initial test projects point out, mobx-react(-lite) plays fine with React 18 in Strict mode. No warnings, errors and features like startTransaction seem to work correctly.

At this moment we don't offer MobX specific concurrent features yet. Some tearing could (theoretically) still happen. And MobX might put React in de-opt mode (theoretically) in cases as well.

That being said, there are no known problems at this moment that would block mobx-react(-lite) from upgrading or using specific React features.

That conclusion is based on some pretty flimsy research, but at least it is some status update :). No known limitations or blockers for a React 18 upgrade at this moment.

@mweststrate @urugator Can you comment on the ramifications of the new React 18 API useSyncExternalStore on MobX?

jamieathans avatar Dec 12 '21 08:12 jamieathans

Nope, I haven't been following the conversation for a long time and I don't expect to have the time to dive into the matter anywhere in the near time. If anyone wants to grok the subject be welcome :)

mweststrate avatar Dec 14 '21 11:12 mweststrate

ramifications of the new React 18 API useSyncExternalStore on MobX

Afaik none. The result of useSyncExternalStore or previously propsed useMutableSource must be an immutable snapshot - making sure that component can re-render any time returning the same output. For a mutable state like Mobx, it requires creating deep copies, which is just incompatible with the design. The hook makes sure that all components read from snapshots that were created from the same version of the external state, so you don't see any temporal inconsistencies between components (tearing).

urugator avatar Dec 15 '21 11:12 urugator

ramifications of the new React 18 API useSyncExternalStore on MobX

Afaik none. The result of useSyncExternalStore or previously propsed useMutableSource must be an immutable snapshot - making sure that component can re-render any time returning the same output. For a mutable state like Mobx, it requires creating deep copies, which is just incompatible with the design. The hook makes sure that all components read from snapshots that were created from the same version of the external state, so you don't see any temporal inconsistencies between components (tearing).

Pardon the naive question. Does this implicitly mean MobX is incompatible with direction of React's concurrent rendering?

ericmasiello avatar Dec 15 '21 13:12 ericmasiello

Pardon the naive question. Does this implicitly mean MobX is incompatible with direction of React's concurrent rendering?

In a sense that you can experience temporal inconsistencies, yes (afaik). Concurrency and shared mutable state never works together. What kind of problem this really is in practice, I dunno and from what I could find, until React 18 is out and concurrent features are being used in larger scale, nobody is absolutely sure. The question also is how these concurrent features are in general compatible with external shared state (immutable or not). I lack the knowledge, so I am speculating here, but eg. consider you have 10 components and they all access the same external state and you suspend one of them. Now you change the external state and notify these components to re-render. Ok, so what can now React do to show consistent result? Either 9 components will wait for the suspended one, so most of your UI is unresponsive or it will unsuspend the component and re-render all 10 components at once. Concurrency requires components can work independently of one another, but how do you do that if they depend on the same shared state - you either allow temporal inconsistencies or components must depend on unrelated pieces of the shared state (so technically there is no state sharing), but in such case there also can't be any inconsitencies (and therefore tearing). Please keep in mind I may be completely wrong or missing something.

urugator avatar Dec 15 '21 15:12 urugator

See https://mobx.js.org/react-integration.html#you-might-not-need-locally-observable-state. TL,DR: for volatile state (selections, loading, active tabs, that kind of stuff), you might want to keep things in useState to leverage suspense features. For domain states (the thing your app is about, which gets stored on the server, etc etc); they should typically not be in some superstate across different renderings, but rather you want to render with the latest state possible (very much imho) I think MobX should be fine even in a concurrent leveraging application.

mweststrate avatar Dec 15 '21 19:12 mweststrate

ramifications of the new React 18 API useSyncExternalStore on MobX

Afaik none. The result of useSyncExternalStore or previously propsed useMutableSource must be an immutable snapshot - making sure that component can re-render any time returning the same output. For a mutable state like Mobx, it requires creating deep copies, which is just incompatible with the design. The hook makes sure that all components read from snapshots that were created from the same version of the external state, so you don't see any temporal inconsistencies between components (tearing).

Pardon the naive question. Does this implicitly mean MobX is incompatible with direction of React's concurrent rendering?

If you only use mobx as a reactive wrapper(only use observable.ref instead of observable, and any update must be wrapped in action), while make sure the internal state are immutable, this is not a problem.

Always use observable.ref instead of observable is already a standard in my team. Thus we don't meet this problem.

For example:

class DomainStore {
  state = { a: 1 };
  constructor() { makeObservable(this, {
    state: observable.ref, // this.state is reactive while the data in this.state is immutable.
    act: action,
    a: computed,
  })}
  act() {
    this.state = produce(this.state, draft => {
      draft.a = 2;
    })
  }
  get a() {
    return this.state.a;
  }
}

We don't use [deep mutable state] with mobx, because it introduces more complexity and lost the advantage of immutability.

From my experience, mobx's core value is [a glitches free reactivity system(based on transaction)] that exactly matches the need of human-machine-interaction development. Deep mutable reactive state is not good enough for us to give up the advantage of immutable data.

TotooriaHyperion avatar Mar 17 '22 05:03 TotooriaHyperion

this is not a problem

Sure it's still a problem. You need an immutable snapshot of your whole state not just it's individual parts. You can still see one immutable part of the state being inconsistent with a different immutable part. However I am not sure if immutability is strictly neccessary with useSyncExternalStore. You must provide getSnapshot function that must return a cached/memoized result. Dunno what exactly that means/implies. Nevertheless you still need to cosume the state through this hook and not access it directly as required by mobx. Maybe it can be solved somehow, dunno, available information around these things are quite limited atm.

Regarding the pattern, since I don't have a time to elaborate further and this isn't a good place to so anyway, let me just say that I do not recommend :)

urugator avatar Mar 17 '22 08:03 urugator

@urugator Do you not recommend mobx with react18 or mobx with useSyncExternalStore?

mtsewrs avatar Mar 17 '22 09:03 mtsewrs

So, basically, updates to shared state must be urgent (not in useTransition), and components must depend on the store immediately (not in useDeferredValue), otherwise tearing will occur temporarily when components with urgent updates read the non-urgently updated state from the store (but consistency should be restored by the first upcoming non-urgent re-render), right?

This is more or less what I'd expect from a shared store, anyways. In particular, I'd expect the value of the store not to change during the execution of the functional component (since it it neither async nor a generator), but change freely between (urgent) re-renders.

kris7t avatar Mar 30 '22 17:03 kris7t

[email protected] / [email protected] where released today, which are compatible with React 18, albeit with the constraints as mentioned in the changelog.

mweststrate avatar May 06 '22 17:05 mweststrate

Not sure if this matters, but I did a test project for checking performance on how well observables jive with the new concurrency features and I must say, it wasn't bad at all. Maybe it's not heavily optimized like other libraries. But the DevUX benefits of far outweighs the compromises. You can still get great benefits from the double buffering approach they seem to be headed towards.

Assume:

A TodoList rendering a single input together with mapping over a list of todos to render<Todo todo=todo>

With 1000 <Todo todo=x /> each with their individual inputs. Writing in the TodoList input should update all the text on Todo items and that's what is being used for the value in <Todo todo=todo> input.

  const [state, setState] = React.useState(props.store.todos[0]?.text ?? "")
  const [isPending, startTransition] = React.useTransition()


      <input
        readOnly
        type="checkbox"
        checked={isPending}
      />
      <input onChange={(e) => {
        setState(e.currentTarget.value)
        props.store.todos.forEach(todo => {
          startTransition(() => {
            todo.setText(e.currentTarget.value)
          })
        })
      }
      } value={state} />

Feel free to remove startTransition to compare performance. It's a huge difference. Not to mention the isPending could inherently let you hide everything until they are done instead of showing inconsistent state. This is very very impressive, hell they've managed to build a scheduler.

seivan avatar May 18 '22 12:05 seivan