-
-
Notifications
You must be signed in to change notification settings - Fork 179
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 useOnListenableChange
#438
Add useOnListenableChange
#438
Conversation
|
@@ -173,7 +171,8 @@ class _OnListenableChangeHookState | |||
@override | |||
void didUpdateHook(_OnListenableChangeHook oldHook) { | |||
super.didUpdateHook(oldHook); | |||
if (hook.listenable != oldHook.listenable) { | |||
if (hook.listenable != oldHook.listenable || |
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.
That's not very efficient. It'd usually add/remove a listener whenever the build method updates.
We should have a listener
method on the Hook class.
Of course we'd need a test for that too :)
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.
Hmm you're right, I never thought about that, most people are going to use it with anonymous functions. Time to refactor all my codebases that use callbacks in hooks as well 😅
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've updated it, I'm not quite sure if this is what you meant, but it seems to work
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.
The issue isn't the anonymous function. It's your didUpdateHook implementation.
Like I said, you could have:
class A extends Hook {
void listener() {
hook.listener();
}
initHook() {
listenable.addListener(listener);
}
dispose() {
listenable.removeListener(listener);
}
}
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.
Thanks for the feedback, however, I honestly can't quite make sense of what you're saying.
Both your previous comment, as well as this one refer to having a listener
method on the hook class, however your pseudo-example calls hook.listener()
from inside a Hook
not a HookState
, which must be a mistake.
I'm therefore assuming you meant to write extends HookState
. It makes sense to optimize listener
changes like this, which is what I did on the most recent commits, but I'm failing to see how this would update whenever the Hook's listenable
changes?
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.
Tests are still passing, aren't they? ;)
this.hook
was mutated when didUpdateHook
was called. So the HookState.listener
method points to the new callback.
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 there's a misunderstanding. If you agree with the latest implementation on this branch, we're on the same page. I was under the assumption that you want to get rid of the entire didUpdateHook
method as well. That's the part that doesn't make sense to me.
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.
If the only parts where the listener is added and removed are initHook
and dispose
respectively, which is what I thought you were pointing at, tests don't pass anymore.
@rrousselGit, I was wondering whether we were on the same page now or if you expect me to do anything still? Just wanted to touch base since I don't know if, and which changes you are proposing, so I'm currently not working on anything 😅 |
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.
Sorry, my bad.
LGTM, but do you mind adding it in the README?
It's added to the "Listenable related hooks:" table, would you want it somewhere else? |
Ah dang it, I didn't see the file. |
Fixes #326
(potentially, people can just call
.value
forValueListenables
, so I would argue an extra hook isn't neededFixes #389