fix(core): Allow retries for errored async reads #705
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/jotai/5YLw1D1DieaXtDJsxuzLoWQCfC59 |
|
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. Latest deployment of this branch, based on commit 3362181:
|
| @@ -95,32 +70,3 @@ it('writable count state', async () => { | |||
| fireEvent.click(getByText('button')) | |||
| await findByText('count: 9') | |||
| }) | |||
|
|
|||
| // FIXME we would like to support retry | |||
| it.skip('count state with error', async () => { | |||
There was a problem hiding this comment.
This test wasn't valid - an observable can't recover from an error state. For the notion of "retrying" observables to work, the error would never reach atomWithObservable, so the retrying logic is out of scope for jotai.
There was a problem hiding this comment.
So, I didn't know about the observable error behavior, and created this test. But, it's an expected behavior.
Now, you find the actual issue of error async reads, and fix it. Very very nice.
| @@ -177,6 +177,7 @@ export const createStore = ( | |||
| return | |||
| } | |||
| atomState.c?.() // cancel read promise | |||
| delete atomState.e // read error | |||
There was a problem hiding this comment.
This was being done when writing to a sync atom, but not an async atom.
There was a problem hiding this comment.
Wow! Nice catch!
Feels like you have fairly deeper understanding of core.
Pinpickle
force-pushed
the
fix-error-recovery
branch
from
e878929
to
e018bd2
Compare
| @@ -177,6 +177,7 @@ export const createStore = ( | |||
| return | |||
| } | |||
| atomState.c?.() // cancel read promise | |||
| delete atomState.e // read error | |||
There was a problem hiding this comment.
Wow! Nice catch!
Feels like you have fairly deeper understanding of core.
| @@ -95,32 +70,3 @@ it('writable count state', async () => { | |||
| fireEvent.click(getByText('button')) | |||
| await findByText('count: 9') | |||
| }) | |||
|
|
|||
| // FIXME we would like to support retry | |||
| it.skip('count state with error', async () => { | |||
There was a problem hiding this comment.
So, I didn't know about the observable error behavior, and created this test. But, it's an expected behavior.
Now, you find the actual issue of error async reads, and fix it. Very very nice.
dai-shi
changed the title
tests/error.test.tsx
Outdated
| @@ -492,7 +492,7 @@ describe('error recovery', () => { | |||
| await findByText('Value: 1') | |||
| }) | |||
|
|
|||
| it.only('recovers from async errors', async () => { | |||
There was a problem hiding this comment.
Oops - thank you!
Yes, that's my plan too. Seems like a test fails. Is this because our tests are not stable enough because we don't properly use fake timers? |
This was because of less mode and meant the atom state was shared between tests I've fixed it now - could you run the workflow again? |
I see. |
When changing a dependency for an atom in an error state, the derived atom's read function is re-run.
We'd expect that if that re-run is successful,
useAtomwill return the new value.This is currently the case with sync atoms, but not async atoms. This PR fixes that, and adds tests (the async variant was failing, but the sync one wasn't).
Fixes #601