Skip to content
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

Using promise to setSelf in atom's effect #1107

Closed
xotahal opened this issue Jul 8, 2021 · 4 comments
Closed

Using promise to setSelf in atom's effect #1107

xotahal opened this issue Jul 8, 2021 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@xotahal
Copy link
Contributor

xotahal commented Jul 8, 2021

Hey folks! First of all thanks for the Recoil. Appreciate it! 🙏

I am having this weird behaviour with using promise in atom's effect. Based on this part of doc. Let me talk you through this codesandbox - https://codesandbox.io/s/set-self-with-promise-b0z6l?file=/src/App.js

1. setSelf is not updating selector

How to reproduce

  • press execute pre-fetch
  • it Suspenses with Value loading... for 500ms and then it displays number 8 (because promise on line 58 got resolved)
  • after 1500ms there is another promise that gets resolved and should update the value to number 10 (on line 31) - but it doesn't (you can see this in console)
  • then you can press 2. button to update value, you will see console logs with number incrementing but values stays the same

2. setSelf twice

How to reproduce

  • comment out the setSelf on line 58 (get rid of init promise)
  • press prefetch
  • you won't see Value loading... because it doesn't suspense, you will see 1 instead (default on line 77) for 1500ms then you will see number 10 (timer on line 31)
  • when you press 2. button it will updates the value correctly, but you can see this console log twice - console.log("dataAtom setSelf onchange#", instance, value);
  • seems that the effect was registered twice

Please let me know if I can help with this. Or if I am doing anything wrong.

@drarmstr drarmstr added the bug Something isn't working label Jul 12, 2021
@drarmstr
Copy link
Contributor

drarmstr commented Jul 12, 2021

  1. doesn't seem to be related to using a Promise (just set it to 8 directly), but it also seems to work if you show the value of the atom directly instead of the selector, so maybe related to selector subscription Selector is not updated when atom changes #1072 (cc @csantos42)

csantos42 pushed a commit to csantos42/Recoil that referenced this issue Jul 27, 2021
…nd with transactions

Summary:
Addresses facebookexperimental#1107 item 2 (this behavior is expected).

TLDR; an atom effect may run multiple times if a snapshot is used to read the atom before root store is aware of the atom, but if that atom is read using transactions it will only run once regardless of whether the root store has already come across that atom.

When an atom is initialized for the first time in a snapshot, knowledge of that initialization will not make its way to the global store as snapshots are independent and do not communicate with the global store. This means an atom effect may initialize multiple times when an atom is read for the **first time** in that snapshot (or more specifically before the global store has knowledge that the atom initialized). If the global store is already aware of the atom, then subsequent snapshots that are created from the global store will not result in the effect being called multiple times.

When an atom is initialized for the first time from a transaction, the global store will be made aware of this initialization as transactions are not backed by snapshots and will mutate the root store. So if for whatever reason it's critical that the effect only runs once per root, transactions should be preferred over snapshots (although this is not officially recommended as transactions are not meant to be used for side effects).

Differential Revision: D29949089

fbshipit-source-id: 58a037098176b05e9d8e920f85b3feeaf104897d
@drarmstr
Copy link
Contributor

drarmstr commented Jul 27, 2021

UPDATE (@xotahal ):

  1. A fix for this issue is landing shortly, just pending a unit test

  2. Seems like this behavior is actually expected. The documented pre-fetch example uses a selectorFamily() to model an async query. In your example you used an atomFamily() with an effect. Since the pre-fetch is operating on a Snapshot, the pre-fetch request is causing the atom in the family to be used for the first-time and initialized. But, that's only for that state snapshot and doesn't affect the actual live state in the <RecoilRoot>. When the atom in the family is accessed again when rendering from the actual <RecoilRoot> context, then it is essentially seen from that state store for the first time and initializing the actual atom in the live store, that is why you are seeing the effect run twice. ([docs] qualify pre-fetching example #1131 )

If you want to do this kind of pre-fetching with atom families, it may be a better option to use useRecoilTransaction_UNSTABLE() instead of useRecoilCallback(). Something for us to test with the upcoming release. (#1130 )

facebook-github-bot pushed a commit that referenced this issue Jul 29, 2021
…nd with transactions (#1130)

Summary:
Pull Request resolved: #1130

Addresses #1107 item 2 (this behavior is expected).

TLDR; an atom effect may run multiple times if a snapshot is used to read the atom before root store is aware of the atom, but if that atom is read using transactions it will only run once regardless of whether the root store has already come across that atom.

When an atom is initialized for the first time in a snapshot, knowledge of that initialization will not make its way to the global store as snapshots are independent and do not communicate with the global store. This means an atom effect may initialize multiple times when an atom is read for the **first time** in that snapshot (or more specifically before the global store has knowledge that the atom initialized). If the global store is already aware of the atom, then subsequent snapshots that are created from the global store will not result in the effect being called multiple times.

When an atom is initialized for the first time from a transaction, the global store will be made aware of this initialization as transactions are not backed by snapshots and will mutate the root store. So if for whatever reason it's critical that the effect only runs once per root, transactions should be preferred over snapshots (although this is not officially recommended as transactions are not meant to be used for side effects).

Reviewed By: drarmstr

Differential Revision: D29949089

fbshipit-source-id: 41732ae8e8b41d32393bf4ad7d3559731dadbc19
csantos42 pushed a commit to csantos42/Recoil that referenced this issue Jul 29, 2021
…itHub issue facebookexperimental#1107

Summary:
Description of setup:
- A selector is read from a snapshot, and the selector depends on an atom that has an atom effect which calls `setSelf()` synchronously and adds some code to call `setSelf()` asynchronously later on, and the first time the app is reading the atom is in that snapshot (to reproduce it's important that the app has not read the atom before and therefore the atom has not initialized before, meaning the effects for that atom have not run yet). Then, the selector is read using a hook.

Expected behavior
- The atom effect runs twice as it is first initialized by the snapshot and then by the root store via a hook. The async calls to `setSelf()` should lead to re-renders with an updated selector value as its dependent atom has changed in value.

Issue:
- The atom effect only initializes once, so only the snapshot is aware of the asynchronous `setSelf()` calls but the root store is not, which results in the async calls to `setSelf()` being ignored and the UI does not update.

Differential Revision: D29886554

fbshipit-source-id: b6270fd8a8a3299b37f4a79de4822d7f7c48cdaf
@drarmstr
Copy link
Contributor

Issue 1 fixed with #1135

facebook-github-bot pushed a commit that referenced this issue Jul 29, 2021
…itHub issue #1107 (#1135)

Summary:
Pull Request resolved: #1135

Description of setup:
- A selector is read from a snapshot, and the selector depends on an atom that has an atom effect which calls `setSelf()` synchronously and adds some code to call `setSelf()` asynchronously later on, and the first time the app is reading the atom is in that snapshot (to reproduce it's important that the app has not read the atom before and therefore the atom has not initialized before, meaning the effects for that atom have not run yet). Then, the selector is read using a hook.

Expected behavior
- The atom effect runs twice as it is first initialized by the snapshot and then by the root store via a hook. The async calls to `setSelf()` should lead to re-renders with an updated selector value as its dependent atom has changed in value.

Issue:
- The atom effect only initializes once, so only the snapshot is aware of the asynchronous `setSelf()` calls but the root store is not, which results in the async calls to `setSelf()` being ignored and the UI does not update.

Reviewed By: drarmstr

Differential Revision: D29886554

fbshipit-source-id: e06cd3643bb7e5f8d4a3e666e90e3edfb36cd140
@drarmstr
Copy link
Contributor

drarmstr commented Jul 29, 2021

Fix for first issue available with Recoil 0.4.0. Tested and confirmed that using useRecoilTransaction_UNSTABLE() also solves the duplicate effects issue (https://codesandbox.io/s/set-self-with-promise-forked-d66bl?file=/src/App.js). Though, note that transactions do not quite yet support selectors.

@drarmstr drarmstr self-assigned this Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants