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

introduce atom.onInit hook #2901

Closed
wants to merge 48 commits into from

Conversation

dmaskasky
Copy link
Collaborator

@dmaskasky dmaskasky commented Jan 2, 2025

Related PRs

#2888
#2801

Summary

Uses atomState change and mount hooks to schedule effect and cleanup.

Key Points

  1. Introduces atom.unstable_onInit that fires in place when an atomState is first created in the store. store and atomState are passed as params to atom.unstable_onInit.
  2. Uses atomState.h that fires in-place when an atom has mounted or unmounted, and if an atom has unmounted it schedules a cleanup to run with batch priority 0.5. Batch is a param passed to atomState.h function.
  3. Uses atomState.u that fires in-place when an atom has updated, and if an atom has updated it schedules an effect to run with batch priority 0.5. Batch is a param passed to atomState.u function.
  4. Makes batch and iterable to make flushBatch evaluation agnostic to its shape.

Check List

  • pnpm run prettier for formatting code and docs

src/vanilla/atom.ts Outdated Show resolved Hide resolved
src/vanilla/store.ts Outdated Show resolved Hide resolved
Comment on lines +172 to 178
type Batch = Set<() => void>[] & {
/** High priority functions */
H: Set<() => void>
/** Medium priority functions */
M: Set<() => void>
/** Low priority functions */
L: Set<() => void>
Copy link
Member

Choose a reason for hiding this comment

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

If we were to introduce atomState.us, I don't think we need queue array. H/M/L would be probably more performant. If not, it still feels more readable. But, if queue array is more performant, I would change my mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bundle size with queue array is smaller, and the performance is the same. A queue array is nice because it makes flush evaluation agnostic to the shape of batch.

error?: unknown
}

export function atomSyncEffect(effect: Effect): Atom<void> {
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep this in effect.test.ts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I just have test for puzzle 3 right now. This PR is not ready to merge until all jotai-effect tests are passing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm keeping separate for now while I have this PR in draft. I still have to remove debugLabels, labeled atomStates, and console.logs as well.

@dmaskasky dmaskasky force-pushed the partial-sync-effect-4 branch from 9edf139 to 080eeb7 Compare January 2, 2025 15:44
@dmaskasky dmaskasky changed the title introduce onInit, mount, and update hooks introduce atom.onInit hook Jan 2, 2025
@dai-shi
Copy link
Member

dai-shi commented Jan 2, 2025

Please reopen a new PR with https://github.com/pmndrs/jotai/tree/partial-sync-effect-4 branch. I will add my suggestions there. So, please avoid force pushing remote branches in the jotai repo.

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