-
-
Notifications
You must be signed in to change notification settings - Fork 264
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
Add watch to valtio/utils #157
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. Latest deployment of this branch, based on commit bab8b5b:
|
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.
Great work. Overall it looks good.
I hadn't thought about nested watches. Would you like to add tests that depend currentCleanups?
I have added a test for this, however it isn't that verbose 😅 |
Thanks for the improvement. I'm still not sure the recursive use case, but it's fine for now. |
I could probably remove it as the nested watch and the cleanup use-cases are already covered, no? |
That looks okay. My (non-blocking) comment is about the use case of nested watch. const stop = watch((get) => {
// do something
watch((get) => {
// do something else
return cleanup1
})
return cleanup2
}) When do you need something like this? |
There may be a use-case wherein inside a watch there's a localized proxy object, in which you can watch(() => {
const localProxy = proxy({ count: 0 });
watch((get) => {
console.log(get(localProxy).count);
});
}); Another use-case is if you want to compose cleanups. Adding cleanups through the cleanup return may be inconvenient. There's also the compositional watch(() => {
const signal = new AbortSignal();
watch(() => () => {
signal.abort();
});
// Do other stuffs
}); Also, there's nothing that's going to stop users from doing nested |
Thanks for the clarification.
Again, I'm fine with your proposal. But, if I were to design it, I would require users to do manual cleanup instead of the auto-cleanup. const stop = watch(() => {
const signal = new AbortSignal();
const innerStop = watch(() => () => {
signal.abort();
});
// Do other stuffs
return () => {
// Do other cleanups
innerStop();
};
}); I think it's just a design choice. I prefer manual one, because you never know if you are in watch or not. const fn = () => {
watch(() => {
return cleanup
}
}
// expecting this usage
const stop = watch(() => {
fn()
})
// but someone else could use it directly
fn() |
I was trying to resolve conflicts, but github warns me about the branch. I guess it should be fine, but I'd leave it (= resolving conflicts) to you. |
Let me merge this and cut a release. |
Resolves #149