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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is this a duplicate of "provider"? #3

Closed
rrousselGit opened this issue Feb 20, 2019 · 11 comments
Closed

Is this a duplicate of "provider"? #3

rrousselGit opened this issue Feb 20, 2019 · 11 comments
Assignees

Comments

@rrousselGit
Copy link

@rrousselGit rrousselGit commented Feb 20, 2019

馃憢 maintainer of provider here

It seems that this library aims at doing exactly what mine does: a set of generic providers to replace Inheritedwidget.

The main selling point of flutter-provide seem to be able to use multiple providers at once; which is already supported by provider through MultiProvider

What is the reasoning by creating a new one?

@filiph filiph self-assigned this Feb 20, 2019
@filiph

This comment has been minimized.

Copy link
Member

@filiph filiph commented Feb 20, 2019

Hey @rrousselGit,

This package originated in Fuchsia and has been used by Google for some time (along with the previous incarnation of it, pkg:scoped_model). We decided to open source it because it's minimal (1 file, 386 LOC + comments), simple to understand (small API surface, simple ), battle-tested, and focused.

I don't think it's a duplicate with your package. Yours is more feature-ful and has different ideas.

Sorry about the naming closeness. I guess great minds (you & @jiaeric) think alike. We did not know about package:provider when we decided to open source this. We thought about renaming pkg:provide to something else to avoid the closeness to pkg:provider but in the end no other naming option won over the simplicity of provide. It might make sense to add a disambiguation note in the README so people know this is not your package.

@rrousselGit

This comment has been minimized.

Copy link
Author

@rrousselGit rrousselGit commented Feb 20, 2019

I don't particularly mind the name conflict.

I'm more concerned about the conceptual conflict.
With the open sourcing of this package, I've discussed with Brian Egan this afternoon about flutter-provide and the future of provider/scoped_model/...

Overall I think:

  • The community needs a popular wrapper over InheritedWidget.
  • Many existing packages try to solve the same problem. They have some differences, but none big enough that justify using both packages in the same project. They overlap too much.
  • A Google-branded solution de facto will dominate over its concurrence

As such scoped_model definitely is doomed in the long term, with provider having a bit more hopes as it's less state management.


Ironically I'm not a fan of the API of flutter-provide at least in my eye.

It pushes bad practices that even the readme do:

  • Provide.stream:
Provide.stream<Counter>(context)
  .where((counter) => counter.value % 2 == 0),

This little snippet causes an issue because we're effectively creating a new stream on every build of CounterApp.

  • Provide.value:
@override
Widget build(BuildContext context) {
    // Gets the Counter from the nearest ProviderNode that contains a Counter.
    // This does not cause this widget to rebuild when the counter changes.
    final currentCounter = Provide.value<Counter>(context);

That one cheats the widgets system by not listening to Counter.

Flutter did a great job at preventing mistakes with InheritedWidget using the of pattern: It is technically impossible to make a widget that depends on Theme, that doesn't update when Theme change.

Provide.value reintroduce this issue while looking innocent.

  • Providers

The idea of exposing multiple providers at once is cool. But by not being a widget, its API is misleading.

We are easily tempted to do:

class Foo extends StatelessWidget {
    const Foo({Key key, this.child}): super(key: key);

    final Widget child;

    @override
    Widget build(BuildContext context) {
        return ProviderNode(
            providers: Providers()
              ..provide(Provider.function((context) => Counter(0))),
            child: child,
        );
    }
}

Again, this looks innocent. But if Foo ever rebuilds, then we're losing all of our providers here.


provider was born from me answering SO questions 鈥 where I could regularly see questions about peoples facing these issues.

I'm not saying that we should use provider. But I'm genuinely concerned about where this package will lead the community.

By having Google back up a solution that looks that unsafe, I fear that instead of solving problems this will increase them.

@jiaeric

This comment has been minimized.

Copy link
Contributor

@jiaeric jiaeric commented Feb 20, 2019

Hi Remi,

Let me read through and think about your concerns for a bit, I need to mull them over to make sure my thoughts make sense. I want to make sure I address the meat of your concerns, and not just gloss them over.
That said, here's a bit of background info and a few points in the meantime.

I think from an approach perspective, the biggest difference between provide and provider is that provide isn't using InheritedWidget directly, and instead uses a container class. You've correctly surmised that one of the big benefits is that it supports many providers at once. But the main reason I went with that option is to allow subtypes and interfaces. So you can create a Provider that returns something that implements T. As I found out the hard way when refactoring code, InheritFromWidgetOfExactType doesn't allow this.

Provide.stream - this indeed creates a new filtered stream each time. I'm not sure what the concern about that is though - is it that people will hang on to old streams?

Provide.value - this was intentionally created to not rebuild, because we found we were having some performance problems when too many things were rebuilding on each change. As such, we needed a way to obtain injected values without creating a rebuild.

@jiaeric

This comment has been minimized.

Copy link
Contributor

@jiaeric jiaeric commented Feb 20, 2019

I think you have a very good point about the Providers part; it seems pretty easy to shoot yourself in the foot there.
I'm thinking that replacing the providers: field with an initProviders: function would solve this. The function would only be called on ProviderNode state initialization

@rrousselGit

This comment has been minimized.

Copy link
Author

@rrousselGit rrousselGit commented Feb 21, 2019

Hello 馃槃

I hope that I do not sounds harsh or trying to sell my product.
I'm only concerned about the community aspect, especially about foolproof-ness.

From a recent discussion with @filiph, it seems that the goal for this library is to be the standard for newcomers.
But as such, it should prevent any mistakes newcomers may do.
Flutter is very good at this. And I think if Google wants flutter-provide to be the standard, then it is necessary for it to be at least on the same level as Flutter.


I think you have a very good point about the Providers part; it seems pretty easy to shoot yourself in the foot there.

That's one of the reasons provider works this way

For example, what would be written as:

ProviderNode(
  providers: Providers()
    ..provide(Provider.value(42)
    ..provide(Provider.function((context) => Counter(0))),
  child: child,
);

becomes:

MultiProvider(
  providers: [
    Provider(value: 42),
    StatefulProvider(valueBuilder: (context) => Counter(0)),
  ],
  child: child,
)

On the verbosity/readability aspects, these two are almost identical (although flutter-provide names are pretty confusing).

The difference is that Provider and StatefulProvider are widgets. As such:

  • rebuilding the creator of MultiProvider doesn't cause any harm. We're not losing the state of StatefulProvider since it is associated with its own Element.
  • the API is very familiar, works with immutability and is compatible with the upcoming collection features for Dart.
  • we're not losing dynamicity (as opposed to a initProviders function like you suggest). If a widget rebuilds and change Provider(value: 42) into Provider(value: 24), then this will correctly update.
  • it is easy to refactor. We can extract one provide from MultiProvider and place them wherever we want.
  • Providers can depend on each other
  • Providers can use keys

But the main reason I went with that option is to allow subtypes and interfaces. So you can create a Provider that returns something that implements T. As I found out the hard way when refactoring code, InheritFromWidgetOfExactType doesn't allow this.

Sorry, I do not understand what you're saying here.
Are you speaking of downcasting?

From my understanding, this is possible:

class Foo {}
class Bar implements Foo {}

Provider<Foo>(
  value: Bar(),
  child: ...
);

But I'm likely misunderstanding the problem.


Provide.stream - this indeed creates a new filtered stream each time. I'm not sure what the concern about that is though - is it that people will hang on to old streams?

  • it is inefficient. Whenever its creator rebuilds, StreamBuilder will unsubscribe to the previous stream and listen to the new one.
  • it can cause an issue since the internal controller is not a BehaviorSubject from Rx.
    Assuming that something has already been pushed in the stream, when the stream changes ConnectionState will reset to waiting (instead of active).
    And if it was a BehaviorSubject, then this would cause a pointless rebuild.

Provide.value - this was intentionally created to not rebuild

If possible, it probably shouldn't be the default behavior (although will probably be difficult with the current API).

As an example, both scoped_model and provider allows obtaining the current value without listening:

  • ScopedModel.of(context, rebuildOnChange: false)
  • Provider.of(context, listen: false)

These do the same thing, but are not the default behavior. In that situation, a mistake from the developer doesn't do any harm.
It may degrade performances a bit, but the application will work.

The opposite is different. It's easy to forget to listen (no warning/error), and such mistake can break the application.

@jiaeric

This comment has been minimized.

Copy link
Contributor

@jiaeric jiaeric commented Feb 21, 2019

Your feedback is very appreciated! As you said, anything that makes the community better is better.

I think the stream issue wouldn't be an issue if initialized. Though as you said, this isn't a problem with your way of doing things.

Re: downcasting - We had a bunch of implemented inheritedWidgets that were writing their own .of(foo) functions. You're right that Provider also solves this problem (as well as scopedModel now it seems, which has changed from the old version I used at the time)

I'm reading through the provider, and your additional comments. Might be easier to do a chat or video chat and loop back here

@brianegan

This comment has been minimized.

Copy link

@brianegan brianegan commented Feb 21, 2019

Hey all, just thought I'd pop in here and let ya know what I'm thinking from these discussions:

  1. Agree it'd be great to have something "standard" that works well for the community!
  2. I do think it'd be great to work towards the simplest possible API so it's really easy to use for as many folks as possible
  3. WRT that, I don't know if the api "doesn't look good", but I was personally wondering if the ProviderScope stuff might be difficult to use / if it would scale well in a larger project. I certainly understand the need for that kind of functionality, but just wasn't sure I'd go that route myself to achieve it. BUT: I want to be clear: I haven't use this lib enough to really know -- that was just an initial impression / thoughts!
  4. I might not be as concerned as others about having multiple packages doing similar things. I think there are about 800 redux implementations at this point on pub :) Peeps can use the solutions that work for them, as long as there's clear advice for beginners.

Overall, I think this is a great discussion to have.

@filiph

This comment has been minimized.

Copy link
Member

@filiph filiph commented Feb 22, 2019

Ok, I just wanted to give a quick update.

  • We really, really like package:provider.
  • Remi's points all make sense, and in most ways package:provider is a better solution than package:provide, especially for newcomers.
  • We don't want to add to confusion & fragmentation in this area.

I regret that I didn't have a better look at package:provider before we went forward with publishing package:provide. (My feeble excuse is that I was immediately distracted by the dependency on flutter_hooks and gave it a pass.) My apologies.

The upside is that we can have this discussion, over concrete code.

There's more discussion to be had with the Flutter team, and the Google teams that use Flutter. I expect to have more to say on this thread sometime next week. In the meantime, I will add a notice to the README so that people don't prematurely start using the package just because it's "official" (i.e. from Google).

@lukepighetti

This comment has been minimized.

Copy link

@lukepighetti lukepighetti commented Feb 24, 2019

I would like to see provider reduce its scope, not depend on flutter_hooks, and all three options merge their efforts. I also think provider belongs inside flutter Just a passing opinion. Cheers. :]

@rrousselGit rrousselGit mentioned this issue Feb 27, 2019
@filiph

This comment has been minimized.

Copy link
Member

@filiph filiph commented Mar 21, 2019

I said I'd update this thread "next week" and then it's next month. Sorry about that.

We're in active discussion with @rrousselGit. He's building the next version of package:provider here in the v2.0 branch of his repo. It's removing the dependency on flutter_hooks, among other things. I've had people on the team look at it, with positive feedback. I think we're getting close.

I also added a note to https://pub.dartlang.org/packages/provide that directs people towards this discussion.

Cheers!

@filiph

This comment has been minimized.

Copy link
Member

@filiph filiph commented Aug 27, 2019

Ok, time to make a difficult decision. We still like provide but I like provider more, and (as has been already said) provider superseded provide even internally at Google. In the area of "simple" InheritedWidget-based state management solutions, provider is clearly more popular, too.

So as not to confuse matters, and because there is not likely to be further development of provide from Google, I'm going to more forcefully redirect people from the README, and I'll ask Google's Open Source team to move this repo to github.com/googlearchive.

To be clear, the package will always be available at https://pub.dev/packages/provide 鈥 pub doesn't allow packages to "disappear". But it won't be updated.

I have looked at all the 33 forks of this project to see if there's active development in any of them. It seems there isn't. If I missed something, please reach out to me. (Just comment on this thread.)

Apologies for open sourcing something that didn't pan out in the end. I personally think it's better for everyone this way, as we now have a very strong community-initiated package. I'm just sorry for the confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can鈥檛 perform that action at this time.