search

polemius / recoil-persist

Package for recoil state manager to persist and rehydrate store
https://polemius.dev/recoil-persist/
MIT License
353 stars 39 forks source link

Async/await causes a race condition in React #35

Open bartvanremortele opened 3 years ago

bartvanremortele commented 3 years ago

When using useRecoilCallback and setting multiple atoms using the same atomEffect the current implementation breaks. It will cause a race condition getting the current localStorage value because of the async/await that was added to onSet and getState.

This bug causes you to lose state persisted in localStorage

rhys-saldanha commented 3 years ago

Could you write a test that recreates this bug? Race conditions are flaky to test, but looking at example code will help explain the issue (which I think I've understood but I'm not 100% sure).

lmartins commented 3 years ago

I've encountered this Issue while looking to solve a problem I'm facing on my own app. My use case is that I have a list of preset values that I need to programmatically fill into several atoms of the same family.

Inside the useRecoilCallback, I'm looping through the array given out by the API and using set to fill in the atom value. The problem I was seeing was that only random atoms would get persisted to Local Storage, sometimes with incomplete values. Applying the change submitted by @bartvanremortele also fixed the issue for me, without unintended side-effects from what I could gather so far.

bartvanremortele commented 3 years ago

Could you write a test that recreates this bug? Race conditions are flaky to test, but looking at example code will help explain the issue (which I think I've understood but I'm not 100% sure).

I'm sorry, I don't have time to write a test. This bug can be easily reproduced by updating two atoms in the same useRecoilCallback.

stefanoTron commented 3 years ago

37 does fix this issue, but two tests fail for the async storage, which is to be expected since with the fix onSet does not take a async function anymore. I didn't go too deep but I don't see an easy solution except maybe adding a asyncStorage boolean flag to the config. 'asyncStorage` could default to true to avoid breaking changes... anyways I'm just brainstorming here, what do you think @polemius ?

polemius commented 3 years ago

I have fixed race condition for synchronize storage (like localStorage) in 2.7.0 version. But the problem still exist for async storage.