Support concurrent features React / React 18 #2526
Comments
|
That is: assuming no one is using concurrent mode in production atm |
|
For anyone interested Personally, I don't think it's worth the hassle supporting Concurrent mode in class components. |
|
They just published a post getting further into details about Concurrent Mode: https://reactjs.org/docs/concurrent-mode-intro.html |
|
I believe we are ready for Concurrent mode with |
|
I hadn't heard about Is it intended to supercede |
|
Um no, |
|
Ok, FWIW, using suspense with observables is pretty natural if you |
mweststrate
changed the title
|
@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 |
|
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. ***@***.***> 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 It's certainly a big mind-shift for everyone. I have modified your example to utilize the |
|
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 |
|
My wires must be crossed, but it's same old cryptic "it won't work" without explaining what is supposed to be broken :)
And practically it means what? :) For me the main reason to utilize 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. |
|
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 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) |
|
Relevant thread: https://twitter.com/acemarke/status/1230687035045896192 |
|
Might be not relevant anymore too, so let's close it till there is someone who wants to investigate this. |
danielkcz
added
🎁 mobx-react-lite
mweststrate
changed the title
mweststrate
changed the title
|
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 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. |
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? |
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? |
@mweststrate @urugator Can you comment on the ramifications of the new React 18 API |
|
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 :) |
|
I checked the constraints mentioned in the changelog and based on my understanding this approach would not work:
We could capture the current values of the last dependency tree to make a snapshot, but then if the next render reads a new dependency, it will be reading the current value. That would lead to an even worse problem, inconsistencies/tearing within the same component, instead of across components. I don't think you can fully support React 18 concurrent rendering without full store snapshots. |
|
I tested today the following snapshot mechanism in a POC reactive state library I'm doing. It should work for Mobx and React-Solid-State as well. @mweststrate @ryansolid Snapshot conceptThe concept is basically using dynamic snapshots that starts empty, and are populated during any observable write. But we still need to solve some problems, information stored in snapshots will be redundant, and once you have a lot of snapshots, it will be costly to update all of them. To solve that, we can link previousSnapshot->nextSnapshot, and only update the latest snapshot during observable writes. That also avoids storing redundant information. To read from a snapshot now, you read the values stored in it, falling back to the next snapshots, falling back to the current observable value. That means, the older the snapshot the slowest it is to read it because of the hops. But that shouldn't be an issue, as React should only use relatively recent snapshots to render. Snapshots could even have an "optimize" function that reduces its hops by copying all values from the next snapshots to itself, and then updating its link to point to latest snapshot. But this API will not be used in React integration. Another advantage of linking that way is that as soon as there is no ref to an old snapshot, all changes that could be read from it will be garbage collected along with it. A sketch of what the API could look like: const obs = observables({ foo: 1, bar: 1 });
obs.foo; // Reads 1 from observable
let snapshot1 = createSnapshot(); // Creates an empty snapshot and point latestSnapshot weakRef to it
snapshot1.read(() => obs.foo); // Read 1 from observable
obs.foo = 2; // Writes to observable, and add oldValue (1) to latestSnapshot (snapshot1)
snapshot1.read(() => obs.foo); // Read 1 from snapshot1
let snapshot2 = createSnapshot(); // Creates an empty snapshot, link snapshot1 to snapshot2, and update latestSnapshot weakRef
obs.foo = 3; // Writes to observable and add oldValue (2) to snapshot2
obs.bar = 2; // Writes to observable and add oldValue (1) to snapshot2
snapshot1.read(() => obs.foo); // Read 1 from snapshot1
snapshot1.read(() => obs.bar); // Read 2 from snapshot2
snapshot1 = null; // Garbage collect snapshot1 and all it's values
snapshot2.read(() => obs.bar); // Read 2 from snapshot2
snapshot2 = null; // Garbage collect snapshot2 and all it's values
obs.bar = 3; // Write to observable, no need to update snapshots because latestSnapshot weakref is emptyOnce a value is written to a snapshot, it cannot be overwritten in that same snapshot. First value wins. Use I think any read from a snapshot should work, including computeds. Write operations should be blocked within a snapshot read. Implement Once you have that, it should be easy to be fully compliant to React 18 concurrent rendering. Sketch: function observer(renderFn) {
return wrapperComponent(props) {
const onStoreChangeRef = useRef();
const snapshot = useSyncExternalStore(
useCallback((onStoreChange) => {
onStoreChangeRef.current = onStoreChange;
return () => (onStoreChangeRef.current = undefined);
}, []),
() => createSnapshot()
);
const renderResult = snapshot.read(() => renderFn(props));
// Subscribe to all detected dependencies and call onStoreChangeRef.current when they change
// That will inform react to take snapshots before the next renders
return renderResult;
};
}For browsers that don't support weakref (IE?), you can just force a new snapshot after latestSnapshot reached a specific size. That will give the GC a chance to collect changes that don't belong to a referenced snapshot. I think the last problem to mitigate is that components that are not re-rendered often will hold a reference to a very old snapshot. So it's good to force all components to rerender every now and then to allow the snapshots to be GC. The re-render trigger could be:
This was super long :-) I hope it is useful |
|
@lucaslorentz Do I understand correctly you don't create any shallow/deep copies. Eg snapshot1.read(() => obj.prop);
delete obj.prop
snapshot1.read(() => array[0]);
snapshot1.read(() => array.map(fn));
snapshot1.read(() => Object.keys(obj)); |
|
@urugator Good point. Every change would need to go to snapshots and be read from snapshots later, including Object keys, array lengths, and so on. I'm not very familiar with Mobx internals, but if obj is a Proxy, every new key added to it should update latest snapshot with something like: obj, $keys, ['foo', 'bar'] Also, intercept ownKeys in Proxy and read from snapshots. Edit: |
|
@urugator This is what I have so far as proof of concept: https://codesandbox.io/s/github/lucaslorentz/react-observable-test Use the UI to monitor snapshots, insert todos, and then time travel by clicking on the snapshots. As you can see in those tests object structure is captured in snapshots and proxy behaves well enough to pass jest assertions. |
|
@lucaslorentz I have a generic question unrelated to the code you posted. It's not meant as a criticism, it's genuine question, something I honestly don't know the answer to and I would like to understand: |
|
@urugator My understanding is that a react render cycle now can be split across multiple ticks, yielding the CPU to other browser tasks in between. The simplest scenario I can think of is:
|
|
@lucaslorentz I think I am missing something obvious. Consider A and B depends on the same state ( So either you render different versions of the same state and you get a tear. EDIT: Oh, perhaps B ignores the update and renders with previous version, flushes to DOM and at the same time a new update is scheduled? |
@urugator That's right, if you re-render A before updating DOM it will not tear. But react will not render the same component twice in the same rendering cycle. If it goes that path, a fast changing state (clock for example), that changes quicker than a render cycle, would cause the rendering cycle to never finish and react would never update DOM.
Yes. That's what it should do. "Ignoring the update" is only possible if it has a snapshot of how the state was at the beginning of the rendering cycle. And yes, components renders with old values even though they're scheduled to be rendered again in the next cycle with new values. |
Individual clock ticks aren't synchronous, yielding in between ticks to render is expected. The concern is that all components should always display the same time - even though they may not immediately re-render with every tick of the clocks.
I am aware react can run render with outdated state/props, but afaik with synchronous updates it won't yield to browser as long as there are updates scheduled. That's what |
|
@urugator I think you're right. Looks like useSyncExternalStore already fixes the tearing (without snapshots). Please disregard my comments above. I will do some testing. |
|
What are the performance impacts? As of now, Mobx integrates quite well with the new scheduler and its hooks for transient changes if you need to yield for other updates, how would the suggested changes add to that? |
|
@seivan Didn't do any benchmarks. If ever added, snapshots should be optional. Depending on which async rendering features snapshots unlock, your application might actually feel more responsive even with snapshots overhead. @urugator I completely misunderstood how useSyncExternalStore works. It is as you said above, it just forces a sync render of all affected components. test sandbox I thought useSyncExternalStore would keep data in sync (via snapshots) throughout an async rendering cycle. I'm a bit disappointed with that. 😄 Turns out I was the one missing something. No tearing risk with useSyncExternalStore. Since useSyncExternalStore is inherently sync. In order to take advantage of react concurrency, it needs to be combined with useDeferredValue. And then snapshots come into play again whenever you want to defer the rendering of complex objects. StartTransition also integrates with UseDeferredValue. Is useDeferredValue already supported by Mobx? It would be nice to just wrap low-priority components with |
|
Thank you for the investigation. My conclusion is that we can make MobX tearing free just by using
Still not sure about it: |
|
Fwiw, I did some experiments with mobx at this repo: https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-rendering Regular, unpatched I then patched const reactionTrackingRef = React.useRef(null);
let forceUpdate;
if (!reactionTrackingRef.current) {
const newReaction = new Reaction(observerComponentNameFor(baseComponentName), function () {
if (trackingData.mounted) {
version = version + 1;
forceUpdate();
}
else {
trackingData.changedBeforeMount = true
}
});
const trackingData = addReactionToTrack(
reactionTrackingRef,
newReaction,
objectRetainedByReact
);
}
useSyncExternalStore(
useCallback((onStoreChange) => {
forceUpdate = () => {
onStoreChange();
}
}, []),
() => version
);This only failed the two level 3 tests (5 & 6), which seems to be more in line with the current level support of some other major state libraries (see this table). I did some quick manual tests and did not detect any extra renders. |
|
@k-ode Thank you, that's promising. However I think |
|
Are there any updates regarding mobx supporting React concurrent features? |
What particular updates are you looking for? Mobx works in conjuction with the scheduler for rendering already #2526 (comment) It will cancel obsolete states rendering/reductions and defer state changes. |
|
@imjordanxd It might be useful for you: dai-shi/will-this-react-global-state-work-in-concurrent-rendering#63 The reason why it's rejected is that implementation doesn't use a common reducer (which is required for this repo but isn't required for React) |
|
Just to let you know, I am working on some improvements. I've managed to rewrite |
|
The effort is here #3590, should be more or less complete. |
@k-ode do you mind sharing the code you used to test MobX against these tests? I recall there is some scaffolding required |
|
@ericmasiello Sure, here it is (most likely out of date though). src/mobx/index.js import React, { useCallback } from 'react';
import { observable, runInAction } from 'mobx';
import { observer } from 'mobx-react';
import {
reducer,
initialState,
selectCount,
incrementAction,
doubleAction,
createApp,
} from '../common';
const state = observable(initialState);
const useCount = () => selectCount(state);
const useIncrement = () => useCallback(() => {
const newState = reducer(state, incrementAction);
runInAction(() => {
Object.keys(newState).forEach((key) => {
state[key] = newState[key];
});
});
}, []);
const useDouble = () => useCallback(() => {
const newState = reducer(state, doubleAction);
runInAction(() => {
Object.keys(newState).forEach((key) => {
state[key] = newState[key];
});
});
}, []);
export default createApp(useCount, useIncrement, useDouble, React.Fragment, observer);src/common.js diff --git a/src/common.js b/src/common.js
index 017acbf..ffe60a3 100644
--- a/src/common.js
+++ b/src/common.js
@@ -80,7 +80,7 @@ export const createApp = (
return <div className="count">{count}</div>;
});
- const Main = () => {
+ const Main = componentWrapper(() => {
const [isPending, startTransition] = useTransition();
const [mode, setMode] = useState(null);
const transitionHide = () => {
@@ -142,7 +142,7 @@ export const createApp = (
<div id="mainCount" className="count">{mode === 'deferred' ? deferredCount : count}</div>
</div>
);
- };
+ });
const App = () => (
<Root> |
|
Slightly related as there's a lot of export const useSelector = <T>(selector: () => T): T => {
return useSyncExternalStore(autorun, selector);
}Discussion here #3589 |
|
closing as landed. |
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: mobxjs/mobx-react-lite#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)
The text was updated successfully, but these errors were encountered: