Closed yelliver closed 2 months ago
That looks interesting.
A few things:
That looks interesting.
A few things:
- We'd need tests for the new hooks
- They should be added to the readme and the changelog
useChangeNotifier
& useDispose
, do we need any info in readme?If we don't expose useChangeNotifier & useDispose, do we need any info in readme?
No but then we should mark those as @internal
I think the new hooks are used by existing hooks (which are changed in implementation) so their existing test cases should cover, how do you think?
Not if we export those hooks.
:wave: Do you still wish to work on this? If so, we need to decide wether you want those hooks part of the public API, or be internal only (and therefore not exported).
Let's recap:
I prefer to export them, if you think it is proper, I will add test cases for them.
If we want to export them, I'll also be a bit more strict on the API.
which restriction do you want to change? I will change it. But I think I am going to make it internal by hiding it to make this PR move forward first.
I think we need to export them, to reduce these boring code:
final controller = useMemoized(
() => SomeChangeNotifier(),
);
useEffect(
() => () {
controller.dispose();
},
[controller],
);
I don't like the API of that useDispose
. Especially that label
parameter. It feels out of place, considering that's a debug only thing.
If we want a reusable hook for this, I'd like a more Riverpod-like API.
final model = useValue((ref) {
// Offer various life-cycles
ref.onDispose(...);
ref.state = ...;
ref.notifyListeners();
return MyModel();
}, [keys]);
This merges various hooks in a single API.
This appears to be stale, so I'll close this :)
Feel free to reopen a PR if you want to resume working on this
If this PR is merged, I will group all of them into a single file, making the repo cleaner.