-
Notifications
You must be signed in to change notification settings - Fork 35
Compose createDevStore with previous createStore #199
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
base: main
Are you sure you want to change the base?
Compose createDevStore with previous createStore #199
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. |
| const prevStore = prevCreateStore?.(); | ||
| const buildingBlocks = ( | ||
| prevStore ? INTERNAL_getBuildingBlocksRev1(prevStore) : [] | ||
| ) as BuildingBlocks; | ||
| buildingBlocks[0] ??= new WeakMap(); | ||
| buildingBlocks[1] ??= new WeakMap(); | ||
| buildingBlocks[6] ??= {}; | ||
| const storeHooks = INTERNAL_initializeStoreHooks(buildingBlocks[6]); | ||
| const atomWrite = buildingBlocks[8]; | ||
| buildingBlocks[8] = (atom, get, set, ...args) => { | ||
| if (inRestoreAtom) { | ||
| return set(atom, ...args); | ||
| } | ||
| return atomWrite | ||
| ? atomWrite(atom, get, set, ...args) | ||
| : atom.write(get, set, ...args); | ||
| }; | ||
| const store = INTERNAL_buildStore(...buildingBlocks); | ||
| const [atomStateMap, mountedAtoms] = buildingBlocks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dai-shi
I think we need more thought around how to derive a store from a previous store. Currently, overriding certain building blocks won't work as expected.
dai-shi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, thanks for working on this.
26b1d10 to
5c27901
Compare
5c27901 to
2c586d0
Compare
| const prevStore = prevCreateStore?.() ?? INTERNAL_buildStore(); | ||
| const buildingBlocks = [ | ||
| ...INTERNAL_getBuildingBlocksRev1(prevStore), | ||
| ] as BuildingBlocks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it already mutable without the type annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think buildingBlocks should be frozen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should be frozen. I'm only talking about the typing. Doesn't this work?
const buildingBlocks = [
...INTERNAL_getBuildingBlocksRev1(prevStore),
];|
Since updating Do you know if this fixes that issue? It might somehow relate to my project using a custom store or it could be something else. I've not been able to work out what's happening yet. |
Summary
If a previous createStore has already overridden createStore, we should try our best to compose with it.