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

onSet() handler in effect not triggered when atom initialized via RecoilRoot initializeState or snapshot #767

Open
abacaj opened this issue Nov 28, 2020 · 19 comments
Assignees
Labels
bug Something isn't working

Comments

@abacaj
Copy link

abacaj commented Nov 28, 2020

RecoilRoot using initializeState:

      <RecoilRoot
        initializeState={({ set }) => set(domainState, props.savedDomainState)}
      >
        <AppRoot root={props.root} />
      </RecoilRoot>

The Atom using an effect:

const domainState = atom({
  key: "domainState",
  default: null,
  effects_UNSTABLE: [syncDomainStorage],
});

The function: syncDomainStorage is only triggered on initial render when I use initializeState, only if I remove initializeState the side-effect function is called when the atom is updated.

Any updates to domainState atom should be triggering the effect, which is not the behavior observed when used along with RecoilRoot initializeState

@drarmstr drarmstr self-assigned this Nov 28, 2020
@drarmstr drarmstr added the bug Something isn't working label Nov 28, 2020
@drarmstr
Copy link
Contributor

@abacaj - I added a unit test for the interaction of atom effects and <RecoilRoot>'s initializeState prop in #771 and it appears to be working. Note that with the current implementation the effect is run first, then the global initializeState runs and thus takes precedence setting the initial atom value. Perhaps that is the discrepancy you are observing? Do you have a reproducer for your issue on codesandbox.io?

@drarmstr drarmstr added pending Pending more information or user action and removed bug Something isn't working labels Nov 28, 2020
@drarmstr drarmstr linked a pull request Nov 28, 2020 that will close this issue
@abacaj
Copy link
Author

abacaj commented Nov 29, 2020

@drarmstr thanks for the quick reply.

Please see example here:
https://codesandbox.io/s/confident-framework-ib1kf?file=/src/index.js

I am appending to the DOM every time the effect is supposed to trigger:

import { atom } from "recoil";

function handleStateSideEffects({ onSet, setSelf, trigger }) {
  const textnode = document.createTextNode(`${trigger} - trigger called`);
  const node = document.createElement("div");
  node.appendChild(textnode);
  document.body.appendChild(node);

  onSet((newState) => {
    const textnode = document.createTextNode(`${newState} - effect called`);
    const node = document.createElement("div");
    node.appendChild(textnode);
    document.body.appendChild(node);
  });
}

export const appState = atom({
  key: "appState",
  default: { visitors: 0 },
  effects_UNSTABLE: [handleStateSideEffects]
});

There is only two times that it is appended. Any time you click the button, it should be appending to the DOM - unless effects are supposed to function that way and I misunderstand them?

The only time it works is when we remove initializeState={initializeState} from recoilRoot, after we remove it we can see that clicking the button keeps appending to the DOM because of onSet

@drarmstr
Copy link
Contributor

Got it, so the effect is executing properly, but the issue is the onSet() handler being called when initialized via a snapshot.

@drarmstr drarmstr added bug Something isn't working and removed pending Pending more information or user action labels Nov 30, 2020
@drarmstr drarmstr changed the title effects_UNSTABLE not triggering when using RecoilRoot initializeState onSet() handler in effect not triggered when atom initialized via RecoilRoot initializeState or snapshot Nov 30, 2020
@kwoktung
Copy link

kwoktung commented Jan 6, 2021

any solutions?

@mthines
Copy link

mthines commented Jan 8, 2021

I found a temporary solution, which is to skip the initializeState logic, and just use the regular useSetRecoilState before mounting the rest of the app.

This is really not ideal, but it does the trick for me, until the problem has been fixed.

Here's my Recoil Provider:

import localForage from 'localforage';
import React, { ReactNode, useEffect } from 'react';
import { RecoilRoot, useSetRecoilState } from 'recoil';

import { useLoading } from 'utils/hooks/loading';

import Loading from 'components/UI/loading/loading';

/** Recoil Provider and persistor */
const RecoilProvider = ({ children }: { children: ReactNode }) => {
  return (
    <RecoilRoot >
      <RecoilPersist>{children}</RecoilPersist>
    </RecoilRoot>
  );
};

export default RecoilProvider;

const RecoilPersist = ({ children }: { children: ReactNode }) => {
  const setAtom1 = useSetRecoilState(atom1);
  const setAtom2 = useSetRecoilState(atom2);
  const [isLoading, setIsLoading] = useState<boolean>(true);

  const persistedAtoms = [
    {
      key: 'atom1',
      setter: setAtom1,
    },
    {
      key: 'atom2',
      setter: setAtom2,
    },
  ];

  const loadPersistedAtoms = () => {
    const loadPersisted = async () => {
      try {
        for await (const atom of persistedAtoms) {
          const persistedData = await localForage.getItem<any>(atom.key);
          atom.setter(JSON.parse(persistedData));
        }

        setIsLoading(false);
      } catch (err) {
        setIsLoading(false);
        return;
      }
    };

    loadPersisted();
  };

  useEffect(loadPersistedAtoms, []);

  return <>{!isLoading ? children : <Loading />}</>;
};

FYI @kwoktung

@drarmstr
Copy link
Contributor

drarmstr commented Jan 8, 2021

A workaround for now could also be to just initialize the atom state in the effect and avoid using the initializeState prop. This is the preferred approach anyway for persistence as it can better handle dynamic atom families and multiple/composable persistence policies.

@mthines
Copy link

mthines commented Jan 9, 2021

@drarmstr That sounds better. Would that be using the setSelf?
If not, can you you provide an example of how to set the initial value in the effect instead?

Awesome work guys 💪

@mthines
Copy link

mthines commented Jan 9, 2021

@drarmstr Sorry, never mind I figure out by following the docs 🤷 ...

I had the issue that I needed to await to get the value from localForage indexedDB, so I ended up wrapping it in a async function.

Here's how it ended

import localForage from 'localforage';
import { AtomEffect, DefaultValue } from 'recoil';

/** Check if there's an initial value persisted and load it on set  */
const loadPersisted = async <T>({ key, setSelf }: { key: string; setSelf: Parameters<AtomEffect<T>>['0']['setSelf'] }) => {
  const savedValue = await localForage.getItem<string>(key);

  if (savedValue != null) {
    setSelf(JSON.parse(savedValue));
  }
};

/**
 * Localstorage Atom Effect
 *
 * Add to `effects_UNSTABLE` to persist atom.
 * Side-effect for Atom Manipulating
 * @see https://recoiljs.org/docs/guides/atom-effects/
 */
export const persistAtomEffect = <T>(key: string): AtomEffect<T> => ({ setSelf, onSet }) => {
  loadPersisted({ key, setSelf });

  onSet(async (newValue) => {
    if (newValue instanceof DefaultValue) {
      localForage.removeItem(key);
    } else {
      localForage.setItem(key, JSON.stringify(newValue));
    }
  });
};

Thanks :)

@drarmstr
Copy link
Contributor

drarmstr commented Jan 9, 2021

Note that Atom Effect functions are themselves not async and should not return a Promise. You can schedule async calls to setSelf() in them, but that will only set the atom value asynchronously after initial render, not initialize the atom state for initial render (which would use the atom's default value). If you need to get the initial state asynchronously you could synchronously call setSelf() with an async Promise. This will cause the atom to be initialized in a pending state that will leverage Suspense for the initial render. The approach you want is up to you, but be aware of the differences.

const loadPersisted = <T>({ key, setSelf }) => {
  setSelf(localForage.getItem(key).then(JSON.parse));
};

@mthines
Copy link

mthines commented Jan 9, 2021

@drarmstr Amazing, I was wondering how to approach this properly.
Thanks!! 💪

@mthines
Copy link

mthines commented Jan 12, 2021

@drarmstr I've added a PR #828 for an update docs with the asynchronous / promise examples we discussed.
I would have liked that when looking through the docs, so maybe other would as well.

Let me know if there's something badly explained.

@vitorcamachoo
Copy link

Hii, any update of initializeState not triggering onSet event on effects_UNSTABLE ?

@notnooblord
Copy link

notnooblord commented Feb 18, 2021

Yep, provided workarounds won't work for example with SSR, where I want to initialize with initializeState prop on inital SSR render, but keep the effects_UNSTABLE onSet() callback functionality.

@drarmstr
Copy link
Contributor

@alexunix - You should be able to initialize state in the atom effect with a synchronous setSelf() call that will be compatible with SSR. This should hopefully avoid the need for initializeState prop.

@notnooblord
Copy link

Hey! Thanks for reply @drarmstr .

I'm using cookies from request header to set Atoms state on initial Server-side render.
And I want effects_UNSTABLE onSet() to mutate cookie client-side when state changes.

My problem is probably framework-specific since nextjs framework gets ServerSide parameters and passes them to components like this <App {...serversideprops}></App> so initializeState is very helpful.
And I couldn't figure out how to pass current cookie header from request to atom effects_UNSTABLE setSelf :(

Would've been perfect combo to have peristent state in cookies if only onSet() in effects_UNSTABLE could be used with initialState.
I'm just using regular useEffect to set a cookie after changing atom state for now.

Added example here.
_app.js - here I pass cookies to initializeState on initial server-side render. (Breaks setSelf())
index.js - example of current behaviour.

@DRoghanian
Copy link

Bump! I'm in the same boat as @Noo8lord - I have a NextJS SSR app in production which relies on session tokens to understand who the user is, fetch data on the server, and initialize recoil state in recoil root. Was hoping that I could initialize atoms from the server as I've been doing, and then use the onSet() atom effect to manage subsequent recoil state persistence and side effects, but it doesn't seem to be possible.

As mentioned somewhere above, the workaround of enabling effects by dropping initializeState in favor of asynchronous setSelf effects for each atom isn't useful in my case because it seems to negate the utility of SSR, and because I can't give the atom effects the context they need to fetch the right data on the server.

Appreciate any help / follow up and happy to provide more info.

@lucax88x
Copy link

same problem as @Noo8lord and @DRoghanian .

in the end I managed to fix the issue by using a combination of @mthines and using a singleton service to pass server cookies.

Yeah, I set server cookies (req.headers.cookie) to a singleton service so my atom can read from this service.

Ugly as f, but until InitializeState works as expected (if it will ever work...) or.. workarounds.

@drarmstr drarmstr removed the help wanted Extra attention is needed label Dec 30, 2021
@drarmstr drarmstr linked a pull request Dec 30, 2021 that will close this issue
@drarmstr drarmstr linked a pull request Dec 31, 2021 that will close this issue
@drarmstr
Copy link
Contributor

drarmstr commented Dec 31, 2021

Should be fixed with #1511 and tested with #1519 for 0.6 release.

Note you'll also be able to initialize atoms using props with the upcoming recoil-sync library and effects instead of initializeState. (https://github.com/facebookexperimental/Recoil/pull/1462/files)

@drarmstr drarmstr linked a pull request Dec 31, 2021 that will close this issue
@neil-morrison44
Copy link

Is this issue fixed?

I've got an atomEffect which tracks the "history" of an atom by pushing into an array each onSet but it doesn't update when changed within the RecoilRoot initializeState

(I'm using Recoil 0.7.5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment