-
-
Notifications
You must be signed in to change notification settings - Fork 691
fix: cleanup pending promises on unmount #3125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
commit: |
|
Playground | Link |
---|---|
React demo | https://livecodes.io?x=id/BWGAGB9MP |
See documentations for usage instructions.
Thanks. I'll have a closer look later. |
mounted = undefined | ||
mountedMap.delete(atom) | ||
storeHooks.u?.(atom) | ||
// clean up pending promises |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still it seems to work better now than before so I would not mind sticking with this partial solution. Can start a new discussion to cover more cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having second thoughts about it now:
I've tried to think of possible reasons behind that recomputeInvalidatedAtoms -> getMountedOrPendingDependents
combo: essentially, why we are making an unmounted atom with a pending promise react eagerly to its dependencies' changes.
Came up with this Suspense
case:
- An async atom is fetched without mounting during the first Suspense render, it suspends with a promise
- Its sync dep changes during the pending phase thus making the initial promise stale
- In this case we would like it to trigger its abort signal and recompute a new value ASAP
- However, if the unmounted-but-pending atom wasn't eagerly recomputed, it wouldn't be until the next render that it's fetched again. In most cases the render only happens when the initial, now-stale promise is resolved. So the atom won't realize it's stale until then and won't trigger another getter: so the loading will take longer and the abort signal won't fire.
This particular case will not be affected by the PR. Although this similar one will:
- An async atom is fetched and successfully mounted after its initial promise resolution in ComponentA
- Then a dep is changed, it start recomputing a new promise
- ComponentB starts rendering and suspends with that promise
- While it's still pending, ComponentA is unmounted
- Then, while it's still pending, the dep changes again
- If we stopped the eagerness on unmount: ComponentB is now suspended on a stale promise with no way of knowing it's stale until it's finished
See https://stackblitz.com/edit/vitejs-vite-qkbef5vo?file=src%2FApp.tsx:
With original jotai it outputs:
> ⏳ start fetch [dep=0, id=1]
> ✅ resolve fetch [dep=0, id=1]
> mount A
StrictMode> unmount A
StrictMode> mount A
> ⏳ start fetch [dep=1, id=2]
> unmount A
> ⏳ start fetch [dep=2, id=3]
> 🛑 abort fetch [dep=1, id=2]
> ✅ resolve fetch [dep=2, id=3]
> mount B
StrictMode> unmount B
StrictMode> mount B
With npm i https://pkg.pr.new/jotai@3125
:
> ⏳ start fetch [dep=0, id=1]
> ✅ resolve fetch [dep=0, id=1]
> mount A
StrictMode> unmount A
StrictMode> mount A
> ⏳ start fetch [dep=1, id=2]
> unmount A
> ✅ resolve fetch [dep=1, id=2]
> ⏳ start fetch [dep=2, id=3]
> ✅ resolve fetch [dep=2, id=3]
> mount B
StrictMode> unmount B
StrictMode> mount B
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I have a feeling now that even the first case I've mentioned doesn't really work in some cases. Working on a reproduction link now to prove the point
It all feels like just another implication of performing side effects in render: the code becomes dependent on the fact if render is called at a specific time or not. IMO it feels like a react-specific implementation detail. I know the talk that getters are supposed to be pure, though the fact that jotai supports async atoms and treats promises differently contradicts it in a sence, since starting any async operation, memoized or not, is strictly speaking a side-effect. Thus I take it as a jotai's fundamental design choice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the link: https://stackblitz.com/edit/vitejs-vite-2rssqjqx?file=src%2FApp.tsx
What I'm trying to prove there is that it's generally impossible to optimally refresh a pending atom when its component is suspended. When the dep changes there is no way to tell:
- if a component is suspended on some other longer promise => We would've wanted to recalculate its promise, however we can only judge about its suspension based on our own atom's promise state
- if a component is still interested it the result of the promise => The component's render could be cancelled mid-suspense if some condition was switched in a parent render. My example doesn't demonstrate this effect, though I can do it if need be
Now it feels that optimizing for such cases isn't really a good idea and instead it would suffice to recommend subscribing to the atoms explicity outside of Suspense boundary if they really need that Suspense-time eagerness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jotai internals does not resolve promises, that is done in useAtomValue. In the future, I think it makes sense to stop unwrapping promises by default and to return the original promise from useAtomValue instead. Then, it would be up to the implementer to wrap with use
.
- Do not use
use
in useAtomValue (migration path: add{ use: true }
option which istrue
by default in v2, and warns it)
This is one of the ideas for v3 described in #2889
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thats true. I wasn't stating anything about jotai resolving promises internally, though. What I meant to say:
- If we start async (therefore impure) operations in render, we make ourselves dependent on the render call times which react tries to keep a semi-private implementation detail.
- Jotai now has pending promise checks when calculating dependencies which seems to be some optimization of the 2 cases I've mentioned, around suspending on an unmounted atom's promise -> dep change -> mount. (Actually, that's just my guess about the reasons, the actual reasoning behind the code might've been different, would love to know more)
- Still, if we tried to optimize the 2 cases, our efforts are bound to fail in some cases or get outdated after some random react changes, as long as render stays impure. And it is generally impure now, when an atom is async, no matter if we
await/use(promise)
it or not. So even after{ use: false }
the renders will stay impure
I know and appreciate the idea about { use: false }
. It will help with this situation to some extent, allowing us to additionally useAtomValue({ use: false })
outside of suspense boudary, where render-mount-unmount order is a bit more straightforward. It still would be prone to some corner cases unless we only do side-effects in useEffects as react wants us to.
eec1e18
to
d3b23e6
Compare
Related Bug Reports or Discussions
Fixes #
#3124
Summary
Cleanup pending promises on unmount.
Check List
pnpm run fix
for formatting and linting code and docs