- Notifications
You must be signed in to change notification settings - Fork34
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 ourterms of service andprivacy 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?
Conversation
codesandbox-cibot commentedJul 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This pull request is automatically built and testable inCodeSandbox. To see build info of the built libraries, clickhere or the icon next to each commit SHA. |
| constprevStore=prevCreateStore?.(); | ||
| constbuildingBlocks=( | ||
| prevStore ?INTERNAL_getBuildingBlocksRev1(prevStore) :[] | ||
| )asBuildingBlocks; | ||
| buildingBlocks[0]??=newWeakMap(); | ||
| buildingBlocks[1]??=newWeakMap(); | ||
| buildingBlocks[6]??={}; | ||
| conststoreHooks=INTERNAL_initializeStoreHooks(buildingBlocks[6]); | ||
| constatomWrite=buildingBlocks[8]; | ||
| buildingBlocks[8]=(atom,get,set, ...args)=>{ | ||
| if(inRestoreAtom){ | ||
| returnset(atom, ...args); | ||
| } | ||
| returnatomWrite | ||
| ?atomWrite(atom,get,set, ...args) | ||
| :atom.write(get,set, ...args); | ||
| }; | ||
| conststore=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.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
26b1d10 to5c27901Compare5c27901 to2c586d0Compare| constprevStore=prevCreateStore?.()??INTERNAL_buildStore(); | ||
| constbuildingBlocks=[ | ||
| ...INTERNAL_getBuildingBlocksRev1(prevStore), | ||
| ]asBuildingBlocks; |
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?
constbuildingBlocks=[ ...INTERNAL_getBuildingBlocksRev1(prevStore),];
sebinsua commentedOct 14, 2025
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.