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

Computed atoms get needlessly triggered again #25

Open
RaffaeleCanale opened this issue Mar 21, 2024 · 11 comments
Open

Computed atoms get needlessly triggered again #25

RaffaeleCanale opened this issue Mar 21, 2024 · 11 comments

Comments

@RaffaeleCanale
Copy link

Problem statement

Let's consider this example:

import { atom, useAtomValue } from "jotai";
import { ScopeProvider } from "jotai-scope";

const AtomA = atom(() => {
  console.log("Computing AtomA");
  return "ValueA";
});

function Child() {
  const valueA = useAtomValue(AtomA);

  return <div>{valueA}</div>;
}

export default function App() {
  return (
    <div className="App">
      <Child />
      <ScopeProvider atoms={[]}>
        <Child />
      </ScopeProvider>
    </div>
  );
}

Note that I purposely do not pass AtomA in the scope provider, so I would intuitively assume that the inner child would read from the same "scope instance" as the parent one. As the read function normally only triggers when dependencies change, I would except it not to trigger again but just return the same cached value as for the parent component.

However that turns out not to be the case here. "Computing AtomA" gets logged twice in the console almost as if the atom was scoped.

Additional context

Perhaps this was implemented with the idea that read functions are pure, so even if we compute them again they'll return the same result. And if they depend on other atoms, the value for those would match the parent scope, so it would again return the same value as the parent scope.

However I find that hinders the assumption that read functions only trigger when dependencies change. For example I have use cases with async read functions that fetch data from an API into a cached atom. So if I were to consume those atoms inside unrelated ScopeProviders, then they'd trigger the API fetch again which is an undesired and unpredictable behavior.

@yf-yang
Copy link
Collaborator

yf-yang commented Mar 22, 2024

Thank you for submitting the issue, I know what happens but I am not sure what is the correct recipe. Let me first explain some design considerations, they are mainly about derived atoms:

  1. Constraint: all the dependency of jotai atoms are dynamically computed.
    An atom has a read function and a write function. Inside the read function, it can access other atoms via get(anotherAtom) call, then such atoms are derived atoms. Otherwise, it becomes a primitive atom.
    Note that get(anotherAtom) is an expression within the read function, which means we have no idea which atom it depends on until we call anAtom.read(). That's what we mean by dynamically computed.
  2. Design: if an atom is scoped, then all of its dependents are also scoped.
    We choose to do so, because atom dependency chains could be complicated, and it makes no sense if an atom is scoped while its dependents are not. Therefore, we choose to implement it as if an atom depends on an scoped atom, then it is also scoped even the atom is not explicitly specified in the ScopeProvider

OK, here comes the issue: In a ScopeProvider's nested children component, when calling useAtom(anAtom), we are not aware whether the atom is scoped or not. We need to first find its dependency chain (by calling anAtom.read), then check if any of the dependency atom is scoped.

So, to make it work, we PESSIMISTICALLY creates a copy of all the atoms called within a ScopeProvider (including atoms depended by them), then calls their read function to detect their dependencies. If they are actually not scoped, what we do is like, making the copy subscribe the original atom, so they look like the same. However, internally, all the atoms within a ScopeProvider are copied. They are different instances.

Comming back to your original case, I have two ideas. The first one would be add another API to explicitly tells ScopeProvider that I don't want the atom to be scoped, but I am not in favor of it because that would lead to additional stuff to learn and I am not sure if that's a good implementation. The second one would be decoupling the cache with your atom. You can implement the "cache fetch" code outside of read function and determines if the cache is available in your read function. Would you like to share a piece of code of your async cache? I'm interested in if it is only initialized once and never changes then, or it could be invalid and retrieved at some time. Anyway, I think the recipe is not so difficult.

I am not so clear if some deeper assumptions are behind. Async functions are not pure. "read functions only trigger when dependencies change" is true, but unfortunately a scoped atom is not the original atom, so multiple initializations occur for each ScopeProvider.

@RaffaeleCanale
Copy link
Author

I see there are some implications that are more complex than what I thought. For example, what if a derived atom is not scoped, but one of its dependent atoms is, it leads to confusing situations indeed.

Would you like to share a piece of code of your async cache?

Sorry I might have mislead you with my language. What I meant by "cache" was really just the atom itself and it how it buffers the result of a derived atom. For example, it could look like this:

import { Suspense } from "react";
import { atom, useAtomValue } from "jotai";
import { ScopeProvider } from "jotai-scope";

const MyObjAtom = atom(async () => {
  // Expecting this to only be called once (unless someone explicitly scopes it)
  const response = await fetch('/api/myobj');
  return response.json();
});

function MyObjView() {
  const myObj = useAtomValue(MyObjAtom);

  return <div>{myObj}</div>;
}

export default function App() {
  return (
    <div className="App">
      <Suspense>
        <MyObjView />
        <ScopeProvider atoms={[]}>
          <Suspense>
            <MyObjView />
          </Suspense>
        </ScopeProvider>
      </Suspense>
    </div>
  );
}

The problem we have is that we constructed most our API requests using plain async atoms like these, which works well enough for our current stage, but whenever someone uses a ScopeProvider, even if it's for a set of unrelated atoms, if they read from any of those API call atoms, they will re-fetch again.

One workaround I found is manually cache those atoms, a bit like so:

const MyObjCacheAtom = atom();
const MyObjAtom = atom(async (get) => {
  if (get(MyObjCacheAtom)) {
    return get(MyObjCacheAtom);
  }
  const response = await fetch('/api/myobj');
  return response.json();
});

This actually works well as MyObjCacheAtom will not be scoped. I guess we could also use jotai-cache for the same effect. My issue with that solution is that we would need to change all our API calls and take the habit of caching them all, not because we want/need the cache, but because some components might potentially use an unrelated ScopeProvider.

I was prioritizing simplicity with this approach for as long as we don't need more advanced features (e.g. invalidation), but it seems that this simplicity won't work with jotai-scope and I either need to drop it, or bring an additional general layer of complexity to our API interactions (e.g. jotai-cache, jotai-tanstack-query, custom caching, ...) which I was hoping to avoid.

@yf-yang
Copy link
Collaborator

yf-yang commented Mar 28, 2024

Yes yes your workaround is exactly correct, I can understand what a more intuitive API would like, but unfortunately I don't know how to do that (maybe that's a wont-fix) :/

The key takeaway is One should not expect the read function being called exactly once, even though the atom is not scoped.

I'll consider documenting it instead of patching it if that is a rare case.

@yf-yang
Copy link
Collaborator

yf-yang commented Apr 30, 2024

Updated readme, close it for now.

@yf-yang yf-yang closed this as completed Apr 30, 2024
@scamden
Copy link

scamden commented May 21, 2024

apologies but i don't think i understand the work around. Are you writing to this cache atom somewhere?

const MyObjCacheAtom = atom();
const MyObjAtom = atom(async (get) => {
  if (get(MyObjCacheAtom)) {
    return get(MyObjCacheAtom);
  }
  const response = await fetch('/api/myobj');
  return response.json();
});

@scamden
Copy link

scamden commented May 22, 2024

@yf-yang @RaffaeleCanale curious if you have more detail on how you're using the cache.

For context, we have two separate bugs when using jotai-scope with atomWithObservable (at first via jotai-urql but it also manifests when used directly).

One involves a combination of unwrap and the observable atom creating a loop. The other manifests as a permanently hung suspense boundary.

I'm not sure how to use a caching solution with the observable since I specifically need it to recalculate on updates from the URQL cache.

Part of my difficulty here is that this also only manifests in our production nextjs build. I have tried hard to create a repro here: https://codesandbox.io/p/sandbox/urql-plus-suspense-plus-scope-mzkct6?file=%2Fsrc%2FApp.tsx and haven't been able to create a consistent one. I have seen an infinite loop intermittently but not often. Whereas, the prod bugs I mentioned happen reliably every time.

Any thoughts here would be very appreciated.

@RaffaeleCanale
Copy link
Author

@scamden Is the sandbox public? Following that link tells me the sandbox wasn't found.

In the workaround I suggested, the atom MyObjCacheAtom is the "cache". My issue was that when using jotai-scope, all derived atoms (even the ones not listed in the scope) will be sort of invalidated and recompute. In other words, the derivation function (in my case, the one making an API call) would get called again if that atom was accessed within the scoped tree.

So by copying the response of the API in a separate atom (which I also don't scope), then even if my derived atom gets computed again, it's fine as it will just return the value from the MyObjCacheAtom.

So in the end, my problem was really just about derived atoms getting recomputed again despite not being in the scope list, which caused unnecessary API calls.

Full disclosure: In the end it was too cumbersome and error prone to cache all our API derived atoms into simple value holders atoms, so we went an alternative route and stopped using jotai-scope altogether unfortunately.

@dmaskasky
Copy link
Member

Is this still an issue? I just merged a full ground-up rewite of jotai-scope. We will create a new release version shortly.

In the new implementation derived atoms still get copied (because they may access scoped atoms), but the way they get copied doesn't call their read or init functions. This also fixes atomWithLazy from jotai/utils.

@RaffaeleCanale
Copy link
Author

Is this still an issue? I just merged a full ground-up rewite of jotai-scope. We will create a new release version shortly.

In the new implementation derived atoms still get copied (because they may access scoped atoms), but the way they get copied doesn't call their read or init functions. This also fixes atomWithLazy from jotai/utils.

This sounds exciting! I'm happy to give it a drive when the new release is pushed. It might take me a few days to find the time to test but I'm happy to report back afterwards 👍

@scamden
Copy link

scamden commented May 23, 2024

@scamden Is the sandbox public? Following that link tells me the sandbox wasn't found.

Whoops it wasn't! Now it is.

Full disclosure: In the end it was too cumbersome and error prone to cache all our API derived atoms into simple value holders atoms, so we went an alternative route and stopped using jotai-scope altogether unfortunately.

If you stopped, what alternative did you choose? I experimented briefly with bunshi personally

@RaffaeleCanale
Copy link
Author

If you stopped, what alternative did you choose? I experimented briefly with bunshi personally

None. For the atoms we intended to scope, we kept them global and manually reset them when relevant. We wanted to scope them to avoid unnecessarily holding onto global memory longer than needed, so for now we just accepted to keep them global nevertheless.

I'm sorry I can't be of more help, but I have hopes that this latest feature could truly help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants