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

Imperatively refresh atomFamily #71

Closed
fkocovski opened this issue Sep 28, 2020 · 18 comments · Fixed by #72
Closed

Imperatively refresh atomFamily #71

fkocovski opened this issue Sep 28, 2020 · 18 comments · Fixed by #72
Labels
has snippet This issue includes code snipppets as solutions or workarounds

Comments

@fkocovski
Copy link

What would be the best way to imperatively refresh an atomFamily?

Let's say I have following case, where I use get(updateAtomFamily) in order to track it as dependency in the atomFamily so that I trigger the fetch imperatively:

export const fetchAtomFamily = atomFamily(
  (param) => async (get) => {
    get(updateAtomFamily);
    const { data } = await getData({
      param1: param.param1,
      param2: param.param2,
    });
  },
  null,
  deepEqual
);

export const updateAtomFamily = atom(0);

Then my idea was to imperatively useUpdateAtom in my component as a refresh function like this:

  const refresh= useUpdateAtom(updateAtomFamily);

and then have the imperative call:

<button onClick={() => refresh(prev => prev + 1)}>refresh</button> 

However, when I trigger the refresh function, I get following error:

Uncaught Error: atom state not found. possibly a bug.

In my opinion however, even if this approach worked, it feels sluggish and cumbersome to me. It would be extremely helpful if the API had a native approach to re-fetch async atoms :)

@dai-shi
Copy link
Member

dai-shi commented Sep 28, 2020

@fkocovski Hi, nice to see you trying atomFamily

atom state not found. possibly a bug.

Would you create a small repro in codesandbox?

It would be extremely helpful if the API had a native approach to re-fetch async atoms :)

Please consider using write (the 2nd arg of atom()) instead of read.
You might need two atomFamilies.

Maybe related: #68

@fkocovski
Copy link
Author

Here is a small codesandbox in which I managed to reproduce the error. Please note that the async atomFamily has both a primitive atom as well as a derived atom as dependency. Hope this helps.

https://codesandbox.io/s/white-shadow-0zqo3

@dai-shi
Copy link
Member

dai-shi commented Sep 28, 2020

Thanks for the repro. It seems to reproduce the error with primitive1 instead of derived.
I will investigate it.

Meanwhile, if you have a question about using write for your use case, let me know.

@fkocovski
Copy link
Author

Thanks for the prompt feedback @dai-shi , for the moment I reverted as per your suggestions to the write function with useEffect and a normal fetch inside of it. Sadly I lost the leverage of Suspense on component re-render, but it works!

The library is truly a game-changer, I love the streamlined APIs, I've started integrating it in one of our projects ;) Looking forward to all the new features in the pipeline, keep up the amazing work.

@dai-shi
Copy link
Member

dai-shi commented Sep 28, 2020

Sadly I lost the leverage of Suspense on component re-render, but it works!

Yeah, suspending on write is very tricky. For now, the atom you invoke write should suspend. Technically, it can't know its dependents before the first write.
If you have time, could you create a small codesandbox to illustrate your use case? I'd like to learn the use cases for write suspense.

@fkocovski
Copy link
Author

https://codesandbox.io/s/elastic-cloud-zj3fe

The idea is to have a normal atom which gets data asynchronously on component render. This part already works perfectly by creating the atom like this:

const fetchData = atom(async (get) => {
  const data = await fetch("https://jsonplaceholder.typicode.com/posts");
  return await data.json();
});

and then having a normal component with useAtom:

export default function App() {
  const [data] = useAtom(fetchData);

  const newPost = async () => {
    await fetch("https://jsonplaceholder.typicode.com/posts", {
      method: "POST",
      body: JSON.stringify({
        title: "foo",
        body: "bar",
        userId: 1
      }),
      headers: {
        "Content-type": "application/json; charset=UTF-8"
      }
    });
  };

  return (
    <>
      <div>
        <button onClick={newPost}>new</button>
      </div>
      <div>
        <ol>
          {data.map((x) => (
            <li key={x.id}>{x.title}</li>
          ))}
        </ol>
      </div>
    </>
  );
}

What I still struggle is with the newPost use-case: how would I imperatively tell the atom to re-fetch the data after I created a new post without having to write the same logic I have in the get part?

@dai-shi
Copy link
Member

dai-shi commented Sep 29, 2020

read should basically be pure (in React terminology), and we can't imperatively fetch data with read. So, write should do the job.

If we could make the initial data empty, and fill it with useEffect on mount, it should be easy.
The question here is how to fill the initial data from the server.

I have one idea, which may not seem perfect, but it works.
https://codesandbox.io/s/headless-hill-zdhi6

(I really enjoy this Jotai puzzle!)

@dai-shi dai-shi reopened this Sep 29, 2020
@fkocovski
Copy link
Author

I like your approach. For the moment I decided to stick to the empty atom variant and fill it with useEffect. I also tried the approach of having a primitive refresh atom with an initial value of 0 and use that as a dependency in the async get part and increment that one in order to trigger a re-fetch. That works now flawlessly in 0.7.3!

@jcoussard
Copy link
Contributor

I really like this library! While I'm still trying the figure out its quirks, I figured solving a puzzle is a good way to learn.

Here is my attempt at it: https://codesandbox.io/s/cool-firefly-xhyc2

Is it a good way to solve that problem? I'm wondering whether const postsAtom = atom(fetchPosts()); is a good practice or not since it initialize the atom with a promise.

(PS it seems codesandbox's refresh and Jotai don't play along well, you might get the atom state not found. possibly a bug. error after editing the code, but it'll work fine after reloading the in-page browser... still I'm curious what might cause it.)

@dai-shi
Copy link
Member

dai-shi commented Sep 30, 2020

@jcoussard

const postsAtom = atom(fetchPosts());
const latestPostsAtom = atom(
  (get) => get(postsAtom),
  async (_get, set) => set(postsAtom, await fetchPosts())
);

Mind blown! 🤯
Yeah, it works. (The old versions of jotai prohibited passing promises to primitive atoms in types. Newer versions removed the restriction.)
The way it works is even though postsAtom has a promise at first, we read it though latestPostsAtom, which resolves the promise. Clever.
Only the concern is it's difficult follow for beginners and TS typing would be a bit tricky.

it seems codesandbox's refresh and Jotai don't play along well

Half expected. Atom references are important, so when it changes with React Fast Refresh, it will work somewhat unexpectedly. By splitting files, it may be improved.
The other issue I found is, even if I modified the jsx in the App component, it wouldn't update. This is unexpected and would be nice to be fixed.

@dai-shi
Copy link
Member

dai-shi commented Sep 30, 2020

even though postsAtom has a promise at first, we read it though latestPostsAtom, which resolves the promise.

I was wrong. The promise in the primitive atom is resolved before render just like the derived atom. So, it works without the pitfall.

https://codesandbox.io/s/late-cache-ujkp0?file=/src/App.js

@jcoussard
Copy link
Contributor

jcoussard commented Sep 30, 2020

I was wrong. The promise in the primitive atom is resolved before render just like the derived atom. So, it works without the pitfall.

I don't think it's resolved before render, it's the <Suspense> container doing its job displaying the fallback while waiting for the atom's promise to be fulfilled.

@dai-shi
Copy link
Member

dai-shi commented Sep 30, 2020

Yep. That's correct. I mean, it's the library that let <Suspense> do the job.
My point was that the codesandbox example in my previous comment just works (which I misunderstand before.)

@dai-shi dai-shi added the has snippet This issue includes code snipppets as solutions or workarounds label Oct 4, 2020
@dai-shi
Copy link
Member

dai-shi commented Oct 4, 2020

I'm not sure what to do with this issue. It seems too technical and edge use cases to write it in the current docs. Suggestions?

@pyrossh
Copy link

pyrossh commented Oct 9, 2020

@fkocovski why don't you just do something like this,

export const refreshAtom = atom(false);
const fetchData = atom(async (get) => {
  get(refreshAtom);
  const data = await fetch("https://jsonplaceholder.typicode.com/posts");
  return await data.json();
});

export default function App() {
  const [refresh, setRefresh] = useAtom(refreshAtom);
  const [data] = useAtom(fetchData);

  const newPost = async () => {
    await fetch("https://jsonplaceholder.typicode.com/posts", {
      method: "POST",
      body: JSON.stringify({
        title: "foo",
        body: "bar",
        userId: 1
      }),
      headers: {
        "Content-type": "application/json; charset=UTF-8"
      }
    });
    setRefresh(!refresh);
  };

  return (
    <>
      <div>
        <button onClick={newPost}>new</button>
      </div>
      <div>
        <ol>
          {data.map((x) => (
            <li key={x.id}>{x.title}</li>
          ))}
        </ol>
      </div>
    </>
  );
}

@fkocovski
Copy link
Author

Hey @pyros2097 thanks for the input. That is however exactly what I wanted to avoid: creating a dummy atom to refresh the async call. Meanwhile using the set part works flawlessly for me and I am using that approach to refresh async atoms.

@dai-shi
Copy link
Member

dai-shi commented Nov 1, 2020

current status:

This issue is still open for suggestions. I think we could either add notes in docs or create a new util. Any volunteers?

@dai-shi dai-shi added the help wanted Please someone help on this label Nov 1, 2020
@dai-shi
Copy link
Member

dai-shi commented Feb 15, 2021

While working on #304, I found this is not super nice. It currently works with undocumented behavior, but TS typing does not support for this use case.

We are getting used to handling async (#248), so there would be a better way.

Closing this as not recommended for now. Please open a new issue with a use case to discuss further.

@dai-shi dai-shi closed this as completed Feb 15, 2021
@dai-shi dai-shi removed the help wanted Please someone help on this label Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has snippet This issue includes code snipppets as solutions or workarounds
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants