jotaijs / jotai-effect

A Jōtai utility package for reactive side-effects
MIT License
110 stars 3 forks source link

Atomeffects never run while read within a suspended suspense boundary #41

Closed scamden closed 3 months ago

scamden commented 3 months ago

See the example here: https://codesandbox.io/p/sandbox/jotai-effect-on-async-r2s4jl?file=%2Fsrc%2FApp.tsx%3A27%2C50

dmaskasky commented 3 months ago

May I know some more specifics of your use-case? The example you provided is fairly generic. Hopefully for your specific use-case we can come up with an acceptable workaround to this issue.

The issue you describe above is a result of how hooks behave with suspense in React.

More details on this behavior

Because asyncyEffectAtom returns a promise, the component suspends and no further atoms will run until the component resumes. The component never resumes because the atomEffect atom never fires. Remember, atomEffects are just atoms; but I agree this is an implementation detail.

Here is your example extended to demonstrate that testAtom also does not run when the component suspends. https://codesandbox.io/p/sandbox/jotai-effect-on-async-forked-tp83wh?file=%2Fsrc%2FApp.tsx%3A29%2C1

Understanding why the atomEffect never reads even when it is mounted before asyncyEffectAtom mounts is a little more nuanced. In Jotai, atoms read for the first time before their onMount is called. atom.onMount actually runs in a useEffect, which is synchronous after component render and after the component suspends. atomEffect needs to run its onMount and run read again in order have access to the get and set functions needed to fire the EffectFn passed in.

When the component is first rendered

  1. atomEffect reads for the first time
  2. asyncyEffectAtom reads and suspends the component
  3. the component is suspended
  4. the component would have finished rendering
  5. the component would have run useEffect
  6. atomEffect.onMount would have fired
scamden commented 3 months ago

mmm i think i understand, my mental model would be that the jotai would suspend after finishing all atom evaluations but this makes a lot of sense actually. (also your sandbox fork isn't public but i believe you!)

Basically my use case is to have the atomEffect set up a subscription and then set the result of that subscription to a primitive atom. The primitive atom should start out as an unresolved promise and then resolve on the first subscription callback, then it can be set to synchronous values there after. (This is my attempt to build my own jotai-urql query hook because the library version uses atomWithObservable and atomwithObservable creates infinite loops and permanently suspended suspense boundaries for us in combination with jotai-scope sadly.. we were talking about it here: https://github.com/jotaijs/jotai-scope/issues/25#issuecomment-2125372288)

dmaskasky commented 3 months ago

oops, ok its public now. https://codesandbox.io/p/sandbox/jotai-effect-on-async-forked-tp83wh?file=%2Fsrc%2FApp.tsx%3A29%2C1

dmaskasky commented 3 months ago

my mental model would be that the jotai would suspend after finishing all atom evaluations

I don't know for sure, but I suspect that jotai will migrate to React 19's use hook at some point. I could possibly be wrong about this, but I've noticed a lot of @dai-shi's state libraries moving to not internally resolving promises.

const asyncAtom = atom(async (get) => fetch(url))
function Component() {
  const result = use(useAtomValue(asyncAtom))
}

atomEffect set up a subscription and then set the result of that subscription to a primitive atom. The primitive atom should start out as an unresolved promise and then resolve on the first subscription callback, then it can be set to synchronous values there after.

You could put the effect in a parent component that doesn't suspend. Tbh, figuring out where to register the atomEffect feels like an unnecessary and annoying part of the api. But since atoms are just definitions and an atom + store represents a reactive value, it makes the most sense to declare your effects under the jotai provider, or as close to the store as possible.

const valueAtom = atom(new Promise())

const subscriptionEffect = atomEffect((get, set) => {
  const unsubscribe = subscribeToUrql((data) => {
    set(valueAtom, data)
  })
  return unsubscribe
})

function MyComponent() {
  const value = useAtomValue(valueAtom)
}

function ParentComponent() {
  useAtom(subscriptionEffect)

  return <Suspense fallback={<>Loading...</>}>
    <MyComponent />
  </Suspense>

}
dmaskasky commented 3 months ago

Anyways this makes sense right? If a component is suspended, then stuff inside should be suspended too. In fact, I believe the negation, _if a component is suspended, then stuff inside should not be suspended", would be incorrect behavior.

My recommendation is to place your effects as close to the store root as possible.

Anyways, closing this ticket for now. Let me know if you would like to discuss further, and I can reopen the ticket if necessary. Also feel free to reach out to me on Discord if you would like to chat more realtime.

scamden commented 3 months ago

Ya i think you're right that since this mirrors a useEffect the behavior is correct.

In terms of putting subscriptions near the store that would sorta defeat the purpose. The whole idea of the urql atom is to have an atom that queries and reacts to the urql cache whenever it's mounted as a dep and not otherwise. That link gets lost if we have to move the subscription away from the atom. I'll have to think a bit harder :)

Thanks for all the thoughts!

dmaskasky commented 3 months ago

Another idea is to do store.sub(yourAtomEffect), but I suppose this won't work for you because if you have access to store, then you could just directly do store.set in the urlql subscription.

dmaskasky commented 3 months ago

One more idea is to hold reading the valueAtom until the effect has mounted.

const baseValueAtom = atom(new Promise())
const effectReadyAtom = atom(false)
const subscriptionEffect = atomEffect((get, set) => {
  set(effectReadyAtom, true)
  const unsubscribeURQL = subscribeURQL((data) => {
    set(baseValueAtom, data)
  })
 return () => {
  unsubscribeURQL()
  set(effectReadyAtom, false)
  }
}
const valueAtom = atom((get) => {
  get(subscriptionEffect)
  if (get(effectReadyAtom)) {
    return get(baseValueAtom)
  }
  return null;
})
scamden commented 3 months ago

Ya that's what I have actually but I'm wanting it to start out in suspense and not have null in the valueAtom's type.

Appreciate all these ideas!

this is one approach that works but it warns that I shouldn't be setting self synchronously ha


export function makeUrqlQueryAtom<Data, Variables extends Record<string, any>>({
  scope,
  getVariables,
  query: document,
  getPaused,
}: {
  query: DocumentInput<Data, Variables>;
  getVariables: (get: Getter) => Variables;
  scope: <A extends Atom<any>>(atom: A) => A;
  getPaused?: (get: Getter) => boolean;
}) {
  type DataResult = {
    data?: Data | undefined;
  };

  const lastResultAtom = scope(atom<null | DataResult>(null));

  const variablesAtom = scope(atom(getVariables));

  const memoizedSubscriptionAtom = atom<
    DataResult | Promise<DataResult>,
    [DataResult | null],
    void
  >(
    (get: Getter, { setSelf }): Promise<DataResult> => {
      setSelf(null);
      const source = get(urqlClientAtom).query(document, get(variablesAtom));
      source.subscribe(setSelf);
      return source.toPromise();
    },
    (get, set, update: DataResult | null) => {
      set(lastResultAtom, update);
    }
  );

  const urqlQueryAtom = atom((get): DataResult | Promise<DataResult> => {
    if (getPaused?.(get)) {
      return { data: undefined };
    }
    const subscriptionPromise = get(memoizedSubscriptionAtom);
    const lastResult = get(lastResultAtom);
    return lastResult ?? subscriptionPromise;
  });
  return urqlQueryAtom;
}
dmaskasky commented 3 months ago

nice!

but it warns that I shouldn't be setting self synchronously

Oh yeah, you need to put that setSelf in a setTimeout or queueMicrotask. In production, I think it will error or noop (I don't remember which).

scamden commented 3 months ago

nice!

but it warns that I shouldn't be setting self synchronously

Oh yeah, you need to put that setSelf in a setTimeout or queueMicrotask. In production, I think it will error or noop (I don't remember which).

Ya the problem is that it needs to be synchronous to be correct or else the atom will return the previously cached result for the incorrect variable input

dmaskasky commented 3 months ago

May I recommend jotai-history for your use-case?

const targetWithPrevious = atomWithHistory(targetAtom, 2)

Ya the problem is that it needs to be synchronous to be correct or else the atom will return the previously cached result for the incorrect variable input

You can do synchronous set during read like this, but it does require onMount to be called at least once.

type PromiseOrValue<T> = T | Promise<T>

export function makeUrqlQueryAtom<Data, Variables extends Record<string, any>>({
  scope,
  getVariables,
  query: document,
  getPaused,
}: {
  query: DocumentInput<Data, Variables>
  getVariables: (get: Getter) => Variables
  scope: <A extends Atom<any>>(atom: A) => A
  getPaused?: (get: Getter) => boolean
}) {
  type DataResult = { data?: Data | undefined }

  const lastResultAtom = scope(atom<null | DataResult>(null))
  const variablesAtom = scope(atom(getVariables))
  const refreshAtom = atom(0)
  const refAtom = atom(
    () => ({ queue: [] }),
    (get, set) => {
      const ref = get(refAtom)
      ref.set = set
      ref.isMounted = true
      set(refreshAtom, c => c + 1)
      ref.queue.forEach((data) => set(lastResultAtom, data))
      ref.queue.length = 0
      return () => {
        ref.isMounted = false
      }
  )
  refAtom.onMount = (setSelf) => setSelf()

  const memoizedSubscriptionAtom = atom<PromiseOrValue<DataResult>>(
    (get) => {
      get(refreshAtom)
      const ref = get(refAtom)
      if (!ref.isMounted) return     // <--- synchronous set does require refAtom.onMount to fire
      const { set } = ref
      set(null)   // <-- does this have to be synchronous?
      const source = get(urqlClientAtom).query(document, get(variablesAtom))
      source.subscribe((data) => {
        const ref = get(refAtom)
        if (!ref.isMounted) {
          ref.queue.push(data)
          return
        }
        set(lastResultAtom, data)
      })
      return source.toPromise()
    }
  )

  const urqlQueryAtom = atom((get): DataResult | Promise<DataResult> => {
    if (getPaused?.(get)) {
      return { data: undefined }
    }
    const subscriptionPromise = get(memoizedSubscriptionAtom)
    const lastResult = get(lastResultAtom)
    return lastResult ?? subscriptionPromise
  })
  return urqlQueryAtom
}
dmaskasky commented 3 months ago

I am investigating making jotai-effect synchronous. I cannot guarantee it will be feasible, but I'm looking into it.

dmaskasky commented 3 months ago

Closing this issue for now as the original question has been answered, this is expected behavior for atomEffect.