fix(core): recompute dependents in edge cases

[dai-shi]

Fix #2738

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
jotai	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 15, 2024 9:07am

[codesandbox-ci]

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.

[github-actions]

Size Change: -72 B (-0.08%)

Total Size: 88.9 kB

Filename	Size	Change
./dist/esm/vanilla.mjs	3.68 kB	-11 B (-0.3%)
./dist/system/vanilla.development.js	3.77 kB	-10 B (-0.26%)
./dist/system/vanilla.production.js	1.97 kB	-7 B (-0.35%)
./dist/umd/vanilla.development.js	4.82 kB	-16 B (-0.33%)
./dist/umd/vanilla.production.js	2.67 kB	-9 B (-0.34%)
./dist/vanilla.js	4.72 kB	-19 B (-0.4%)

ℹ️ View Unchanged

Filename	Size
./dist/babel/plugin-debug-label.js	932 B
./dist/babel/plugin-react-refresh.js	1.14 kB
./dist/babel/preset.js	1.41 kB
./dist/esm/babel/plugin-debug-label.mjs	1 kB
./dist/esm/babel/plugin-react-refresh.mjs	1.19 kB
./dist/esm/babel/preset.mjs	1.49 kB
./dist/esm/index.mjs	62 B
./dist/esm/react.mjs	1.4 kB
./dist/esm/react/utils.mjs	746 B
./dist/esm/utils.mjs	67 B
./dist/esm/vanilla/utils.mjs	5.03 kB
./dist/index.js	242 B
./dist/react.js	1.44 kB
./dist/react/utils.js	1.39 kB
./dist/system/babel/plugin-debug-label.development.js	1.1 kB
./dist/system/babel/plugin-debug-label.production.js	775 B
./dist/system/babel/plugin-react-refresh.development.js	1.29 kB
./dist/system/babel/plugin-react-refresh.production.js	928 B
./dist/system/babel/preset.development.js	1.59 kB
./dist/system/babel/preset.production.js	1.14 kB
./dist/system/index.development.js	252 B
./dist/system/index.production.js	183 B
./dist/system/react.development.js	1.56 kB
./dist/system/react.production.js	863 B
./dist/system/react/utils.development.js	860 B
./dist/system/react/utils.production.js	462 B
./dist/system/utils.development.js	257 B
./dist/system/utils.production.js	187 B
./dist/system/vanilla/utils.development.js	5.24 kB
./dist/system/vanilla/utils.production.js	3.15 kB
./dist/umd/babel/plugin-debug-label.development.js	1.08 kB
./dist/umd/babel/plugin-debug-label.production.js	852 B
./dist/umd/babel/plugin-react-refresh.development.js	1.27 kB
./dist/umd/babel/plugin-react-refresh.production.js	1 kB
./dist/umd/babel/preset.development.js	1.54 kB
./dist/umd/babel/preset.production.js	1.22 kB
./dist/umd/index.development.js	383 B
./dist/umd/index.production.js	328 B
./dist/umd/react.development.js	1.56 kB
./dist/umd/react.production.js	934 B
./dist/umd/react/utils.development.js	1.53 kB
./dist/umd/react/utils.production.js	1.01 kB
./dist/umd/utils.development.js	399 B
./dist/umd/utils.production.js	342 B
./dist/umd/vanilla/utils.development.js	6.24 kB
./dist/umd/vanilla/utils.production.js	3.79 kB
./dist/utils.js	247 B
./dist/vanilla/utils.js	6.11 kB

_{compressed-size-action}

[github-actions]

Preview in LiveCodes

Latest commit: 79adf23
Last updated: Oct 15, 2024 9:03am (UTC)

Playground	Link
React demo	https://livecodes.io?x=id/AQGQ99S6C

---

See documentations for usage instructions.

[Pinpickle]

@dai-shi I'm not sure why but removing !force from this makes all tests pass... do you know why this would work?

[dai-shi]

cd67e40 --- yeah. I think we miss some optimization tests. I'll try to add one.

[dai-shi]

Hmm, it actually seems fine, because in normal cases (no reordering), dirtyAtoms doesn't include dependencies. So, no performance drawback.

[dai-shi]

There might exist some other edge cases we miss now, but the change is small and looks reasonable.

Thanks to @Pinpickle

---

Memo: Originally, I was thinking about removing from dirtyAtoms to avoid recomputing the same atom again, but we actually need to remove from topSortedAtoms, but then it doesn't add to changedAtoms. Because readAtomState recursively checks epoch number even if it's marked as dirty, it actually doesn't recompute thanks to dropping force.

[xc1427]

This change breaks my app related to some atomEffect usage, anyway I have to figure out why and post an minimum viable repro.

[dai-shi]

Oh... Yeah, a repro would be helpful and we need a test case to investigate further.

[dmaskasky]

I installed both 2.10.1 and https://pkg.csb.dev/pmndrs/jotai/commit/79adf231/jotai as dev dependency for jotai-effect. All tests pass with both versions.

I might be missing some test cases. Please create a minimally viable repro and I can take a look.

[na2hiro]

I also got exceptions with 2.10.1 that doesn't happen in 2.10.0. Right now I don't have time to reduce my repo down to repro but here's the stack trace

Uncaught RangeError: Maximum call stack size exceeded
    at isAtomStateInitialized (chunk-CGZX2XVC.js?v=367fa982:63:30)
    at readAtomState (chunk-CGZX2XVC.js?v=367fa982:164:9)
    at chunk-CGZX2XVC.js?v=367fa982:172:11
    at Array.every (<anonymous>)
    at readAtomState (chunk-CGZX2XVC.js?v=367fa982:168:35)
    at chunk-CGZX2XVC.js?v=367fa982:172:11
    at Array.every (<anonymous>)
    at readAtomState (chunk-CGZX2XVC.js?v=367fa982:168:35)
    at chunk-CGZX2XVC.js?v=367fa982:172:11
    at Array.every (<anonymous>)

[dai-shi]

It feels like a bug with this PR. Hope someone can create a failing test case!

[xc1427]

@dai-shi @na2hiro @dmaskasky

FYI, I fixed the issue (Uncaught RangeError: Maximum call stack size exceeded) in my app.

It seems I may have introduced "circular dependencies" between an atom and the effect of it, like below,

const atomA = atom((get) => {
  get(atomAEffect);
})
const atomAEffect = atomEffect((get) => {
   get(atomA)
})

I think I had exploited a hidden feature by accident. This "feature" is apparently not allowed since version 2.10.1. I fixed my code by eliminating these circular dependencies.

I am now surprised that my app worked 😂😂. I remember the purpose of organizing code like this is to colocate logically atom and its effect, and make sure I do not forget to mount the effect. Because when atomA is mounted, atomAEffect will also take effect. The purpose of atomAEffect is to let atom's change be propagated to some other places.

Is that usage legal ? If not, what is the best practice ?

[dai-shi]

#2823 (comment)

[na2hiro]

It turned out that I was making a direct circular usage of get() between an atom and an atom affect, similarly to what @xc1427 described. Removing it fixed my exception. Thanks folks for the help.

I remember the purpose of organizing code like this is to colocate logically atom and its effect, and make sure I do not forget to mount the effect. Because when atomA is mounted, atomAEffect will also take effect. The purpose of atomAEffect is to let atom's change be propagated to some other places.

Is that usage legal ? If not, what is the best practice ?

To answer @xc1427's question, @dmaskasky posted in #2823 how to always hook effect into an atom. With exporting countWithEffect without countAtom, we can force a client code to use always with an effect:

You can use the withAtomEffect api.

const countAtom = atom(0)
export const countWithEffect = withAtomEffect(countAtom, (get, set) => {
  console.log('count is now', get(countAtom))
})

(I'd name the first one _countAtom and the second one countAtom when effect is a must btw)