provider icon indicating copy to clipboard operation
provider copied to clipboard

Make update() methods take a non-nullable previous

Open aran opened this issue 4 months ago • 5 comments

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

The problem to be solved is that update() methods can't depend on create() having been called, so they have to duplicate code snippets with create. For example:

ChangeNotifierProxyProvider<Input, Output>(
                          create: (context) => Output(...output's parameters...)
                          update: (context, Input input, Output? previous) {
                            previous ??= Output(...output's parameters again...);
                            return previous..updateWithInput(input);
                          }),

Describe the solution you'd like Make previous non-nullable throughout the API and internally guarantee that create() (or .value or whatever) is called first.

aran avatar Feb 29 '24 23:02 aran

create is optional. That's not quite feasible

rrousselGit avatar Mar 01 '24 01:03 rrousselGit

Maybe I am missing something. previous looks optional in InheritedProvider and several others, and inProxyProviderBuilder, which is the underlying issue. For several others, like StreamProvider, FutureProvider, there is no update method. For ListenableProxyProvider, create is optional and update takes a nullable parameter (side quest: why use create at all for these?). But for the ChangeNotifierProxyProvider family, create is required while update takes an optional previous.

https://github.com/rrousselGit/provider/blob/master/packages/provider/lib/src/change_notifier_provider.dart#L106 https://github.com/rrousselGit/provider/blob/master/packages/provider/lib/src/change_notifier_provider.dart#L216

With an optional previous, I don't see how to follow this guidance:

"- DON'T create the [ChangeNotifier] inside update directly." https://github.com/rrousselGit/provider/blob/master/packages/provider/lib/src/change_notifier_provider.dart#L188

If create is required there, maybe ChangeNotifierProxyProvider shouldn't use ProviderBuilder as is. I suppose an alternative would be to make create optional for the ChangeNotifierProxyProvider family, like ListenableProxyProvider, which would also let programmers avoid the duplication by always creating in update (which we are forced to do by the type signature). In that case, the warning guidance should be removed.

aran avatar Mar 01 '24 19:03 aran

You're right, in the few places where create is required, we could make it non-nullable.

It's low priority for me though.

rrousselGit avatar Mar 01 '24 20:03 rrousselGit

Looks like it may not be easy to do in a clean way without a breaking change, because the InheritedProvider supertype would need to make create required as well, or the type hierarchy would need to be changed. Not sure if you are open to a breaking change/major version PR. I don't have the historical context on why that API is the way it is.

aran avatar Mar 04 '24 17:03 aran

You could always use ! in the places where create is required. I don't think changing InheritedProvider is necessary

rrousselGit avatar Mar 04 '24 20:03 rrousselGit