[ Provider Doc ] ChangeNotifier inside update

[tperraut]

samples/provider_shopper/lib/main.dart

Line 32 in b7ec38a

CartModel(catalog, previousCart),

I was implementing a little test project with official Flutter doc about state management and looking at your awesome example. Thank you a lot btw.

While reading the ChangeNotifierProxyProvider documentation, I found this :

/// - **DON'T** create the [ChangeNotifier] inside `update` directly.
///
///   This will cause your state to be lost when one of the values used updates.
///   It will also cause uncesserary overhead because it will dispose the
///   previous notifier, then subscribes to the new one.
///
///  Instead reuse the previous instance, and update some properties or call
///  some methods.

So, if creating a new ChangeNotifier inside update callback is a bad practice as the doc says, you could change a bit the sample behavior here or add a little comment.

[redbrogdon]

Huh.

It looks like the chief concern for @rrousselGit is that you don't lose the state contained in your ChangeNotifier by tossing it out and creating a new one. Though it does create a new instance, the app doesn't do that, since it uses a copy constructor of sorts to maintain the selected item state:

https://github.com/flutter/samples/blob/master/provider_shopper/lib/models/cart.dart#L20

@filiph What do you think? I appreciate both that the code is correct and that @tperraut is making a valid point. Maybe a one-liner comment on L31 would help explain what's going on in the update method?

https://github.com/flutter/samples/blob/master/provider_shopper/lib/main.dart#L31

[rrousselGit]

There's more than losing the state to that statement
Some background: rrousselGit/provider#260

By cloning the instance instead of reusing it, it'll force an update on dependents for no reason.

It may also be very dangerous with click handlers.
The community likes doing:

class MyButton {
  ...

  Widget build(context) {
    final myNotifier = Provider.of<MyNotifier>(context, listen: false);

   return RaisedButton(
     onPressed: myNotifier.increment,
   );
  }
}

But if since they pass listen: false, the click handler may point to a previous instance of the notifier.

It's effectively their fault, but not cloning simply removes the possibility of making a mistake.

[rrousselGit]

A more concrete dangerous use-case is asynchronous operations:

class MyNotifier with ChangeNotifier {
  String property;  

  Future<void> doSomething() async {
    property = await something();
  }
}

In such code, when using clone, if update was somehow called before doSomething finished, the mutation will be performed on the wrong object.

[redbrogdon]

@rrousselGit That makes total sense. Thanks for dropping by. 😄

Bottom line is that the whole point of ChangeNotifier is to be mutable and notify upon change, so to swap out the ChangeNotifier instance itself rather than mutating it and updating dependents is working against the design.

@filiph, based on this discussion, I feel we should change CartModel so a single instance can be updated with a new catalog. What do you think?

[filiph]

Makes sense! Good points all around. I'm traveling but will try to get to make the PR (and possibly upgrade to provider in a follow on change) this week.

[filiph]

I made #298. I would be grateful for feedback from y'all.

[redbrogdon]

LGTM

[filiph]

Closing with #298.