Skip to content
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

Reduce boilerplate code of controller (ChangeNotifier) hooks #415

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yelliver
Copy link

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

@rrousselGit
Copy link
Owner

That looks interesting.

A few things:

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

/// ```
T useChangeNotifier<T extends ChangeNotifier>(
T Function() factory, [
String? label,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this being part of the public API

Copy link
Author

@yelliver yelliver Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I commented, it can be used to hook a common ChangeNotifier that has no hook yet, for example,CupertinoTabController which was expected by some people:
#222

@yelliver
Copy link
Author

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?

@rrousselGit
Copy link
Owner

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

No but then we should mark those as @internal

@rrousselGit
Copy link
Owner

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
Copy link
Owner

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

@yelliver
Copy link
Author

yelliver commented Mar 3, 2024

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.

@rrousselGit
Copy link
Owner

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

@yelliver
Copy link
Author

yelliver commented Mar 4, 2024

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.

@lvsecoto
Copy link

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

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

@rrousselGit
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants