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(core): more readability for addBatchFunc #2898

Merged
merged 6 commits into from
Jan 3, 2025

Conversation

dmaskasky
Copy link
Collaborator

@dmaskasky dmaskasky commented Jan 1, 2025

Summary

Adds iterable to batch to make flushBatch agnostic to the internals of batch.
edit: Just refactoring for now delaying the decision

Check List

  • pnpm run prettier for formatting code and docs

Copy link

vercel bot commented Jan 1, 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 3, 2025 1:43am

Copy link

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

Open in Stackblitz

More templates

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

commit: 6cec4e1

Copy link

github-actions bot commented Jan 1, 2025

LiveCodes Preview in LiveCodes

Latest commit: 6cec4e1
Last updated: Jan 3, 2025 1:43am (UTC)

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

See documentations for usage instructions.

@dai-shi
Copy link
Member

dai-shi commented Jan 1, 2025

I thought this would decrease the bundle size, but why does this increase?


Size Change: +205 B (+0.22%)

Total Size: 92.2 kB

Filename Size Change
./dist/esm/vanilla.mjs 4.28 kB +40 B (+0.94%)
./dist/system/vanilla.development.js 4.38 kB +40 B (+0.92%)
./dist/system/vanilla.production.js 2.13 kB -10 B (-0.47%)
./dist/umd/vanilla.development.js 5.72 kB +66 B (+1.17%)
./dist/umd/vanilla.production.js 2.86 kB +5 B (+0.18%)
./dist/vanilla.js 5.61 kB +64 B (+1.15%)
ℹ️ 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

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

dmaskasky commented Jan 1, 2025

I thought this would decrease the bundle size, but why does this increase?

  • I had added a placeholder for syncEffect, but I realized we should do that in the syncEffect PR, so I reverted it just now
  • replacing inlined functions with named functions
  • Omit<ReadonlyArray<Set<() => void>>, number> & { length: 3 } is to preserve jsdoc comments while retaining tuple-like functionality

@@ -163,30 +163,24 @@ const addDependency = <Value>(
// Batch
//

type BatchPriority = 'H' | 'M' | 'L'
type BatchPriority = 0 | 1 | 2
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use numbers as object keys (internally they will be converted into strings.)

So, my proposition is:

type Batch = Readonly<{
  0: Set<() => void>
  1: Set<() => void>
  2: Set<() => void>
  D: Map<AnyAtom, Set<AnyAtom>>
}>

Copy link
Member

@dai-shi dai-shi Jan 1, 2025

Choose a reason for hiding this comment

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

https://github.com/pmndrs/jotai/pull/2898/files#r1900329774

If we were to generalize the batch queues, I might consider a single ordered queue. Not sure about the performance or the bungle size, though.

Copy link
Collaborator Author

@dmaskasky dmaskasky Jan 1, 2025

Choose a reason for hiding this comment

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

A queue queue might be nice. We wouldn't need explicit placeholders defined in the store for third-party applications.
Its a little more on bundle size though.

Rough:

const batch = (() => {
  const H = new Set()
  const M = new Set()
  const L = new Set()
  const queue = [H, M, L]
  const D = new Map()
  const batch = Object.assign(queue, { H, M, L, D })
  return batch
})()
...
const syncEffect = new Set()
// schedule sync effect after H, but before M
const hIndex = batch.findIndex(element => element === batch.H)
batch.splice(hIndex + 1, 0, syncEffect)

Copy link
Member

@dai-shi dai-shi Jan 1, 2025

Choose a reason for hiding this comment

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

I meant a single combined queue. But don't take it seriously as it may have performance drawbacks.

Copy link
Member

Choose a reason for hiding this comment

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

For now, I think queue array is better. 8945774

Copy link
Collaborator Author

@dmaskasky dmaskasky Jan 2, 2025

Choose a reason for hiding this comment

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

I wonder if we could also allow "insert queue after..." capability.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I'd consider a single priority queue.

src/vanilla/store.ts Outdated Show resolved Hide resolved
@dmaskasky dmaskasky changed the title refactore(core): replace batch letter priorities with number priorities refactor(core): add iterable to batch Jan 2, 2025
@dmaskasky dmaskasky mentioned this pull request Jan 2, 2025
1 task
@dai-shi
Copy link
Member

dai-shi commented Jan 3, 2025

Using iterable batch is still under consideration. However, I'm leaning towards it.
As it's very related with onInit and sync effect, I'd like to delay the decision.
Meanwhile, I want to release a patch. So, let me tweak this PR for a simple refactor. (and acceptable bundle increases.)

Comment on lines +188 to +199
const createBatch = (): Batch => {
const batch = { D: new Map() } as {
D: Map<AnyAtom, Set<AnyAtom>>
[BATCH_PRIORITY_HIGH]: Set<() => void>
[BATCH_PRIORITY_MEDIUM]: Set<() => void>
[BATCH_PRIORITY_LOW]: Set<() => void>
}
batch[BATCH_PRIORITY_HIGH] = new Set()
batch[BATCH_PRIORITY_MEDIUM] = new Set()
batch[BATCH_PRIORITY_LOW] = new Set()
return batch
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super confident with it, but this seems better for minification.
image

We will be revisiting it soon with sync effect anyway, otherwise, we can revert it back.

@dai-shi dai-shi changed the title refactor(core): add iterable to batch refactor(core): more readability for addBatchFunc Jan 3, 2025
@dai-shi dai-shi merged commit 4091b49 into pmndrs:main Jan 3, 2025
42 checks passed
@dai-shi dai-shi added this to the v2.11.1 milestone Jan 3, 2025
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