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

Make update() methods take a non-nullable previous #865

Open
aran opened this issue Feb 29, 2024 · 5 comments
Open

Make update() methods take a non-nullable previous #865

aran opened this issue Feb 29, 2024 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@aran
Copy link

aran commented Feb 29, 2024

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 aran added enhancement New feature or request needs triage labels Feb 29, 2024
@rrousselGit
Copy link
Owner

create is optional. That's not quite feasible

@aran
Copy link
Author

aran commented Mar 1, 2024

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.

@rrousselGit
Copy link
Owner

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

It's low priority for me though.

@aran
Copy link
Author

aran commented Mar 4, 2024

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.

@rrousselGit
Copy link
Owner

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

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

No branches or pull requests

2 participants