pmndrs / jotai

👻 Primitive and flexible state management for React
https://jotai.org
MIT License
18.72k stars 617 forks source link

[atomWithHash] multiple atoms connected to url hash overwrite each other sometimes #1109

Closed pakettiale closed 2 years ago

pakettiale commented 2 years ago

We have a strange problem with our react application where multiple atomWithHash mounting at the same time cause the subscribe function of atomWithHash to behave badly. But only sometimes. I'm also thinking whether the atomWithHash is suitable for our use.

The application is composed like this:

<Router>
  <Route1 path="page1">
    <Page1 uses page1atom = atomWithHash("page1", initialState1)>
      <Filters uses filterAtoms = atomFamily(key => atomWithHash("filters", initialState)) key is "page1" />
    </Page1>
  </Route1>

  <Route2 path="page2">
    <Page2 uses page2atom = atomWithHash("page2", initialState2)>
      <Filters uses filterAtoms = atomFamily(key => atomWithHash("filters", initialState)) key is "page2" />
    </Page2>
  </Route2>

  <Route3 path="page3">
  ...
</Router>

The problem:

  1. Modify state in Page1 and the Filters inside Page1, url hash is updated correctly
  2. Navigate to some other page in the application so that the Page1 component unmounts
  3. Navigate back to Page1, url is set to "localhost/page1" so the hash is empty
  4. Page1 mounts, the previous state of the page1atom flashes briefly after which it changes into the initialState1, the state of the filterAtom("page1") is remembered correctly. Hash now contains filterAtom state but nothing from page1atom.

Curiously enough this same problem does not happen in the Page2 component even though they are quite similar components (both display a list of things). I guess there is some kind of data race happening when both atoms try to read/write the urlHash at the same time.

If I modify the atomWithHash like this so that the subscribe field of hashStorage does not initialize the value in case the corresponding part of hash is missing, then the problem disappears. But now the hash is updated only after I make some state change.

    subscribe: (key: string, setValue: (value: Value) => void) => {                                                  
      const callback = () => {                                                                                       
        const searchParams = new URLSearchParams(location.hash.slice(1));                                            
        const str = searchParams.get(key);                                                                           
        if (str !== null) {                                                                                          
          setValue(deserialize(str));                                                                                
        } else {                                                                                                     
          // setValue(initialValue);                                                                                 
        }                                                                                                            
      };

What we are trying to achieve with jotai and atomWithHash is:

  1. Persisting the page and filter state when navigating across the application
  2. Writing the current state into url so that users can send links which loads the page with same filters and whatever state is in the pageXatom

Is atomWithHash an overkill for the point 2? We only care about reading the url when the application is first loaded / refreshed. Otherwise the URL needs only be write only. Thus listening to hashchange events seems unnecessary. And if I understood the code correctly this can cause unnecessary rerenders when other atomWithHashes are updating the hash.

P.S. jotai is a very nice library, thanks for creating it :)

Edited the original post: urlParamStorage into hashStorage, this was an artefact left from my own testing

dai-shi commented 2 years ago

Thanks for using jotai and investigating atomWithHash. To be honest, I haven't thought about using it with router. So, it's an open field.

a) atomWithHash might be able to improved to handle your use case, or b) it may not suit for you because hashchange is the key for atomWithHash (in such a case another atomWithFoo based on atomWithStorage might be possible.)

I'm not sure which is good for now. I don't think I fully understand the problem yet. Would it be possible for you to create a minimal example with codesandbox? What's the router library you use?

pakettiale commented 2 years ago

Fix our problem some time ago and commenting here in case someone else stumbles upon this issue.

This was actually due to using a stale setter function and had nothing to do with routing. Our state contained an object with many fields and to make state updates cleaner we had a wrapper for setter like this

  const [urlParams, _setUrlParams] = useAtom(FilterState(name));
  const setUrlParams = (input: Partial<UrlParams>) =>
    _setUrlParams({ ...urlParams, ...input });

The setUrlParams was then passed to multiple child components. Stale state setters then created issues when multiple setUrlParams calls were made in a short amount of time.

To fix this we could have changed setUrlParams to this

  const setUrlParams = _setUrlParams((input: Partial<UrlParams>) =>
    { ...urlParams, ...input });

But for reusability we created a record atom that handles partial state updates by filling the missing top level fields from the existing state.

import { Atom, useAtomValue, useSetAtom, WritableAtom } from 'jotai';
import { Scope, SetAtom } from 'jotai/core/atom';
import { useCallback } from 'react';

type Awaited<T> = T extends Promise<infer V> ? Awaited<V> : T;

/**
 * QoL hook for record atoms. Setter updates the state by default instead of
 * replacing it. Setter uses callback form to prevent stale setter
 * functions in child components.
 */
export function useRecordAtom<
  Value,
  Update,
  Result extends void | Promise<void>
>(
  atom: Atom<Value> | WritableAtom<Value, Update, Result>,
  scope?: Scope
): [Awaited<Value>, SetAtom<Update, Result>] {
  /**
   * jotai has problems with their type casting which forces us to use ts-ignore
   * see here: https://github.com/pmndrs/jotai/blob/042a9027bf59662962d2db73b9639734f4afdf3f/src/core/useAtom.ts#L29
   */
  // eslint-disable-next-line @typescript-eslint/ban-ts-comment
  // @ts-ignore
  const setter = useSetAtom(atom as SetAtom<Update, Result>, scope);
  const memoizedRecordSetter = useCallback(
    (newValue: Value) => {
      setter((oldValue: Value) => ({ ...oldValue, ...newValue } as Value));
    },
    [setter]
  );

  return [
    useAtomValue(atom, scope),
    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
    // @ts-ignore
    memoizedRecordSetter,
  ];
}