fix(mobx-react-lit): dispose reactions right after render in StrictMode and Suspense #3777
Conversation
…de and Suspense onBecomeUnobserved never triggered with observer component mobxjs#3774
|
|
Few quick notes without giving it proper look: |
|
I see your point. In this PR useLayoutEffect is used only for:
I tested this implementation on the huge project with use of Suspense and haven't noticed any issues. But it seems that this implementation works the same way in the most cases. |
|
I will try to move logic from |
|
This implementation doesn't re-render components if the reaction has been disposed of. How it works:
I've tested this implementation with extra logs, and there are no extra re-renders |
|
@urugator I also added some extra tests for abandoned and suspended components to ensure that reactions are disposed of correctly |
|
Are you aware of the 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. |
|
Yes, it's the same idea as There is a small difference between the original implementation and this one; despite using Are there any blockers to use |
|
Here is an example: 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. |
Atm
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: |
|
idea: we can probably use
so when we dispose of a reaction because of timeout, we can also change EDIT: Maybe it's only works for transactions |
That's what we do: mobx/packages/mobx-react-lite/src/useObserver.ts Lines 55 to 64 in 273e017 |
|
It's equal to run it in |
|
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 |
|
I see. So, potentially, we can move to and get rid of because that should never happen (react throws away uncommited component, because it sees a new version) ?? |
|
Yes, that's exactly what I mean |
|
With shorthand implementation, I can notice this warning in the tests: but I call |
|
If it works, I am personally open to that change. But it doesn't help much with the original issue or does it?
If these are from the deprecated
Where? In finalizer? |
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: |
|
there new two from |
|
I've pushed these small changes, I haven't noticed any This won't fix the original issue (dispose of reaction right when the component is mounted or abandoned) |
Thoughts why it's three but not 2:
|
|
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:
***@***.***>
|
|
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 :). |
|
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) |
This pull request replaces
observerFinalizationRegistrywith the combination ofuseLayoutEffectwithrequestAnimationFrameThe 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