Problems with React Fast Refresh

[StarpTech]

Hi, while it works well in most cases, we experience some bugs.

1. Editing functions out of react tree aren't updated. For example, update the logic of your read-only atom and issue a state update.
2. The referential integrity of the atom store gets broken. I receive no longer state updates. I need to reload to fix it.

All issues happen when using react fast refresh with https://github.com/gsoft-inc/craco. We use the jotai babel preset as documented on the website!

Thank you and keep up the good work!

[dai-shi]

Hi, can you reproduce some of concrete examples in codesandbox?
(Then, @Thisen can investigate it?)

[Thisen]

Hi, can you reproduce some of concrete examples in codesandbox? (Then, @Thisen can investigate it?)

Yes, I'll look into it, when there's a reproduction 👍🏼

[StarpTech]

Yes, I'll try.

[StarpTech]

@Thisen Hi, this seems related. Shouldn't the state be preserved when updating the read function?

• Example 1- https://codesandbox.io/s/nextjs-with-jotai-async-forked-dk9316?file=/store.js (line 30)
• Example 2 - https://codesandbox.io/s/nextjs-with-jotai-async-forked-dk9316?file=/pages/index.js (line 25)

[StarpTech]

@Thisen friendly ping

[Thisen]

After I looked at it, I'd expect your scenarios to break, given the somewhat simple implementation of our React refresh integration.

The question is if we are willing to solve it. If so, we need to either re-implement the solution or find some smart way to clean up the Map, when the atom function changes.

[StarpTech]

Hi @Thisen, good to hear that you found it. Since atoms are functions with business logic, it would be very bad DX if React refresh is not fully supported. For my use case, it's already a mess because I need to reload my page for every change in an atom.

[dai-shi]

@Thisen Would it be possible to use Function.toString() and see if the code is changed?

@StarpTech This is a very difficult problem, but I understand it'd be nice to fix. If you have suggestions/ideas, you are very welcome. In essence, we need to learn how React deals with it and follow what it does.
Meanwhile, to help DX with Fast Refresh, I'd suggest to define each atom in a file. store/postIdAtom.js, store/postId.js and so on, and re-export from store.js.

[StarpTech]

but I understand it'd be nice to fix

I'll try to look into it as well. It's definitely not just nice :) It doesn't fall back to hot module reload. The application will no longer work because the components aren't rerendered when a dependent atom is changed.

[StarpTech]

@dai-shi how do you handle that in your workflow?

[Thisen]

Hi @StarpTech, could you try out the build in #1030?

[StarpTech]

@Thisen Unfortunately, it doesn't help. After changing a string in an atom function used in a parent and children relationship, the parent no longer receives updates.

[dai-shi]

@dai-shi how do you handle that in your workflow?

👇

Meanwhile, to help DX with Fast Refresh, I'd suggest to define each atom in a file. store/postIdAtom.js, store/postId.js and so on, and re-export from store.js.

I don't know if it works your case, though.

[StarpTech]

@dai-shi I tried but without success.

[dai-shi]

I tried but without success.

Hmm, I need to look into it. Would you be able to share a codesandbox repro?

[StarpTech]

@dai-shi I shared two. Are they sufficient to start the work? I can try to provide another one.

[dai-shi]

Yeah, if you have split store/*.js example, it's helpful.

[dai-shi]

@StarpTech Here you go: https://codesandbox.io/s/quizzical-andras-q3tmze?file=/src/store.js

[Thisen]

Is there more we can do here @dai-shi?

[dai-shi]

@Thisen I'm still curious why this doesn't work. Would you investigate it please?

const postIdAtom = atom(INITIAL_POST_ID);

export const postId = atom(
  (get) => {
    // update the number, will reset "postIdAtom"
    console.log(3);
    return get(postIdAtom);
  },
  (get, set, update) => set(postIdAtom, update(get(postIdAtom)))
);

Even without #1030, I expect postIdAtom should be cached, no?

[Thisen]

@Thisen I'm still curious why this doesn't work. Would you investigate it please?

const postIdAtom = atom(INITIAL_POST_ID);

export const postId = atom(
  (get) => {
    // update the number, will reset "postIdAtom"
    console.log(3);
    return get(postIdAtom);
  },
  (get, set, update) => set(postIdAtom, update(get(postIdAtom)))
);

Even without #1030, I expect postIdAtom should be cached, no?

It should 🤔 I'll look into it!

[Thisen]

@dai-shi, I couldn't reproduce it - It should work with jotai/babel/plugin-react-refresh. You got a reproduction with the plugin?

[dai-shi]

@Thisen #1025 (comment) should be the reproduction, as far as I recall.

[Thisen]

@dai-shi #1025 (comment) I don't think we can support this scenario tbh. Then we need to redo the caching mechanism for the Babel plugin.

[dai-shi]

Hm, there must be some misunderstanding of mine.
I thought the simplified scenario was:

const postIdAtom = atom(INITIAL_POST_ID);

export const postId = atom(
  (get) => {
    // update the number, will reset "postIdAtom"
    console.log(3);
    return get(postIdAtom);
  },
  (get, set, update) => set(postIdAtom, update(get(postIdAtom)))
);

What am I missing? Is it nextjs specific?

[Thisen]

I still don't have a reproduction with our babel plugin.

[Thisen]

@StarpTech do you have a reproduction?
I can't see the Babel plugin being configured in these examples: #1025 (comment)

If not, I will close the issue.

[TwistedMinda]

@dai-shi @alan-nf Hey, I've been reading through quickly. This issue is a bit concerning right? I'll try to check if I can find a workaround, but don't count on it :p

I feel like it would be important to mention the status of this in the docs.
We could create a new menu item "Fast Refresh" to address:

• current workaround: as long as you don't modify a file that contains the atom declaration, it will not reset the state
• our feeling: DX not the best, forcing to split atoms to avoid state reset, but we are aware/working on it
• potential edge-cases where state is reset even if no atom declaration was in the edited file
• we're very much open to support/contribution on this

I'm interested in knowing if that can become an issue in the long run. Pretty sure some others are too.
Feels like it matters to be transparent on the caveats as much as possible.

What you guys think?

[alan-nf]

@TwistedMinda I'm actually still completely stumped by #1150 so I'm in favor of calling this out as a DX issue. The other big bullet item I would add is:

• even when you split the atom out to a separate file it might get included in hot-update bundles and diagnosing WHY (e.g. in Vite) might be very difficult

[TwistedMinda]

@alan-nf Yeah, alright, our best Fast Refresh wizards shall come once we raise the help flag.
Gonna make a PR

it might get included in hot-update bundles and diagnosing WHY (e.g. in Vite) might be very difficult

Means that determining the reason why it was included in "hot-update bundles" might be difficult? (eg. in Vite)
I got you right?

[dai-shi]

Sorry for ambiguous issue status.
We were waiting for a repro. #1025 (comment)

Let's close this as stale.

#1203 seems a good suggestion. Let's continue.