jotaijs / jotai-devtools

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

fix: defer setAtom in subscribers of store change during main render to next micro task #109

Closed yf-yang closed 9 months ago

yf-yang commented 10 months ago

fix https://github.com/pmndrs/jotai/issues/2211

I am not familiar with this repo, so I just search all the devSubscribeStore.

  1. Not sure if I find all the places that need a fix.
  2. The tests are broken, I haven't taken a look yet.
codesandbox-ci[bot] commented 10 months 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 6cac845d212f03c44b47c22e3e710aa379d7439b:

Sandbox Source
React Configuration
React TypeScript Configuration
yf-yang commented 10 months ago

Need more guidance 😅

dai-shi commented 10 months ago

on second thought, it might be better to detect if it's sync, and only delay if necessary.

let isRendering = true;
useLayoutEffect(() => {
  isRendering = false;
});
const callback = () => {
  const fn = () => setAtoms(Array.from(store.dev_get_mounted_atoms?.() || []));
  if (isRendering) {
    Promise.resolve().then(fn)
  } else {
    fn()
  }
};
yf-yang commented 10 months ago
let isRendering = true;
useLayoutEffect(() => {
  isRendering = false;
});

Tricky 😆, let me refactor it.

yf-yang commented 10 months ago
const isRendering = useRef(true);
useLayoutEffect(() => {
  isRendering.current = false;
});

Should it be useRef?

yf-yang commented 10 months ago

Now one test fails: ● DevTools - AtomViewer › List of atoms › auto unmount › should unselect the atom when an atom is unsubscribed

https://github.com/jotaijs/jotai-devtools/blob/12ba74a21968029ec19cfaa8d81f75ddb4cb51aa/__tests__/devtools/AtomViewer.test.tsx#L254

dai-shi commented 10 months ago
const isRendering = useRef(true);
useLayoutEffect(() => {
  isRendering.current = false;
});

Should it be useRef?

No, that's incorrect. Using the local variable is necessary.

dai-shi commented 10 months ago
const isRendering = useRef(true);
useLayoutEffect(() => {
  isRendering.current = false;
});

Should it be useRef?

No, that's incorrect. Using the local variable is necessary.

That was my oversight. In this case, we need useRef, and violate a rule of purity.

const isRendering = useRef(true);
isRendering.current = true; // This violates the purity.
useLayoutEffect(() => {
  isRendering.current = false;
});
arjunvegda commented 10 months ago

Looks good to me as well! Thanks so much for your contribution!

Could you please help us test this on NextJs and non-NextJS applications?

You may fork these CSBs (Non-NextJS and NextJS) to save some time 🙌

yf-yang commented 10 months ago

@arjunvegda seems I cannot fork the cra sandbox

We were unable to fork the sandbox Cannot fork protected sandbox out of workspace

yf-yang commented 10 months ago

https://codesandbox.io/p/sandbox/jotai-devtools-demo-next-forked-g6fncd This is the forked nextjs version. Does it expose the browser devtool console? I didn't upgrade packages yet but I cannot see it anywhere

yf-yang commented 10 months ago

CodeSandbox is really annoying. Whenever I tried to upgrade a package, the .next cache does not change and a require of the stale chunk would fail. Delete the .next folder is useless. I don't know what to do.

arjunvegda commented 10 months ago

CodeSandbox is really annoying. Whenever I tried to upgrade a package, the .next cache does not change and a require of the stale chunk would fail. Delete the .next folder is useless. I don't know what to do.

Sorry for the delayed response. Could you try running it locally? I've been bitten by that error before too. Seems caching specific

dai-shi commented 9 months ago

@yf-yang What's the current status?

yf-yang commented 9 months ago

@dai-shi Stuck here

  1. CSB, unable to fork/unable to install the branch
  2. install github branch from a local folder, is it viable? I can't make it work
  3. install from another prereleased local folder, still struggling with multiple react issue.
dai-shi commented 9 months ago

Thanks.

@arjunvegda What do you think our options are?

yf-yang commented 9 months ago

Maybe I need some guidance, how do you release a branch as a npm installable cloud url? I remember you did that with jotai-scope, but I am not so familiar with the package release stuff.

dai-shi commented 9 months ago

You can install the codesandbox build. https://ci.codesandbox.io/status/jotaijs/jotai-devtools/pr/109 See "Local Install Instructions" ☝️

yf-yang commented 9 months ago

Cool, will get back to you tmr.

arjunvegda commented 9 months ago

Yes, testing this locally (using the Canary CSB build) would be our best option if you're running into issues with CSB.

@yf-yang: LMK if the above approach doesn't work , I can help test it locally later this week 🙌

yf-yang commented 9 months ago

OK, have got both tested with https://pkg.csb.dev/jotaijs/jotai-devtools/commit/6cac845d/jotai-devtools

yf-yang commented 9 months ago

@arjunvegda Could the patch be merged?

arjunvegda commented 9 months ago

Hey @yf-yang

Yes! I'll merge and release this tomorrow. Appreciate your patience! 🙏