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

fix: should update dependents with the value of the unwrapped atom when the promise resolves #2920

Closed
wants to merge 15 commits into from

Conversation

dmaskasky
Copy link
Collaborator

@dmaskasky dmaskasky commented Jan 8, 2025

Related Bug Reports or Discussions

#2914

Summary

Caused by 34b4f8e and e777f46

Fix

  1. Mount and unmount of dependencies added synchronously.
  2. Do not mount or unmount of dependencies added asynchronously.
  3. Mount and unmount dependencies added asynchronously when the returned promise resolves.

Implementation

atomState.m.q is a Set of all atoms added async. Atoms added async with getter in readAtomState are added to this set. This Set is cleared when a new atom value is set and after recomputeDependents has completed. Only unmount dependencies not present in this Set.

Example:

const f = atom((get) => {
  // allow mounting synchronous atoms immediately
  if (!get(a)) {
    get(b)
  } else {
    get(c)
  }
  return Promise.resolve().then(() => {
    // mount this once promise resolves and don't unmount
    if (!get(a)) {
      get(d)
    } else {
      get(e)
    }
  })
})

// mount a, b
store.sub(f, () => {})
// mount d, e
await Promise.resolve()
// unmount b, mount c
store.set(a, 1)
// unmount d, mount e
await Promise.resolve()

Check List

  • pnpm run fix:format for formatting code and docs

Copy link

vercel bot commented Jan 8, 2025

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 Jan 16, 2025 2:01pm

Copy link

codesandbox-ci bot commented Jan 8, 2025

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.

Copy link

pkg-pr-new bot commented Jan 8, 2025

Open in Stackblitz

More templates

npm i https://pkg.pr.new/jotai@2920

commit: 897dc90

Copy link

github-actions bot commented Jan 8, 2025

Size Change: -142 B (-0.15%)

Total Size: 91.7 kB

Filename Size Change
./dist/esm/vanilla.mjs 4.15 kB -26 B (-0.62%)
./dist/system/vanilla.development.js 4.25 kB -25 B (-0.59%)
./dist/system/vanilla.production.js 2.15 kB -18 B (-0.83%)
./dist/umd/vanilla.development.js 5.64 kB -29 B (-0.51%)
./dist/umd/vanilla.production.js 2.76 kB -15 B (-0.54%)
./dist/vanilla.js 5.53 kB -29 B (-0.52%)
ℹ️ 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.04 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 864 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.25 kB
./dist/system/vanilla/utils.production.js 3.14 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.57 kB
./dist/umd/react.production.js 936 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.78 kB
./dist/utils.js 247 B
./dist/vanilla/utils.js 6.1 kB

compressed-size-action

Copy link

github-actions bot commented Jan 8, 2025

LiveCodes Preview in LiveCodes

Latest commit: 897dc90
Last updated: Jan 16, 2025 2:01pm (UTC)

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

See documentations for usage instructions.

Comment on lines +154 to +166
it('should update dependents with the value of the unwrapped atom when the promise resolves', async () => {
const store = createStore()
const asyncTarget = atom(() => Promise.resolve('value'))
const target = unwrap(asyncTarget)
const results: string[] = []
const derived = atom(async (get) => {
await Promise.resolve()
results.push('effect ' + get(target))
})
store.sub(derived, () => {})
await new Promise((r) => setTimeout(r))
expect(results).toEqual(['effect undefined', 'effect value'])
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. So, it's a regression, right? My bad... I'll see what I can do with the impl. It will be step by step and take a bit of time. Sorry folks for waiting.

@dai-shi dai-shi added this to the v2.11.1 milestone Jan 9, 2025
@dmaskasky dmaskasky closed this Jan 15, 2025
@dai-shi
Copy link
Member

dai-shi commented Jan 15, 2025

Continue to #2936.

@dai-shi dai-shi deleted the fix-unwrap-atomEffect branch January 15, 2025 10:59
@dai-shi dai-shi restored the fix-unwrap-atomEffect branch January 15, 2025 15:17
@dai-shi
Copy link
Member

dai-shi commented Jan 15, 2025

I change my mind. Let me use this branch, which is easier for me to push changes.

@dai-shi dai-shi reopened this Jan 15, 2025
@@ -94,7 +94,7 @@ it('correctly handles the same promise being returned twice from an atom getter
await expect(store.get(derivedAtom)).resolves.toBe('Asynchronous Data')
})

it('keeps atoms mounted between recalculations', async () => {
it('keeps sync atoms mounted between recalculations', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the planned breaking change in v2.12.0.

@yuneco Will you be able to check if this breaks your app?

npm i https://pkg.csb.dev/pmndrs/jotai/commit/b4bb8a8d/jotai

Copy link
Member

@dai-shi dai-shi Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuneco Never mind! Moved to #2940.

@dai-shi dai-shi marked this pull request as draft January 16, 2025 13:06
@dai-shi
Copy link
Member

dai-shi commented Jan 16, 2025

Split to #2940.

@dai-shi dai-shi closed this Jan 16, 2025
@dai-shi dai-shi deleted the fix-unwrap-atomEffect branch January 16, 2025 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants