flutter_hooks icon indicating copy to clipboard operation
flutter_hooks copied to clipboard

Reduce boilerplate code of controller (ChangeNotifier) hooks

Open yelliver opened this issue 1 year ago • 10 comments
trafficstars

If this PR is merged, I will group all of them into a single file, making the repo cleaner.

yelliver avatar Feb 12 '24 18:02 yelliver

That looks interesting.

A few things:

  • We'd need tests for the new hooks
  • They should be added to the readme and the changelog

rrousselGit avatar Feb 12 '24 19:02 rrousselGit

That looks interesting.

A few things:

  • We'd need tests for the new hooks
  • They should be added to the readme and the changelog
  • 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?
  • If we don't expose useChangeNotifier & useDispose, do we need any info in readme?

yelliver avatar Feb 12 '24 20:02 yelliver

If we don't expose useChangeNotifier & useDispose, do we need any info in readme?

No but then we should mark those as @internal

rrousselGit avatar Feb 16 '24 15:02 rrousselGit

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.

rrousselGit avatar Feb 16 '24 15:02 rrousselGit

: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).

rrousselGit avatar Feb 28 '24 11:02 rrousselGit

Let's recap:

  • If we want to export them -> Need to add test cases for them
  • If we don't export them -> Make them @internal

I prefer to export them, if you think it is proper, I will add test cases for them.

yelliver avatar Mar 03 '24 04:03 yelliver

If we want to export them, I'll also be a bit more strict on the API.

rrousselGit avatar Mar 03 '24 09:03 rrousselGit

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.

yelliver avatar Mar 04 '24 04:03 yelliver

I think we need to export them, to reduce these boring code:

      final controller = useMemoized(
        () => SomeChangeNotifier(),
      );
      useEffect(
        () => () {
          controller.dispose();
        },
        [controller],
      );

lvsecoto avatar Mar 13 '24 06:03 lvsecoto

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.

rrousselGit avatar Mar 14 '24 16:03 rrousselGit

This appears to be stale, so I'll close this :)

Feel free to reopen a PR if you want to resume working on this

rrousselGit avatar Jul 08 '24 05:07 rrousselGit