search

jotaijs / jotai-devtools

A powerful toolkit to enhance your development experience with Jotai
https://jotai.org/docs/tools/devtools
MIT License
134 stars 33 forks source link

feat(utils): cache redux extension connections to prevent duplicates #32

Open PrettyCoffee opened 1 year ago

PrettyCoffee commented 1 year ago

Originated from this discussion: https://github.com/pmndrs/jotai/discussions/1795#discussioncomment-5111855

Notes:

codesandbox-ci[bot] commented 1 year ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit eef4381203f9b1d94c744e5fa957cd9e5df39961:

Sandbox Source
React Configuration
React Typescript Configuration
arjunvegda commented 1 year ago

Hey, @PrettyCoffee thanks a lot for your contribution!

Sorry, I missed the discussion you filed on the Jotai repo. If I understand correctly, this "bug" is caused by React 18's new approach to enforce idempotency in Strict mode. It runs hooks like useEffect and useMemo twice in development mode. See: https://beta.reactjs.org/reference/react/useEffect#my-effect-runs-twice-when-the-component-mounts

So making the useAtomDevtools hook idempotent should address the bug (i.e. each connection must also disconnect).

Please see this fix: #33 Here is the fork of your CodeSandbox with the fix: https://codesandbox.io/p/sandbox/jotai-redux-devtools-forked-2dfvcv

⚠️ Note

I think the bigger issue you caught is that the atom values are not being updated

Did you get a chance to investigate why that could be happening?

PrettyCoffee commented 1 year ago

Is it only an issue with StrictMode? And, do yo know how react-redux solves it? This doesn't feel right, because we never received such issues in zustand, which does something similar. (Wait, zustand doesn't use hooks, so it's jotai specific. Hmmm 🤔 )

Yess, thats actually the problem, due to the render cycles it could be executed multiple times which should not be happening. I guess zustand, as well as react-redux, are only using it as a middleware when initializing the store, therefore it will only be executed once in the apps live-time.

My gut feeling says it could be more beneficial to do the exact same with atoms: Having it as a middleware or something like that instead of a hook. But I am not really deep into jotai's mechanics, so there might be reasons why you wouldn't want to do that.

PrettyCoffee commented 1 year ago

Hey, @PrettyCoffee thanks a lot for your contribution!

Thank you for jotai and having devtools, really feels like a blessing! :)

So making the useAtomDevtools hook idempotent should address the bug (i.e. each connection must also disconnect).

There lies the problem: You cannot do that with redux devtools (at least I didn't find a proper way for that)

Please see this fix: #33 Here is the fork of your CodeSandbox with the fix: codesandbox.io/p/sandbox/jotai-redux-devtools-forked-2dfvcv

I will have a closer look on that later. But I think that this will actually introduce bugs 😕

The __REDUX_DEVTOOLS_EXTENSION__.disconnect() function actually removes all existing connections (devtools in the current code). This works for useAtomsDevtools if you are only using this one and not more of the useAtomDevtools or useAtomsDevtools hooks! And it does not work for the useAtomDevtools hook since that would disconnect all other useAtomDevtools instances as well, while you probably only want to disconnect one.

According documentation: https://github.com/reduxjs/redux-devtools/blob/main/extension/docs/API/Methods.md#disconnect

I did not find a function to actually disconnect one connection from the extension. It is all or none.

I think the bigger issue you caught is that the atom values are not being updated Did you get a chance to investigate why that could be happening?

Unfortunatly I didn't find the problem there. Could be that it lies within the other cleanup issues I addressed, but I am not entirely sure. If I got the time I can have a closer look at this again. 👍

arjunvegda commented 1 year ago

I will have a closer look on that later. But I think that this will actually introduce bugs 😕

I'm unfamiliar with the internals of Redux DevTools Extensions, so could you help us create a small repro of the issues you're referring to?

I added useAtomsDevtools("foo-store"); on top of useAtomDevtools(atom) but was unable to reproduce the issue.

Unfortunatly I didn't find the problem there. Could be that it lies within the other cleanup issues I addressed, but I am not entirely sure. If I got the time I can have a closer look at this again. 👍

Found the issue, this is happening because <DebugAtoms/> is located outside of the Jotai Provider. Moving it within the <Provider/> fixes it. See updated CSB - https://codesandbox.io/p/sandbox/jotai-redux-devtools-forked-2dfvcv

PrettyCoffee commented 1 year ago

I'm unfamiliar with the internals of Redux DevTools Extensions, so could you help us create a small repro of the issues you're referring to?

I will add a small documentation to the redux tool files and submit them in a separate PR, then we can discuss that detail there 👍

I added useAtomsDevtools("foo-store"); on top of useAtomDevtools(atom) but was unable to reproduce the issue.

What I meant there is, that when you

  1. render the useAtomsDevtools
  2. render the useAtomDevtools
  3. unrender the useAtomsDevtools
  4. extension.disconnect() is executed on cleanup

you will loose all connections that are existing, even tho theoretically some should be there.

Tweaked example

Found the issue, this is happening because <DebugAtoms/> is located outside of the Jotai Provider. Moving it within the <Provider/> fixes it. See updated CSB - https://codesandbox.io/p/sandbox/jotai-redux-devtools-forked-2dfvcv

Aaaah, yes, that makes sense! Thank you for fixing :)

PrettyCoffee commented 1 year ago

Added an issue in redux-devtools to see if they got a solution for the problem: https://github.com/reduxjs/redux-devtools/issues/1370