fix(mobx-react-lit): dispose reactions right after render in StrictMode and Suspense

[Wroud]

This pull request replaces observerFinalizationRegistry with the combination of useLayoutEffect with requestAnimationFrame

The benefit is simple: reactions are disposed of right after the component is unmounted or thrown an exception or in case of react suspense or strict mode.

Related issue: #3774

[changeset-bot]

⚠️ No Changeset found

Latest commit: 94bc499

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[urugator]

Few quick notes without giving it proper look:
I proposed useLayoutEffect + Promise.resolve in the past. One disadvantage is that it doesn't allow delayed commit, without the extra re-render. Dunno how it compares to requestAnimationFrame.
We've just migrated to useSyncExternalStore, part of the motivation/hope was getting rid of the effects with funny logic, so not super enthusiastic about reintroducing these.
I've a feeling that the presence of useLayoutEffect automatically opts out of some optimizations or async rendering or something like that.

[Wroud]

I see your point. In this PR useLayoutEffect is used only for:

1. To prevent reaction being disposed
2. To force component re-render in case when reaction have been disposed (usually if react delayed component mounting for performance optimisation)

I tested this implementation on the huge project with use of Suspense and haven't noticed any issues.
I imagine that there can be a case when force re-render can cause some problems.

But it seems that this implementation works the same way in the most cases.

[Wroud]

I will try to move logic from useLayoutEffect to useSyncExternalStore thank you for the note

[Wroud]

This implementation doesn't re-render components if the reaction has been disposed of.
I'm using a combination of setTimeout and useSyncExternalStore (it seems like subscribe is called in the effects phase)

How it works:

1. we create a reaction and plan its disposal
2. capture component observations
3. if the component hasn't been rendered in 100ms, we will dispose of the reaction
4. if the subscribe of useSyncExternalStore has been called, we cancel the reaction disposing of; if the reaction is already disposed of, we request component re-render.
5. if the component is unmounted, we will dispose of the reaction immediately

I've tested this implementation with extra logs, and there are no extra re-renders

[Wroud]

@urugator I also added some extra tests for abandoned and suspended components to ensure that reactions are disposed of correctly

[urugator]

Are you aware of the TimerBasedFinalizationRegistry that is being used if FinalizationRegistry isn't available?

mobx/packages/mobx-react-lite/src/utils/UniversalFinalizationRegistry.ts

Line 12 in 273e017

export class TimerBasedFinalizationRegistry<T> implements FinalizationRegistryType<T> {

It's the same idea, but doesn't create timeout for each component and is a bit less aggresive with disposal. It's intended as a poor men's GC. The ability to rely on actual GC is considered a feature here.

[Wroud]

Yes, it's the same idea as TimerBasedFinalizationRegistry, and it can be used here as well. The main goal is to unsubscribe from observable once the component is unmounted.

There is a small difference between the original implementation and this one; despite using setTimeout probably, there will be no difference in using TimerBasedFinalizationRegistry instead of setTimeout (I mean that observables still will be unobserved a bit later and in a bunch)

Are there any blockers to use TimerBasedFinalizationRegistry even if FinalizationRegistry is available?

[Wroud]

Here is an example:
I have a component that relies on some data. Usually, I will use hooks or something to fetch and keep this state up so that data won't be fetched when the component is unmounted. But there also can be some cases when I get data transformed from another place that uses data lazy and has no logic to fetch it.

The easiest way here is to load data in the background if data is observed.

With GC, data will be kept up to date for a long time, even if it's no longer used.

[urugator]

Are there any blockers to use TimerBasedFinalizationRegistry even if FinalizationRegistry is available?

Atm TimerBasedFinalizationRegistry is a workaround that ideally would not need to exist.

With GC, data will be kept up to date for a long time, even if it's no longer used.

I do understand the concern, but the current solution is deliberate. It's not about providing an implementation (while appreciated), but about changing some design decision.

cc @mweststrate:
FinalizationRegitry/GC timing is unreliable and can take quite some time to kick in, wich also delays onBecomeUnobserved, so it's not practical as a disposal mechanism for expensive resources.
The proposed solution is to not rely on FinalizationRegistry, but either use timers with shorter deterministic delay (configurable?), or dispose the reaction in the next tick/frame/..., which would require the use of useLayoutEffect, which has a guarantee of running synchronously with render (useSyncExternalSource does not). Use of useLayoutEffect means a component can never be commited with a delay without immediate re-render, but that's effectively quite similar to using shorter delay as we have to schedule re-render anyway.

[Wroud]

idea: we can probably use useSyncExternalSource instead of useLayoutEffect for delayed renders

If the store is mutated during a non-blocking transition update, React will fall back to performing that update as blocking. Specifically, React will call getSnapshot a second time just before applying changes to the DOM. If it returns a different value than when it was called originally, React will restart the transition update from scratch, this time applying it as a blocking update, to ensure that every component on screen is reflecting the same version of the store.

so when we dispose of a reaction because of timeout, we can also change stateVersion so react will re-render the component by itself

EDIT: Maybe it's only works for transactions

[urugator]

so when we dispose of a reaction because of timeout, we can also change stateVersion so react will re-render the component by itself

That's what we do:

mobx/packages/mobx-react-lite/src/useObserver.ts

Lines 55 to 64 in 273e017

	if (!adm.reaction) {
	// We've lost our reaction and therefore all subscriptions, occurs when:
	// 1. Timer based finalization registry disposed reaction before component mounted.
	// 2. React "re-mounts" same component without calling render in between (typically <StrictMode>).
	// We have to recreate reaction and schedule re-render to recreate subscriptions,
	// even if state did not change.
	createReaction(adm)
	// `onStoreChange` won't force update if subsequent `getSnapshot` returns same value.
	// So we make sure that is not the case
	adm.stateVersion = Symbol()

[Wroud]

It's equal to run it in useEffect, but we also can change stateVersion at the same time when we dispose of the reaction, and IF react will access getSnapshot a second time before committing then it will start re-render immediately

[Wroud]

in that case component will try to render 2 times, but the actual render will be the only once, in case we change the state in effect's phase component will be committed and then will start a new render

[urugator]

I see. So, potentially, we can move

mobx/packages/mobx-react-lite/src/useObserver.ts

Line 64 in 273e017

adm.stateVersion = Symbol()

to

mobx/packages/mobx-react-lite/src/utils/observerFinalizationRegistry.ts

Lines 4 to 9 in 273e017

	export const observerFinalizationRegistry = new UniversalFinalizationRegistry(
	(adm: { reaction: Reaction | null }) => {
	adm.reaction?.dispose()
	adm.reaction = null
	}
	)

and get rid of

mobx/packages/mobx-react-lite/src/useObserver.ts

Line 55 in 273e017

if (!adm.reaction) {

because that should never happen (react throws away uncommited component, because it sees a new version)
??

[Wroud]

Yes, that's exactly what I mean

[Wroud]

With shorthand implementation, I can notice this warning in the tests:

Warning: Cannot update a component (`c`) while rendering a different component (`c`). To locate the bad setState() call inside `c`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render

but I call onStoreChange also to be sure that component re-rendered

[urugator]

If it works, I am personally open to that change. But it doesn't help much with the original issue or does it?

I can notice this warning in the tests:

If these are from the deprecated useAsObservableSource/useLocalStore you can usually ignore these.

but I call onStoreChange also to be sure that component re-rendered

Where? In finalizer? onStoreChange shouldn't be available there I think (it's provided by subscribe and subscribe runs after commit).

[Wroud]

test("sync state access count", async () => {
    let snapshotAccess = 0
    function sub() {
        return () => {}
    }

    function getStateSnapshot() {
        snapshotAccess++
        return 0
    }

    const TestComponent = () => {
        React.useSyncExternalStore(sub, getStateSnapshot)

        return <>0</>
    }

    render(<TestComponent />)

    expect(snapshotAccess).toBe(2)
})

Output quite surprised me:

● sync state access count
                                                                                                                                                                                                            
    expect(received).toBe(expected) // Object.is equality                                                                                                                                                   
                                                                                                                                                                                                            
    Expected: 2                                                                                                                                                                                             
    Received: 3                                                                                                                                                                                             
                                                                                                                                                                                                            
      220 |     render(<TestComponent />)
      221 |
    > 222 |     expect(snapshotAccess).toBe(2)
          |                            ^
      223 | })
      224 |

[Wroud]

there new two from __tests__/observer.test.tsx:
but seems like it's not related to the change stateVersion because the reaction fires it

console.error                                                                                                                                                                                           
      Warning: Cannot update a component (`Unknown`) while rendering a different component (`Unknown`). To locate the bad setState() call inside `Unknown`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render                                                                                                                                                                          
          at D:\work\mobx\packages\mobx-react-lite\src\observer.ts:104:27                                                                                                                                   
                                                                                                                                                                                                            
      54 |         // If state changes in between initial render and mount,                                                                                                                                 
      55 |         // `useSyncExternalStore` should handle that by checking the state version and issuing update.                                                                                           
    > 56 |         this.onStoreChange?.()                                                                                                                                                                   
         |                           ^
      57 |     }
      58 |
      59 |     private dispose() {

      at printWarning (../../node_modules/react-dom/cjs/react-dom.development.js:86:30)
      at error (../../node_modules/react-dom/cjs/react-dom.development.js:60:7)
      at warnAboutRenderPhaseUpdatesInDEV (../../node_modules/react-dom/cjs/react-dom.development.js:27492:15)
      at scheduleUpdateOnFiber (../../node_modules/react-dom/cjs/react-dom.development.js:25498:5)
      at forceStoreRerender (../../node_modules/react-dom/cjs/react-dom.development.js:16977:5)
      at ObserverAdministration.handleStoreChange [as onStoreChange] (../../node_modules/react-dom/cjs/react-dom.development.js:16953:7)
      at ObserverAdministration.forceUpdate (src/useObserver.ts:56:27)
      at Reaction.runReaction_ (../mobx/dist/mobx.cjs.development.js:2169:16)

    console.error
      Warning: Cannot update a component (`c`) while rendering a different component (`c`). To locate the bad setState() call inside `c`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render
          at c (D:\work\mobx\packages\mobx-react-lite\__tests__\observer.test.tsx:27:35)

      54 |         // If state changes in between initial render and mount,
      55 |         // `useSyncExternalStore` should handle that by checking the state version and issuing update.
    > 56 |         this.onStoreChange?.()
         |                           ^
      57 |     }
      58 |
      59 |     private dispose() {

      at printWarning (../../node_modules/react-dom/cjs/react-dom.development.js:86:30)
      at error (../../node_modules/react-dom/cjs/react-dom.development.js:60:7)
      at warnAboutRenderPhaseUpdatesInDEV (../../node_modules/react-dom/cjs/react-dom.development.js:27492:15)
      at scheduleUpdateOnFiber (../../node_modules/react-dom/cjs/react-dom.development.js:25498:5)
      at forceStoreRerender (../../node_modules/react-dom/cjs/react-dom.development.js:16977:5)
      at ObserverAdministration.handleStoreChange [as onStoreChange] (../../node_modules/react-dom/cjs/react-dom.development.js:16953:7)
      at ObserverAdministration.forceUpdate (src/useObserver.ts:56:27)
      at Reaction.runReaction_ (../mobx/dist/mobx.cjs.development.js:2169:16)

[Wroud]

I've pushed these small changes, I haven't noticed any Warning: Cannot update a component in my application or any artifacts

This won't fix the original issue (dispose of reaction right when the component is mounted or abandoned)

[Wroud]

test("sync state access count", async () => {
    let snapshotAccess = 0
    function sub() {
        return () => {}
    }

    function getStateSnapshot() {
        snapshotAccess++
        return 0
    }

    const TestComponent = () => {
        React.useSyncExternalStore(sub, getStateSnapshot)

        return <>0</>
    }

    render(<TestComponent />)

    expect(snapshotAccess).toBe(2)
})

Output quite surprised me:

● sync state access count
                                                                                                                                                                                                            
    expect(received).toBe(expected) // Object.is equality                                                                                                                                                   
                                                                                                                                                                                                            
    Expected: 2                                                                                                                                                                                             
    Received: 3                                                                                                                                                                                             
                                                                                                                                                                                                            
      220 |     render(<TestComponent />)
      221 |
    > 222 |     expect(snapshotAccess).toBe(2)
          |                            ^
      223 | })
      224 |

Thoughts why it's three but not 2:

1. The first one is when we render the component
2. second one before we ready to commit it
3. third one after we subscribed to store (effect phase)

[mweststrate]

Sorry no time atm to think this one through, I'll try to look later this week / next weekend

…

On Sun, 22 Oct 2023, 13:46 Alexey, ***@***.***> wrote: test("sync state access count", async () => { let snapshotAccess = 0 function sub() { return () => {} } function getStateSnapshot() { snapshotAccess++ return 0 } const TestComponent = () => { React.useSyncExternalStore(sub, getStateSnapshot) return <>0</> } render(<TestComponent />) expect(snapshotAccess).toBe(2)}) Output quite surprised me: ● sync state access count expect(received).toBe(expected) // Object.is equality Expected: 2 Received: 3 220 | render(<TestComponent />) 221 | > 222 | expect(snapshotAccess).toBe(2) | ^ 223 | }) 224 | Thoughts why it's three but not 2: 1. The first one is when we render the component 2. second one before we ready to commit it 3. third one after we subscribed to store (effect phase) — Reply to this email directly, view it on GitHub <#3777 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBAKDVQVTLBDOPSTMMLYAUBQTAVCNFSM6AAAAAA6HTXGZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZUGA3TCOBVHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mweststrate]

Apologies, just found this PR, totally lost track of it. Do we feel this still worth pursuing? I lost track in the convo above what it does and doesn't improve :).

[Wroud]

the main goal here is to remove subscription to observer immediatly if component rendering abandoned. This will fix onBecomeObserved/onBecomeUnobserved callbacks in some cases (current implementation based on GC and have a problem with long living subscriptions)

[jeffijoe]

I found this PR linked to this issue as I've been debugging why onBecomeUnobserved isn't triggering sometimes. For my use case, a slightly delayed unobserve would be fine, but the onBecomeUnobserved sometimes isn't firing at all on my end. 🤔

I tried with and without React StrictMode, however, I do use Suspense as well, which sounds like it could be a factor?

[jeffijoe]

I was reading on FinalizationRegistry on MDN, which states (emphasis mine):

• Developers shouldn't rely on cleanup callbacks for essential program logic. Cleanup callbacks may be useful for reducing memory usage across the course of a program, but are unlikely to be useful otherwise.
• A conforming JavaScript implementation, even one that does garbage collection, is not required to call cleanup callbacks. When and whether it does so is entirely down to the implementation of the JavaScript engine. When a registered object is reclaimed, any cleanup callbacks for it may be called then, or some time later, or not at all.

Considering this, I personally think the changes in this PR make sense. Perhaps we could even explore whether using React's scheduler is possible to avoid the use of setTimeout.

EDIT: What about a hybrid approach that uses setTimeout with a higher timeout value as a fallback for cases where FinalizationRegistry doesn't trigger?

[Nokel81]

This is quite important because it means that composing the use of react suspense and higher level mobx constructs like fromResource don't correctly clean up after themselves

[Nokel81]

@mweststrate @jeffijoe Would it be possible to revisit this work, we are having a lot of issues where onBecomeUnobserved is not firing, (we can consistently reproduce it in unit tests where an observer component has 2 suspense points and also starts trying to observe some computed data). This breaks our unsubscribe logic completely.

[Nokel81]

@Wroud FYI I have found an issue with this solution, namely it doesn't work (onBecomeUnobserved is not called) in situations with multi-suspense components. I have pushed the failing test suite to my PR branch.

[Nokel81]

Annoyingly the following code seems to always work (as a replacement for calling .get() or equivalent within an observer component)

export const useTracked = <T>(computed: IComputedValue<T>) => {
  const subscribe = useCallback((cb: VoidFunction) => autorun(() => (computed.get(), cb())), [computed]);
  const getSnapshot = useCallback(() => untracked(() => computed.get()), [computed]);

  return useSyncExternalStore(subscribe, getSnapshot);
};

And this code seems to always correctly report onBecomeUnobserved immediately on unmount. I believe that it doesn't maintain observations across suspense points, but – at least in my use case – it is more important to always have onBecomeUnobserved called at the correct times then to fake observations that between suspense points

[Nokel81]

Unfortunately after several days of trying I was unable to produce a solution that satisfied all of my unit tests and requirements along with the original unit tests of maintaining observers across suspense points. Furthermore, I was only able to do this by using a 0ms timeout – which feels both clunky and as if react doesn't give us enough control here – so for the time being I have given up.

[Nokel81]

@Wroud @jeffijoe I have updated my PR #4609 with what seems to be a working solution, it does cause additional onBecomeObservered and onBecomeUnobserved calls (namely, when a component is suspended we now stop observations).

This fixes all the unit tests, but I am still observing errors in my production code. I will get back to you if I can figure out why reliably PTAL

[jeffijoe]

@Nokel81 just for the record, I'm not a maintainer, just a long-time user 😅 But I am subscribed to this either way! Thanks for your effort in getting this resolved, but we are still at the mercy of @urugator or @mweststrate to get any potential fix released. 🙏

[Nokel81]

Understood, also I have found in production that I do actually still need the 100ms timeout. For some reason react has some intermediate renders that cause useSyncExternalStore to start new subscriptions but does not the cleanup code is not run (I haven't figure out why nor can I reproduce in unit tests yet).

It does satisfactorily work, but it is technically a race condition. So it only ensures that the unobservation happens on unmount if the unmount is >100ms after the last suspense