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

Issue with state change timing? (preact + react-query) #570

Closed
damianstasik opened this issue Jun 28, 2021 · 11 comments
Closed

Issue with state change timing? (preact + react-query) #570

damianstasik opened this issue Jun 28, 2021 · 11 comments
Labels
help wanted Please someone help on this on hold Nothing is actionable now, but hope to revisit in the future

Comments

@damianstasik
Copy link

I have a pretty interesting issue that I stumbled upon when trying to swap recoil with jotai. I am still not sure if I am posting this to the right repo as this bug might be in something else. It happens for me when using preact, react-query, and jotai, but works fine with recoil.

I prepared a simple reproduction of this bug, but first here is how it should behave:
https://codesandbox.io/s/react-react-query-jotai-fwhj3 (react + react-query + jotai)

You should be able to go through three steps and then back to the beginning. There is an async function executed before going from one step to another that in the reproduction is just a Promise that resolves after 1 second.

Here is a similar app that uses preact instead of react and recoil instead of jotai:
https://codesandbox.io/s/preact-react-query-recoil-sd8pu

And here is the reproduction of the bug which happens only with preact, react-query, and jotai:
https://codesandbox.io/s/preact-react-query-jotai-5nsfu

As you can hopefully see, after the async function resolves and the second step is shown, the button in the second step is disabled as the useIsMutating function still thinks that there is a mutation running.

It looks like the second step's logic is run after the setPage triggers the change of the currently displayed step, and after that is done the component is displayed with the old data.

This might be an issue in preact, but why does it work with recoil and does not with jotai?

@Aslemammad
Copy link
Member

Hello @visualfanatic!
As far as I know, preact underlying API's works a little bit different than react. So I assume it's about supporting preact more than fixing a bug. I think it's about differences between react & preact's way of handling async stuff.
So @dai-shi and I are working on this; I hope we resolve it soon.

@dai-shi
Copy link
Member

dai-shi commented Jun 30, 2021

@Aslemammad My question is if the issue is only about preact + async stuff.
If there's an issue with preact + sync, we want to fix it.
But, if it's only about async, even if we fix it now, preact may change the behavior in the future. https://preactjs.com/guide/v10/switching-to-preact/#suspense-experimental So, it might not worth fixing.

@Aslemammad
Copy link
Member

Aslemammad commented Jun 30, 2021

I fixed it by scheduling the set function in the macro task(task) phase. codesandbox

export const pageState = atom(1, (get, set, update) => {
  setTimeout(() => set(pageState, update(get(pageState))));
});

What's your opinion now?

Edit: It didn't work when I scheduled it into the microtask phase (Promise.resolve); maybe there's a problem in Preact scheduling?

@dai-shi
Copy link
Member

dai-shi commented Jun 30, 2021

Wow, you rock. Yeah, probably preact uses microtask for batching.
I'm not sure if there's anything we can do on jotai end. recoil may change the timing in the future.
For now, the setTimeout workaround seems good.
I would put setTimeout in onSuccess.
https://codesandbox.io/s/preact-react-query-jotai-forked-8g8j3


only about preact + async stuff

This issue is about preact + async stuff, but not preact + async atoms.
One guess of mine is our useMutableSource emulation uses some techniques, and it isn't compatible with preact. If we could reveal an issue in preact + uMS emulation, there would be a chance to fix. It may not worth the effort as it will migrate to the real uMS sooner or later.

@dai-shi dai-shi changed the title Issue with state change timing? Issue with state change timing? (preact + react-query) Jun 30, 2021
@dai-shi
Copy link
Member

dai-shi commented Jul 29, 2021

Please try again with recoil 0.4 when it's out.

@dai-shi
Copy link
Member

dai-shi commented Aug 10, 2021

https://codesandbox.io/s/preact-react-query-recoil-forked-88klx works fine with recoil 0.4

@dai-shi
Copy link
Member

dai-shi commented Aug 10, 2021

Hmm, thought turning off batching helps, but not.
https://codesandbox.io/s/preact-react-query-jotai-forked-dq0k5

@Aslemammad Do you think we can reproduce the issue without react-query?

@dai-shi dai-shi added the on hold Nothing is actionable now, but hope to revisit in the future label Nov 9, 2021
@dai-shi
Copy link
Member

dai-shi commented Nov 30, 2021

@Aslemammad Althought this is marked on hold, feel free to continue the investigation. Especially, #854 might change some behaviors.

btw, recoil is v0.5 now.

@dai-shi
Copy link
Member

dai-shi commented Dec 30, 2021

v1.5.0 doesn't change the behavior. This is probably because jotai uses the subtle behavior of useReducer. Would anyone like to dig this issue deeper?

@dai-shi dai-shi removed the on hold Nothing is actionable now, but hope to revisit in the future label Dec 30, 2021
@dai-shi
Copy link
Member

dai-shi commented Mar 31, 2022

Let's keep this open for some more time, but it's likely we close this eventually unless someone works on it.

@dai-shi dai-shi added on hold Nothing is actionable now, but hope to revisit in the future help wanted Please someone help on this labels Apr 26, 2022
@dai-shi
Copy link
Member

dai-shi commented Jun 7, 2022

With latest versions, it behaves still the same: https://codesandbox.io/s/preact-react-query-jotai-forked-u36uoe

However, recoil one also behaves the same with latest versions: https://codesandbox.io/s/preact-react-query-recoil-forked-klowl7

So, I'd assume the issue is not on our end, but on preact(-compat). Closing.

@dai-shi dai-shi closed this as completed Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Please someone help on this on hold Nothing is actionable now, but hope to revisit in the future
Projects
None yet
Development

No branches or pull requests

3 participants