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

refactor: eliminate batch #2925

Merged
merged 27 commits into from
Jan 14, 2025
Merged

refactor: eliminate batch #2925

merged 27 commits into from
Jan 14, 2025

Conversation

dai-shi
Copy link
Member

@dai-shi dai-shi commented Jan 10, 2025

Background

Batch (formerly Pending) was introduced a long time ago. The major purpose was to provide an async context (Like AsyncLocalStorage) to hold state concurrently. It was really necessary at some point (before v1?), but after v2 or so, the purpose has been more for organizing store.ts code so that atoms are isolated. Like the store doesn't know about all atoms. However as we introduced #2827 and other PRs, the store now coordinates atoms as the single source of truth, and we had to tweak Batch. #2905 went further. I expected that I could change it towards the designed shape, but #2919 and #2918 didn't work.

Moving forward

If we use the store as the single source of truth, I don't think we need batch. Interestingly, I just noticed that batch isn't used in readAtomState at all. We are just passing it recursively without consuming it. I don't remember when this has changed. This PR is only to eliminate batch, but I have a big picture in mind which is close to #2911, but I'll explore a bit more. The end goal is probably providing a bunch of hooks, so that the ecosystem libraries can do anything on their own risks. There should be more room to minimize the store code, but that will be tackled afterwards.

Copy link

vercel bot commented Jan 10, 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 14, 2025 2:38am

Copy link

codesandbox-ci bot commented Jan 10, 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 10, 2025

Open in Stackblitz

More templates

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

commit: c601bd9

Copy link

github-actions bot commented Jan 10, 2025

Size Change: -192 B (-0.21%)

Total Size: 91.9 kB

Filename Size Change
./dist/esm/vanilla.mjs 4.18 kB -41 B (-0.97%)
./dist/system/vanilla.development.js 4.27 kB -40 B (-0.93%)
./dist/system/vanilla.production.js 2.16 kB -7 B (-0.32%)
./dist/umd/vanilla.development.js 5.67 kB -10 B (-0.18%)
./dist/umd/vanilla.production.js 2.77 kB -84 B (-2.94%)
./dist/vanilla.js 5.56 kB -10 B (-0.18%)
ℹ️ 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 10, 2025

LiveCodes Preview in LiveCodes

Latest commit: c601bd9
Last updated: Jan 14, 2025 2:38am (UTC)

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

See documentations for usage instructions.

@dai-shi
Copy link
Member Author

dai-shi commented Jan 10, 2025

Wait, I might notice some issues. I will continue working on it. Until then, this isn't ready.

@dmaskasky
Copy link
Collaborator

Beautiful PR. This is the next evolution of the store code. I had a feeling the store was getting quite bloated with all the batch logic, I'm glad you figured this out.

src/vanilla/store.ts Outdated Show resolved Hide resolved
@dmaskasky
Copy link
Collaborator

dmaskasky commented Jan 10, 2025

Is my understanding of the semantic difference between these correct?

  1. atomState.n === previousEpochNumber - the version of the atom value or error. Used to determine whether the atom has changed.
  2. atomState.x - atom is invalidated, indicates whether the atom value needs to be recomputed.
  3. changedAtoms.has(atom) - changedAtoms is a set of atoms where their atom version has changed.

Could we replace ++atomState.n with changedAtoms.add(atom)?

Co-authored-by: David Maskasky <[email protected]>
@dai-shi
Copy link
Member Author

dai-shi commented Jan 11, 2025

Could we replace ++atomState.n with changedAtoms.add(atom)?

Maybe not. n is also used for dependency checking. changedAtom is for write only. Though, I'm not saying what it shows now is ideal. As I noted, there would be many room for improvements, and this is only the first step.

@dai-shi dai-shi marked this pull request as ready for review January 11, 2025 03:40
@dai-shi
Copy link
Member Author

dai-shi commented Jan 11, 2025

/ecosystem-ci run

Copy link

Ecosystem CI Output

---- Jotai Ecosystem CI Results ----
{
  "bunshi": "PASS",
  "jotai-devtools": "PASS",
  "jotai-effect": "FAIL",
  "jotai-scope": "PASS"
}

@dai-shi dai-shi added this to the v2.11.1 milestone Jan 11, 2025
@dai-shi
Copy link
Member Author

dai-shi commented Jan 13, 2025

/ecosystem-ci run

Copy link

Ecosystem CI Output

---- Jotai Ecosystem CI Results ----
{
  "bunshi": "PASS",
  "jotai-devtools": "PASS",
  "jotai-effect": "FAIL",
  "jotai-scope": "PASS"
}

src/vanilla/store.ts Outdated Show resolved Hide resolved
Co-authored-by: David Maskasky <[email protected]>
@dai-shi
Copy link
Member Author

dai-shi commented Jan 14, 2025

Let me confirm that this still fails with jotai-effect.

@dai-shi
Copy link
Member Author

dai-shi commented Jan 14, 2025

/ecosystem-ci run

Copy link

Ecosystem CI Output

---- Jotai Ecosystem CI Results ----
{
  "bunshi": "PASS",
  "jotai-devtools": "PASS",
  "jotai-effect": "FAIL",
  "jotai-scope": "PASS"
}

@dai-shi
Copy link
Member Author

dai-shi commented Jan 14, 2025

Final status:

@dai-shi dai-shi merged commit efb4573 into main Jan 14, 2025
45 checks passed
@dai-shi dai-shi deleted the refactor/eliminate-batch branch January 14, 2025 05:53
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