-
-
Notifications
You must be signed in to change notification settings - Fork 706
store hook for when the atom graph recomputes #3200
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?
store hook for when the atom graph recomputes #3200
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. |
|
Is there a better letter name than |
|
| Playground | Link |
|---|---|
| React demo | https://livecodes.io?x=id/2Y5H7SVKD |
See documentations for usage instructions.
commit: |
4975c6f to
c7233f2
Compare
| } | ||
| invalidatedAtoms.delete(a) | ||
| } | ||
| storeHooks.g?.() |
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.
Is this enough for your case? I was thinking about adding hook in the above if loop for each recomputed atom.
This looks pretty similar to storeHooks.f. Doesn't it work for your case? If not, I would like to understand what is the difference.
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 was thinking I would use storeHooks.c for inside the loop and storeHooks.g once the loop has finished.
I agree storeHooks.g is too similar to storeHooks.f. My concern with using storeHook.f was more about semantics. I really don't want to add more hooks if it can be avoided, but I worry that it might just be coincidence that flushCallbacks always runs after recomputeInvalidatedAtoms.
Also, it might race things that run in storeHooks.f, such as atomEffect, but I have a workaround in mind for atomEffect of ot comes to it.
What do you suggest?
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.
My thought is yes, .f and .g are too similar, and not good for the future.
If you care about the semantics, my suggestion is to drop both the current .f and this new .g and create a new hook that works both for -scope and -effect. I'd expect the new one to have better semantics, and probably it will be moved out of flushCallback internal loop.
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.
Ok, I think I'll close the PR for now. I'll try to make it work with storeHooks.f. Keeping storeHooks.f inside the flushCallbacks loop is intentional to process the atoms set by effect.
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.
Keeping
storeHooks.finside the flushCallbacks loop is intentional to process the atoms set by effect.
I see.
Summary
Fires after recomputeInvalidatedAtoms is called.
Check List
pnpm run fixfor formatting and linting code and docs