riverpod icon indicating copy to clipboard operation
riverpod copied to clipboard

Warn against using `ref.watch` on a provider's notifier

Open songyang-dev opened this issue 3 months ago • 15 comments

Is your feature request related to a problem? Please describe.

I'm new to Riverpod and I come from Provider. As I was using Riverpod, I thought ref.watch(provider.notifier) was doing the same thing as ref.watch(provider) when it comes to registering the widget for changes. Since the recommended way of triggering a notification to the provider is to use the custom methods declared inside the provider, I thought ref should just watch the provider's notifier to cut down the lines of code.

This leads to the false impression that the widget was watching the provider for changes.

Describe the solution you'd like

  1. The linter should warn against watching a provider's notifier.

Describe alternatives you've considered

  1. A differently named method of ref should be recommended to access the provider instance's methods.

Additional context Here is an example code snippet that I tripped over today.

@override Widget build(BuildContext context, WidgetRef ref) {
    var chosenIntervals = ref.watch(freePracticeIntervalsProvider.notifier); // this does not make the widget rebuild 
    var isSelected = chosenIntervals.contains(musicInterval);
}

songyang-dev avatar Mar 27 '24 02:03 songyang-dev

Can confirm that this is a very common issue I see. I've onboarded multiple juniors onto riverpod, and they all make this mistake at least once.

martinralfreindl avatar Mar 27 '24 07:03 martinralfreindl

In some edge-cases you might want to watch the notifier, however, I agree that this is a super common issue for people who are new to Riverpod, and it should generally be discouraged. Any setup where you don't access a notifier via ref.read whenever you want to use it is also often not the best way to do it I've noticed.

timcreatedit avatar Mar 27 '24 10:03 timcreatedit

Only valid cases when you want to watch notifier that I know of are onPressed: ref.watch(someProvider.notifier).fire and

@override Widget build(BuildContext context, WidgetRef ref) {
    final notifier = ref.watch(someProvider.notifier);
    return Column(
         children: [
                Button(onPressed: () => notifier.fire('boom')),
                Button(onPressed: () => notifier.fire('beep')),
         ]
    );
}

noriHanda avatar Mar 27 '24 10:03 noriHanda

A bit surprised to see this as a warning, as they return different types. I don't see any case to watch a notifier, but we watch the state (AsyncValue) with a provider.

PollyGlot avatar Mar 27 '24 10:03 PollyGlot

To me, that's more a misuse of the notifier than a misuse of ref.watch It is part of the good practices that Notifiers should not expose any state outside of its state field.

There's already a warning telling users not to expose public getters/properties in their notifiers. That notifier.contains(...) in the OP is similar to this. It just bypasses the linter because that's not a getter/property.

There are legitimate use-cases for ref.watch(provider.notifier), especially with the 3.0 where a new Notifier will be recreated when Notifier.build is reinvoked.

Typically this is when using tear-offs:

final notifier = ref.watch(provider.notifier);

return Button(onTap: notifier.increment);

If the notifier were to be recreated, it should re-invoke the build method because the increment method changed.


I could instead see a warning against using Notifier members within build:

Widget build(ctx) {
  final notifier = ref.watch(p.notifier); // OK, no warning

  notifier.doSomething(); // KO, don't use notifier members inside build
}

rrousselGit avatar Mar 27 '24 11:03 rrousselGit

final notifier = ref.watch(provider.notifier);

return Button(onTap: notifier.increment);

Wouldn't this also be less desirable than

 return Button(onTap: ref.read(provider.notifier).increment);

due to the unnecessary rebuilds?

timcreatedit avatar Mar 27 '24 12:03 timcreatedit

No, onTap: ref.read(provider.notifier).increment breaks “No ref.read in build method” rule.

2024/03/27 21:51、Tim @.***>のメール:

final notifier = ref.watch(provider.notifier);

return Button(onTap: notifier.increment); Wouldn't this also be less desirable than

return Button(onTap: ref.read(provider.notifier).increment); due to the unnecessary rebuilds?

— Reply to this email directly, view it on GitHub https://github.com/rrousselGit/riverpod/issues/3451#issuecomment-2022691710, or unsubscribe https://github.com/notifications/unsubscribe-auth/AENYN5FAPUGLCSUBBGZEMGTY2KXDZAVCNFSM6AAAAABFKAHYVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRSGY4TCNZRGA. You are receiving this because you commented.

noriHanda avatar Mar 27 '24 12:03 noriHanda

Wouldn't this also be less desirable than

 return Button(onTap: ref.read(provider.notifier).increment);

due to the unnecessary rebuilds?

No that's incorrect.
Whatever rebuild we have here by using ref.watch is necessary.

If the notifier instance changed, the button must have its onTap method changed to point to the new notifier.

Otherwise you'd be calling increment on a disposed object.

rrousselGit avatar Mar 27 '24 13:03 rrousselGit

Oh, so in that case the tear-off points to an outdated function? That's crazy, I thought what I wrote would be equivalent to

return Button(onTap: () => ref.read(provider.notifier).increment());

Or am I crazy and that also wouldn't work?

Unrelated, but unnecessary_lambdas is a bit misleading then.

timcreatedit avatar Mar 27 '24 13:03 timcreatedit

Nope, both variants aren't the same. That's a common tear-off / variable dereference topic.

unnecessary_lambdas

The warning doesn't appear if a referenced variable is coming from an unknown source. There won't be such an issue here. Otherwise you would see the warning already when using ref.read :)

rrousselGit avatar Mar 27 '24 13:03 rrousselGit

The warning doesn't appear if a referenced variable is coming from an unknown source. There won't be such an issue here. Otherwise you would see the warning already when using ref.read :)

That's why I said misleading, not wrong :) I'm gonna go out there and suspect that if you survey all Flutter devs, most would think that tear-offs are just syntactic sugar (in part because in some cases, their linter tells them to put them there), but maybe I'm projecting.

Anyway, thanks for the explanation!

timcreatedit avatar Mar 27 '24 13:03 timcreatedit

This is technically not coming from tear-offs, but how variables behave.

Consider:

int state = 0;
final previousState = state;
state++;

print('$previousState $state'); // prints "0 1"

That's why you should use ref.watch if you write:

final notifier = ref.watch(provider.notifier);

return Button(onTap: () => notifier.increment());

All of that will be covered by a separate lint that spots watch vs read mistakes.

rrousselGit avatar Mar 27 '24 13:03 rrousselGit

Hi @rrousselGit, thanks for the clarification! I'll keep in mind the differences between watching the provider and the notifier.

songyang-dev avatar Apr 02 '24 03:04 songyang-dev

can you please add a section in the docs clearly explaning this differences with code examples ?

nateshmbhat avatar Apr 26 '24 08:04 nateshmbhat

can you please add a section in the docs clearly explaning this differences with code examples ?

Consider starting a new issue for this if there isn't already one.

songyang-dev avatar May 05 '24 23:05 songyang-dev