fix(useAtom): issue-947 propagate atom revision changes correctly

[xbaun]

Track revision changes in useAtom to ensure that the latest atom value is returned.

Issue #947

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/jotai/3qbvdE5ADQCkYhZxF5cof8HCmkwL
✅ Preview: https://jotai-git-fork-xbaun-fix-issue-947-pmndrs.vercel.app

[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.

Latest deployment of this branch, based on commit 6df2937:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
Next.js	Configuration
Next.js with custom Babel config	Configuration
React with custom Babel config	Configuration

[dai-shi]

Oh, you solved it? Nice!
You might feel it too, but I'm not super happy with this ceremony in useAtom.ts.
I feel like we should solve this in store.ts.

Would you like to continue investigating it? We need a test case for this too.

[dai-shi]

This technically means, when we have the same value, but a different revision, this hook triggers unnecessary re-render. We should avoid it and should make a test case to catch it if possible. (guessing, it may or may not happen with suspense.)

[xbaun]

This technically means, when we have the same value, but a different revision ...

That's true, but was exactly my case where the problem emerged: An atom that conditionally read another atom which should invoke onMount and start a subscription, but had initially the same default value.

... this hook triggers unnecessary re-render

Because the same value was returned as in the previous render cycle, the effect was not getting re-rendered, therefore commitAtom not fired and flushPending not invoked.

With the current implementation the effect firing commitAtom should be a good place to call it.
I guess, as an alternative it could be also fired directly in the rerenderIfChanged dispatcher when the revision changed. This would save a render cycle.

      useCallback(
        (prev, nextVersion) => {
          const [nextValue, nextRevision] = getAtomValue(nextVersion)

          if (
            Object.is(prev[1], nextValue) &&
            Object.is(prev[2], nextRevision) &&
            prev[3] === atom
          ) {
            return prev // bail out
          }

          if (!Object.is(prev[2], nextRevision)) {
             setTimeout(() => store[COMMIT_ATOM](atom, version))
          }
          
          return [nextVersion, nextValue, nextRevision, atom]
        },
        [getAtomValue, atom]
      ),

jotai/src/core/useAtom.ts

Lines 78 to 91 in 84a0d68

	useCallback(
	(prev, nextVersion) => {
	const [nextValue, nextRevision] = getAtomValue(nextVersion)
	if (
	Object.is(prev[1], nextValue) &&
	Object.is(prev[2], nextRevision) &&
	prev[3] === atom
	) {
	return prev // bail out
	}
	return [nextVersion, nextValue, nextRevision, atom]
	},
	[getAtomValue, atom]
	),

[dai-shi]

Please forgive me, I didn't looked your issue #947 closely yet. You filed this PR so quickly. 😅

I think if we suspend and resolve with the same state, not useEffect will fire. Or maybe not?

Can you do some experiments without jotai only with React useState, and see if useEffect fires with the said situation?

In any case, this can be an issue on jotai end. If useEffect doesn't fire, and we don't want to do side effects in dispatch phase, so the only solution would be requiring Provider to fire effects. Okay, I feel like I need to sit down and take some time to think about it. If you can create some test cases, that would be helpful.

[dai-shi]

Wait, this is not related to Suspense? My misunderstanding...

[xbaun]

Updates the revision if the dependencies have changed. That's necessary to track when the dependent atom has the same initial value as the accessed one.

[dai-shi]

I ended up with another hacky way in #960. But, it feels comfortable without modifying useAtom.ts and without changing the semantics of revision.

closing this. this pr helped to understand the issue.