AtomicStore features #8
Conversation
|
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. |
src/atomicStore.ts
Outdated
| * @template State - Type of the state definition object | ||
| */ | ||
| export function createAtomicStore<State extends object>( | ||
| initial: AtomicState<State>, |
There was a problem hiding this comment.
another idea:
| initial: AtomicState<State>, | |
| definition: AtomicDefinition<State>, |
src/atomicStore.ts
Outdated
| baseValues[k] = desc.value; | ||
| } | ||
| } | ||
| const rootAtom = atom(baseValues); |
There was a problem hiding this comment.
I think it's better (perhaps more performant) to create baseAtoms first and derive rootAtom from them.
There was a problem hiding this comment.
Yes, it's more performant, and we can try it. It'd be nice to be maximally performant.
I think the tradeoff is that updating several properties at once is a bit more tricky and less atomic, but perhaps not much of a problem — I didn't yet try to make the change.
In terms of performance, at least the direct subscriptions to the atoms will not be triggered unless the atom value changes, so it's just loop over properties, not a loop over all the consumers of the properties.
There was a problem hiding this comment.
Hm, I removed rootAtom and it seems to work
src/atomicStore.ts
Outdated
| ): WritableAtom<void, any[], void> { | ||
| type Args = T[typeof key] extends (...args: infer P) => any ? P : never; | ||
|
|
||
| return atom(null, (get, set, ...args: Args) => { |
There was a problem hiding this comment.
Let's use const ACTION = Symbol() instead of null.
src/atomicStore.ts
Outdated
| get: any, | ||
| store: AtomicStore<T>, | ||
| baseAtoms: Map<keyof T, PrimitiveAtom<any>>, | ||
| initial: AtomicState<T>, |
There was a problem hiding this comment.
I don't think we need to pass this. Just loop over baseAtoms and store?
There was a problem hiding this comment.
I just inlined the helper methods - they were quite tightly coupled to the code anyways (lots of parameters).
| : Atom<T[K]>; | ||
| }; | ||
|
|
||
| const ACTION = Symbol('ACTION'); // Unique symbol to denote action atoms |
There was a problem hiding this comment.
We need to export this from this file, but...
| const ACTION = Symbol('ACTION'); // Unique symbol to denote action atoms | |
| export const ACTION = Symbol('ACTION'); // Unique symbol to denote action atoms |
| @@ -2,3 +2,4 @@ export { atomWithStore } from './atomWithStore.js'; | |||
| export { atomWithActions } from './atomWithActions.js'; | |||
| export { useSelector } from './useSelector.js'; | |||
| export { create } from './create.js'; | |||
| export * from './atomicStore.js'; | |||
There was a problem hiding this comment.
...not from this file.
| export * from './atomicStore.js'; | |
| export { createAtomicStore } from './atomicStore.js'; |
| const baseAtoms = new Map<keyof State, PrimitiveAtom<any>>(); | ||
|
|
||
| // Helper to check property types | ||
| const isPropType = { |
There was a problem hiding this comment.
Just a nit:
Minification doesn't work for properties, so I'd define three functions:
const isActionPropType = (key) => ...;
const isDerivedPropType = (key) => ...;
const isBasePropType = (key) => ...;| const state = {} as T; | ||
|
|
||
| const get = () => state; | ||
| const set = (partial: Partial<T>) => Object.assign(state, partial); |
There was a problem hiding this comment.
This is too naive. state must be in atoms.
I don't think true "atomicStoreFromZustand" is possible. If we limit some usages, there might be a way.
There was a problem hiding this comment.
What if we added back the rootAtom to hold the basic state, and derived atoms from that? The rootAtom would just basically be the Zustand state.
There was a problem hiding this comment.
atoms are configs and actual values are store in the Jotai store. So, there a contract mismatch. But, if we limit something, it might work. The rootAtom can be created from baseAtoms. The question is how to update the rootAtom from set().
|
Hm, I see a lot of errors - let me try to clean up and fix things first so that the tests run at least. Pls hold off reviewing for a while. |
See #7