[On hold] React Concurrent mode: avoid side effects in render

[SomeHats]

Hi! I've been really enjoying mobx-react-lite, thank you so much for this library. I've recently been doing some investigation about using MobX with suspense in react and have noticed some issues with the current useObserver implementation.

First of - components can and will throw. Suspense relies on throwing a promise. I'm a little unsure how MobX handles reactions throwing in general, but right now if you have an observed component and it throws, that upsets MobX and your thrown exception can't be handled in the usual way React does that - with error boundaries and suspense boundaries. That's a relatively simple issue to solve though.

The more serious issue is that once a component throws, React will often completely discard the tree up to the nearest boundary. That means all hooks get rolled back - so you can't rely on each render resulting in a subsequent commit (where useEffect hooks are called and their cleanup functions registered). In general, with the upcoming Concurrent mode, render will often get called more than once.

This can be demonstrated fairly easily with a component like this:

import React from "react";
import ReactDOM from "react-dom";

let isFirstRun = true;
function Suspend() {
  console.log("render");

  if (isFirstRun) {
    isFirstRun = false;
    console.log("suspend");
    throw new Promise(resolve => setTimeout(resolve(1000)));
  }

  React.useEffect(() => {
    console.log("mounted");
    return () => {
      console.log("unmounted");
    };
  }, []);

  return null;
}

function App() {
  return (
    <React.Suspense fallback={<div />}>
      <Suspend />
    </React.Suspense>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

CodeSandbox link

This component logs the following to the console:

render
suspend
render
mounted

So, what does this mean for mobx-react-lite? Right now, useObserver works by creating a reaction in the render phase, then cleaning up when the component unmounts:

mobx-react-lite/src/useObserver.ts

Lines 24 to 32 in d46cea3

	const reaction = useRef(
	new Reaction(`observer(${baseComponentName})`, () => {
	forceUpdate()
	})
	)
	useUnmount(() => {
	reaction.current.dispose()
	})

But each render call isn't guaranteed to come with a subsequent mount & render. The moment an observed component throws, all the effects of hooks (including the useRef for keeping track of the reaction) get discarded, so we end up leaking reactions all over the place.

I wonder if there's maybe some sort of thing we can do to defer calling .track until a useEffect callback, but the problem there is there can be an async gap between render and commit - when useEffect gets called. What happens when the derived state changes in that time?

Anyway. Thanks again for this project, and for reading all the way through this issue. Super happy to help try and resolve this and submit PRs, but right now it's not clear to me that there's an easy solution. Concurrent mode seems like the future of React though, so I think it's important to figure something out!

[joenoon]

@SomeHats I think you bring up good points and I'm not sure how all this works under the hood. I ran into an issue where I was using a hook that threw for suspense and needed a workaround, so I came up with the following for now. I was thinking by the time all of this stuff stabilizes I will probably be able to remove it, but here it is in case it helps or anyone has objections/suggestions:

import {useObserver} from 'mobx-react-lite';

export function useCustomObserver<T>(fn: () => T, baseComponentName?: string): T {
  let err;
  let res: any;
  useObserver<any>(() => {
    try {
      res = fn();
    } catch (e) {
      err = e;
    }
  }, baseComponentName);
  if (err) {
    throw err;
  }
  return res;
}

I use it pretty much all over the place like this, as opposed to using the observer HOC:

export const FooComponent: React.SFC<{}> = () =>
  useCustomObserver(() => {
    // ... other hooks, rendering
  });

I'm not sure if this is better/worse performance-wise than the observer HOC, but I like the code style this way better than a HOC.

[SomeHats]

Yep- that’s actually what led me to notice the memory leak/side effect issue in the first place! I wound up doing something similar in my own prototype. It seems like a reasonable enough fix for the thrown issue, but the side effects thing is I think a lot harder to solve. There shouldn’t be much difference between useObserver and the observer HOC - more or less all the HOC does is wrap your entire component in useObserver anyway: https://github.com/mobxjs/mobx-react-lite/blob/d46cea3a7f812f75ec5582237806b084f0b70828/src/observer.ts#L38

…

On Fri, 1 Feb 2019 at 21:08, Joe Noon ***@***.***> wrote: @SomeHats <https://github.com/SomeHats> I think you bring up good points and I'm not sure how all this works under the hood. I ran into an issue where I was using a hook that threw for suspense and needed a workaround, so I came up with the following for now. I was thinking by the time all of this stuff stabilizes I will probably be able to remove it, but here it is in case it helps or anyone has objections/suggestions: import {useObserver} from 'mobx-react-lite'; export function useCustomObserver<T>(fn: () => T, baseComponentName?: string): T { let err; let res: any; useObserver<any>(() => { try { res = fn(); } catch (e) { err = e; } }, baseComponentName); if (err) { throw err; } return res; } I use it pretty much all over the place like this, as opposed to using the observer HOC: export const FooComponent: React.SFC<{}> = () => useCustomObserver(() => { // ... other hooks, rendering }); I'm not sure if this is better/worse performance-wise than the observer HOC, but I like the code style this way better than a HOC. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#53 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABa6cOBziWtxdNB1FJeFbst2ZPPr6Qdlks5vJR1JgaJpZM4afN4n> .

[danielkcz]

Thank you for opening this discussion.

First of - components can and will throw. Suspense relies on throwing a promise.

Suspense for data fetching is far from official yet. Throwing a promise is most likely going to happen, but we don't know certainly that it will cause discard of the hooks or what else is going to happen. The code for that is not in React yet.

Either way, I think the PR #51 (just merged) has fixed this concern of recreating reactions on every render.

I would gladly accept a PR that handles a thrown promise within a reaction. Not sure what should be a correct behavior at this point. Perhaps it just shouldn't be supported at all. If someone wants to throw a promise, the useObserver or <Observer> could be a better tool for that.

[SomeHats]

I did encounter these issues working with Suspense, but it's fairly easy to demonstrate issues even without Suspense on 16.8.0:

https://codesandbox.io/s/18lo9r7kp3

If you run this and take a look at the console, you'll see how:

1. Render doesn't always result in a subsequent mount
2. Refs (and other hooks) get re-created each render until the tree is committed
3. The entire tree is discarded on throw - even other components that didn't throw.

I mention concurrent mode in the title of this issue, but that's not the root of the problem here - but it will as far as I can tell make these issues come up a lot more frequently. In general, the advice from the React team has been to avoid side effects in render since March 2018.

[SomeHats]

create-subscription is the React team's suggested pattern for solving this. Applying that same pattern here would look something like:

const useObserver = (fn) => {
  const [cachedResult, setCachedResult] = useState(null);

  useEffect(() => {
    return mobx.reaction(
      () => fn(),
      result => setCachedResult(result),
      {fireImmediately: true},
    );
  }, [fn]);

  if (cachedResult) {
    return cachedResult;
  } else {
    return fn();
  }
}

This way the reaction isn't created until the component is mounted, but as that might be after an async gap where data it depends on may have changed, we need to trigger a re-render on mount which obviously is not ideal.

edit: the above doesn't actually work for a bunch of reasons but something along those lines (deferring creating the reaction until the component is mounted) is an approach that would work

[danielkcz]

I did encounter these issues working with Suspense, but it's fairly easy to demonstrate issues even without Suspense on 16.8.0:

My point is that Suspense for data fetching is planned for Q4 of this year. Whatever the current behavior is, it will most likely change, so I wouldn't be really building anything on top of that. It's just way too experimental.

I've just released version 0.3.7. Can you please check if it behaves better?

Your suggestion cannot work I think. There is an important reason to have the Reaction available on the first render otherwise anything rendered won't be tracked for observables. And running renders again in useEffect ... many users would be confused why their component is rendered twice ... not a good idea.

[Jessidhia]

This is not in any alpha, but function components that invoke any hook at all will always be double-rendered in StrictMode, specifically to help debug these kinds of problems.

In particular, all state created with hooks will be thrown out on the first pass, so all useRef initialize twice, all useMemo initialize twice (even with [] as 2nd argument), all useState initialize twice, etc. Only use*Effects are guaranteed to only run once per commit.

This simulates well how a pre-empted render behaves in ConcurrentMode (all progress that is not committed is thrown out). It has been in master for a while now, though, so it's just a matter of 16.8.0 being released.

[Jessidhia]

useLayoutEffect runs synchronously when a commit happens, so it might work as a hack for your use case. useEffect may be delayed because it waits for the browser to be "idle enough" when possible.

Also, a setState invoked inside useEffect will be asynchronous in ConcurrentMode, but always synchronous in useLayoutEffect; you really need the latter for this. setCachedResult will not trigger a rerender if the value is === the previous one (again, on master; in alphas it'll rerender) so it's not as bad as it sounds, and it's a cost only paid on mount.

(caveat: this is just how it's implemented right now, there's no guarantee this is how the "final" ConcurrentMode will work)

[mweststrate]

First of - components can and will throw

That can be fixed quite easily, will create a PR.

Your suggestion cannot work I think. There is an important reason to have the Reaction available on the first render otherwise anything rendered won't be tracked for observables. And running renders again in useEffect ... many users would be confused why their component is rendered twice ... not a good idea.

This is correct, the reaction needs to be setup (either in componentWillMount, or at beginning of rendering), or otherwise MobX won't be able to track what is happening during render.

@Jessidhia thanks for the pointers! Will investigating this more..

[mweststrate]

So far biggest risk seem:

1. reaction is setup and tracked the first rendering
2. tree is not committed
3. the useEffect in useUnmount is never called -> reaction is never disposed

First dirty work around that pops to my mind, keep track of whether a useEffect was actually called, and if not, dispose the reaction after a set timeout (e.g. 1000 ms?)

[Jessidhia]

Try testing against a build from master, or waiting for 16.8.0. It should be safe to just call setState in a useLayoutEffect -- the component will only rerender if the value actually changed even without React.memo, and it will be synchronous even in ConcurrentMode so users won't see the outdated data on screen.

The "double render" will be necessary for correctness, unless you're going to do a garbage collection hack as you mentioned. If you are, you probably should give a longer timeout, like 6s (the default deadline before an asynchronous update becomes synchronous is 5s; but this is an implementation detail).

[SomeHats]

Thanks you, @Jessidhia & @mweststrate. One way we could avoid the timeout hack would be if MobX had some way of telling if a value had gone out of date without creating a reaction. You could imagine some API that gets passed an expression, and returns the value of that expression and some token that represents everything that would have been tracked in order to create that value. Later, in useLayoutEffect, you could set up your reaction but also use that token to check whether a re-render is needed.

I'm not hugely familiar with MobX internals but from stepping through the source a few times it seems this would be pretty hard to do right now. Maybe still worth throwing out there though!

[mweststrate]

I think it might be possible (read the values, subscribe later) by introducing an abstraction for that in the Reaction class. But it has two problems: 1) it is bug sensitive, as between reading and subscribing, the value might have changed, and for that reason we might miss the next update (as unchanged values don't propagate). 2) if the value being read is a @computed, it would be suspended between read and subscription, since it thinks it has no subscribers. So in that case when the subscription comes, it would re-evaluate again.

I think there are two possible solutions for this:

1. Implement a scheduler that cleans up too old reactions for components that never became committed
2. Capture the rendering in a computed value, and subscribe only in the after effect. That would work fine as far as I can see. But the clear downside is that a double rendering would occur (we would need to force an additional state-change / rendering just to make sure we can setup the tracking. UseLayoutEffect would probably be needed here).

Would anybody that has little experience with this, willing to set up a PR with tests that check the concurrent render behavior, with render trees that will be discarded etc, so that we can verify the impact of either solutions?

[solkimicreb]

Wouldn't cleaning up the reactive wiring in caught errors solve this issue for suspended renders and user errors at least? I mean calling an onUnmount here. This way it would clean up on real errors (not much else that can be done anyway) and on suspends (react will re-render the components after the thrown Promise is resolved and mobx can reobserve it).

It wouldn't solve uncommited renders without thrown errors though (which may happen with async React, I have no idea.)

[Jessidhia]

If there is a suspend on your boundary, whatever you rendered so far will be thrown out anyway. I think there might be some degree of overthinking here.

It's also not possible to catch suspends.

[mweststrate]

Note to self: should investigate if mobx-react suffers from the same issue

[mweststrate]

I think at this point we can put this easy on hold; we know how we could eliminate the issue, but since concurrent mode is so alpha, I am a bit hesitant to fix an issue that might be potentially addressed by React itself. (Assuming no one is using concurrent mode in production atm?)

[mweststrate]

Also interesting approach, where the rendering is stored in state: https://medium.com/@suhdev/mobx-quick-way-to-converting-class-based-components-to-use-react-hooks-c54f8b67755d. Since the initial rendering doesn't subscribe, there will be a double rendering (could maybe be skipped with a flag), but no reaction leak

[RoystonS]

It may be worth considering fixing this up even though concurrent mode isn't 'prime time' yet: React <StrictMode> becomes significantly less useful otherwise:

We recently turned on React <StrictMode> in our app, to ensure that we didn't accidentally add bad practices in our own code. Unfortunately, the behaviour of mobx-react-lite causes lots of warnings to be flagged up: basically, every time any useObserver-based component gets unmounted and where the state it was observing later changes.

Because one of the things that <StrictMode> does is to deliberately double-render, useObserver creates at least two reactions, and only one is unsubscribed when the component is unmounted. So a later change to the observed observables causes a forceUpdate which calls a state setter on an unmounted component, which <StrictMode> flags up.

The result, unfortunately, is that our console contains a lot of warnings caused by mobx-react-lite.

[danielkcz]

@RoystonS If you look at the useObserver, would you have some idea how to solve that problem? I am fairly confused about how it can create two reactions when there is an useRef and check for it.

mobx-react-lite/src/useObserver.ts

Line 28 in ed1634d

if (!reaction.current) {

Does it mean that useRef is unreliable in Concurrent mode as well? A component code can be executed in parallel?

[RoystonS]

I believe that's the case, yes; the useRef calls (and even useState calls) are done during React's render phase, which can occur multiple times for any component. (We've been recently bitten by this, where we'd attempted to use useRef to guarantee singleton creation of model objects for components. Oops. That's why we turned StrictMode on.) It's only the commit phase that's guaranteed to run once, so anything that causes side effects such as subscriptions has to be in a useEffect.

I'm just attempting to put together a test case in a PR (as requested by @mweststrate earlier: #53 (comment)) that should demonstrate the problem.

[danielkcz]

I find it hard to believe that initializing value is considered a side effect.

The problem I see here is that we will be deliberately rendering component without observable logic because Reaction won't be created at first. And then useEffect would create it, we would forceUpdate only to apply reaction.

Meh...if Concurrent mode is about such deliberate awkwardness, I think I am going to avoid it for now in hoping they improve it in some way or provide better tools.

[RoystonS]

Yep, even useState with an initialization callback will also cause that callback to be invoked multiple times, I'm afraid.

Sadly, StrictMode is useful for things other than preparing for concurrent-mode, and it's not possible to turn on parts of StrictMode. :( I'll get this test together to demonstrate the problem.

[RoystonS]

Thinking about the two options mentioned in #53 (comment):

Outside of strict mode (i.e. in concurrent mode) multiple render phases on a single component (causing leaked Reactions right now) ought to be 'possible but rare', so optimising for that case by introducing something that always caused every observer component to need to render twice (option 2) wouldn't be ideal.

Option 1 (cleaning up 'unconfirmed' reactions) might be preferable; one little tweak would be needed to avoid the StrictMode warnings: forceUpdate would need to check that the component got committed before attempting to trigger the update via a state set, in case observable changes are made before the cleanup occurred.

e.g. via something like this (which could be made more efficient by being combined into other existing hook calls) in useObserver:

/**
 * Like useForceUpdate, but doesn't force updates on
 * components that have never been committed.
 */
function useSaferForceUpdateOnlyIfCommitted() {
  const committed = React.useRef(false);
  React.useEffect(() => {
    committed.current = true;
  }, []);

  const forceUpdate = useForceUpdate();
  return React.useCallback(
    () => {
      if (committed.current) {
        // Only pass on the request to force the update if this component was committed
        forceUpdate();
      }
    },
    [forceUpdate]
  );
}

[urugator]

Is the following viable?

function useObservable(cb,deps = []) {
  if (SSR) return cb();
    
  // It will run twice, but the first run is nearly a no-op as we don't render anything
  const [element, setElement] = useState(null);
  
  // LayoutEffect so there is no chance to repaint   
  useLayoutEffect(() => {    
    return reaction(cb, setElement, { fireImmediately: true });    
  }, deps);
  
  // The initial `null` won't be seen by user  
  return element;
}

[danielkcz]

@urugator Check #119, that's already solved there. And in #121 we are close to tackling lingering Reactions.

[urugator]

@FredyC well the idea was to not to create "lingering" reactions, without the need to run rendering logic twice...

[danielkcz]

But we are not rendering twice anything. The lingering reactions will be cleanup up later. I wouldn't really rely on useLayoutEffect to prevent double render, that feels strange in Concurrent world.

[urugator]

But we are not rendering twice anything.

You're not, but there was mentioned setState approach with initializer function which does.
This is the same apporach, but doesn't require to run callback logic twice.

I wouldn't really rely on useLayoutEffect to prevent double render.

It doesn't prevent double render (it will double render as mentioned in comment).
It prevents repaint (flushing rendered elements to actual DOM).
It works like componentDidUpdate, it runs synchronously after render (afaik).

[danielkcz]

Sorry, I don't see any real benefit to your solution compare to the one we made. Except for those lingering reaction, but it's no big deal to clear them up later.

[RoystonS]

That useLayoutEffect approach wouldn't just be a simple double-render of a tree: you'd get N render loops where N is the height of the observer tree. e.g. if you had observer component A rendering observer component B rendering observer component C and so on down the tree, you'd have A rendering once, then a second time which also rendered B, which then causes a second render, which also renders C the first time, which then triggers a second render and so on. The initial layout of a deep tree would require a lot of renders. The useState double-render (which, whilst not ideal) causes two React render loops rather than N. The timer-based solution avoids double-renders entirely (at the cost of a background timer tidying up the [normally rare] leaked reactions).

[urugator]

wouldn't just be a simple double-render of a tree

If I count properly you get 2N component updates/renders, which is the same as rendering whole tree twice.
The only difference is that there will be N different produced trees during the process, instead of 2 (which is what you refer to as "render loop" I believe).
My assumption (possibly wrong) is that there is no interleaving inbetween, that there is no complex logic involved between these produced trees, that it simply runs sequentially until it resolves.

EDIT: Created a sandbox to check it out: https://codesandbox.io/s/l2k7opn649
There is 2N component updates, but probably flushed sooner than I hoped for.

[danielkcz]

@urugator Correct me if I am wrong, but you essentially want to prevent updating a DOM in a situation when rendered tree changes between initial render and commit phase. How often is that going to happen? In theory, it could happen for animated components which are re-rendering fairly quickly, but I kinda doubt that anyone would use MobX for that anyway :)

[urugator]

How often is that going to happen?

Always, because the proposed solution returns null on initial render (to skip the rendering logic). So on the second render (initiated from layoutEffect) the tree is changed.

[danielkcz]

I see. Would it hurt that much to actually render what is supposed to without reaction for the first time? If there wouldn't be any change between that and commit, no DOM needs to be updated and it would be only a matter of double call of a render.

Only...Well, if we really do that, I can already predict a bunch of people coming here and complaining that their components are rendered twice. It might be a nasty show stopper for some folks obsessed with performance (some might have a good reason for it).

As @RoystonS mentioned in #121 (comment) there is a chance that our solution from #119 can potentially cause loss of an update.

Well, there is a tradeoff in every solution apparently :/

[RoystonS]

Give me a little bit to write some more unit tests, and then it'll be easier to discuss implementations.
The issue I flagged up with #119 is certainly mitigatable, and was probably present beforehand.

2N renders isn't the same as rendering the tree twice. useEffect comes with a guarantee that it'll run before the next render cycle, which explains your 'flushed sooner than I'd hoped for' behaviour. That effectively forcibly flushes all the useEffect calls in the tree immediately, which isn't good for performance.

[urugator]

Idea (a bit convoluted though):
What if we wouldn't create a reaction on the initial render, but only collected accessed dependencies and stored them to ref - these will be normally GCed if render isn't commited.
Then we would create standard reaction in effect, while providing these deps as "initial", without the need to invoke render callback again.

[RoystonS]

You'd need to know if the values of those observables changed between the first render and the effect (so you can trigger an immediate rerender [this is the same issue I flagged up with #119]). If you only knew the dependencies, that would be hard to do. That's why creating the reaction once and leaving it established is a good thing.

[danielkcz]

Released mobx-react-lite@next with the @RoystonS' great work that should tackle all issues with Concurrent mode. Please report any further issues separately.