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

Request for comments: Should all providers be merged into one? #39

Closed
rrousselGit opened this issue Jul 8, 2020 · 12 comments
Closed

Request for comments: Should all providers be merged into one? #39

rrousselGit opened this issue Jul 8, 2020 · 12 comments
Labels
enhancement New feature or request

Comments

@rrousselGit
Copy link
Owner

rrousselGit commented Jul 8, 2020

Hello and thanks for reading!

This issue is about slightly changing the syntax of how provider variants are created.
Feel free to comment and share your opinion on the matter!

The problem

Currently, we have many classes, which are all basically the same modulo a small name + prototype change.

The current syntax is a permutation of:

State Provider
AutoDispose ChangeNotifier Family
Future
Stream
StateNotifier

For a total of 24 variants.

With a secondary table for Computed.

But that's not very ideal:

  • It leads to ridiculously long names, such as AutoDisposeChangeNotifierProviderFamily
  • It pollutes the auto-complete with tons of classes named almost the same
  • Adding new permutations increase the number of providers exponentially

The last point is especially important, as ideally, I would like to add new variants:

  • A "Retry" variant, for well, having states that can be retried
  • Extract Future/Stream in a separate column, such that we could have a FutureStateProvider for example

Which means the table of variants would become:

Provider _
AutoDispose Retry Future StateNotifier Family
Stream State
ChangeNotifier

That leads to a ridiculous 128 providers

The Solution

The idea is to merge all the variants into two public classes: Provider and Computed.
Riverpod would still have 128 providers internally, but they would be code-generated so it doesn't matter for end-users.

The way this new syntax would work is using the Builder design pattern.

Basically, instead of:

final provider = AutoDisposeChangeNotifierProviderFamily<MyNotifier, int>((ref, id) {
  return MyNotifier();
});

We would have:

final provider = Provider
    .changeNotifier
    .autoDispose
    .family<MyNotifier, int>((ref, id) {
  return MyNotifier();
});

Whereas:

final provider = AutoDisposeProvider<Object>((ref) => Object());

would become:

final provider = Provider.autoDispose<Object>((ref) => Object());

And Provider((ref) => Object()) would be untouched.

The difference:

  • Only a single public class. We are not polluting the auto-complete anymore (the 128 variants would still be public but in a different import like package:riverpod/providers.dart)
  • More intuitive. Newcomers will have an easier time discovering all the possibilities
  • A more logical documentation. Instead of documenting "AutoDisposeStreamProvider", we can individually document the .stream and .autoDispose.

Questions

Should some provider variants stay as is, such that we would still have StateProvider(...) instead of Provider.state(...)?

If so, what are the providers that we want to keep this way? I'm thinking of:

  • Provider
  • ChangeNotifierProvider
  • FutureProvider
  • StreamProvider
  • StateProvider
  • StateNotifierProvider

And then for fancier versions like FutureStateNotifierProvider would become StateNotifierProvider.future.

@rrousselGit rrousselGit added the enhancement New feature or request label Jul 8, 2020
@davidmartos96
Copy link
Contributor

davidmartos96 commented Jul 8, 2020

I agree with the proposal.
I think that having too many different class names can be confusing for people trying out the package.
I also agree that keeping some of the providers with the original name can be helpful for people used to the well known Providers in the provider package.

@smiLLe
Copy link

smiLLe commented Jul 8, 2020

For my current project I started to do

final myProvider = createProvider.stateNotifier.family<T, Fam>((r, f) => .. );

by just using callable classes :)

@rrousselGit
Copy link
Owner Author

I've just realised that we can't implement Provider.changeNotifier(...) as ChangeNotifier is Flutter-only, and extensions do not allow adding static members.

So this reinforces the idea of keeping StateNotifierProvider & co, and apply this logic only to AutoDispose, Family and a potential Retry.

@smiLLe
Copy link

smiLLe commented Jul 8, 2020

Personally i would prefer Retry over anything else :P

@rrousselGit
Copy link
Owner Author

Another alternative is to do like @smiLLe suggested and go for final foo = provider.autoDispose(...) instead of final foo = Provider.autoDispose(...).

This will allow using extensions to add custom members. And to begin with, we don't care about const constructors, so this using a function over a constructor doesn't matter.

This was referenced Jul 8, 2020
@rrousselGit
Copy link
Owner Author

rrousselGit commented Jul 9, 2020

I'll go for StateNotifierProvider.autoDispose.family(...).
This avoids a name clash with variables named provider, and is more natural for people migrating from provider

It'll then be accompanied by:

  • .retry, as per Implement a "Retry" feature #42
  • (maybe) .restorable, for state restoration:
    final model = StateNotifierProvider.future.restorable(
      (ref) async {
        final json = await ref.restore();
        final initialState = json == null ? null : MyModel.fromJson(json);
        return MyNotifier(initialState: initialState);
      },
      name: 'model',
      serialize: (state) => state.toJson(),
    );

@rrousselGit
Copy link
Owner Author

I like how composable the modifier idea is. It allows implementing commonly wanted features almost without doing anything.

Do you have any modifiers in mind that I haven't mentioned yet?

Currently we'd have:

  • retry
  • autoDispose
  • family
  • restorable

What other big feature would we want? 🤔

@davidmartos96
Copy link
Contributor

@rrousselGit Some kind of logging mechanism? Not sure if that is possible, just sharing the idea 😅

@rrousselGit
Copy link
Owner Author

rrousselGit commented Jul 9, 2020

Logging in implemented directly on ProviderScope

ProviderScope(
  observers: [
    LoggerObserver(),
  ],
  child: MyApp(),
);

class LoggerObserver extends ProviderStateOwnerObserver {
  ...
}

Although maybe we could have an "onChange" on providers individually

@smiLLe
Copy link

smiLLe commented Jul 9, 2020

What other big feature would we want? 🤔

Do you know recoil js? It is very similar to riverpod :) Maybe we can take some inspiration from them.
But right now, all i am missing is Retry and i like the idea of .restorable

@rrousselGit
Copy link
Owner Author

I do. Finding Recoil made me rename Family. Originally the pattern was called "ParametrizedProvider", which just sounds bad

Anyway, we'll see later. I'll focus on retry/restorable. There's a bunch of work to do on for the provider code-generator

@AlexHartford
Copy link
Contributor

I would agree that the change in name convention makes a great deal of sense here.

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

4 participants