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

[RFC] Renaming the parameter 'builder' of providers #259

Closed
rrousselGit opened this issue Nov 12, 2019 · 30 comments · Fixed by #275 · May be fixed by #243

Comments

@rrousselGit
Copy link
Owner

@rrousselGit rrousselGit commented Nov 12, 2019

Hello!

TD;DR:
This proposal is for a breaking change: renaming builder of Provider/ChangeNotifierProvider/..., to either initialValue or initialValueBuilder.

Which means before:

Provider<int>(
  builder: (context) => 42,
);

after:

Provider<int>(
  initialValue: (context) => 42,
);

or initialValue -> initialValueBuilder.

Feel free to suggest other names or express any point of view on the topic.

Motivation

Currently, the default constructor of providers all has a named required parameter builder, that builds a value once.

But this name causes a few issues.

  • it's misleading.
    In the context of Flutter, a parameter named builder is usually a function that returns a widget, not a model. Such as:

    StreamBuilder(
      builder: ...
    )

    Renaming it to will remove the confusion.
    And a name such as initialValue also makes it more apparent that builder will not be called again.

  • it prevents fusing *Provider and *ProxyProvider.
    These two widgets are very similar.
    Usually, *Provider is just syntax sugar for *ProxyProvider with only an initialBuilder like so:

    Provider(
      builder: (_) => 42,
    );

    is equivalent to:

    ProxyProvider<*, int>(
      initialBuilder: (_) => 42,
      builder: (_, __, value) => value,
    );

    But it is currently impossible to merge both these widgets because they have both a parameter named builder but with a different prototype.

    By renaming builder to something more expressive, it is possible to host both *Provider and *Proxy under the same class.

    We could have:

    ChangeNotifierProvider(
      initialValue: (_) => MyNotifier(),
    );

    Then we'd remove ProxyProvider.
    And add an extra value or valueBuilder parameter on Provider:

    ChangeNotifierProvider(
      initialValue: (_) => MyNotifier(),
      value: (context, notifier) {
        // Works like *ProxyProvider.builder
        // we can safely call `Provider.of` here
        return notifier
          ..foo = Provider.of<Foo>(context)
      },
    );

    Of course, we'd still have numeric syntax sugar like ProxyProvider2/..., but without the Proxy keyword:

    Provider1<Dependency, Result>(
      value: (context, Dependency dep, Result previous) {
        return Result(dep);
      },
    )
    
  • it prevents using builder for an actual "builder" that behaves likes Consumer/StreamBuilder.

    For example, a common use-case is to do:

    Provider<T>(
      builder: (_) => T(),
      child: Consumer<T>(
        builder: (_, value, __)  {
          // TODO: use `value`
        },
      ),
    )

    But that's pretty verbose.

    We could simplify it to:

    Provider<T>(
      initialValue: (_) => T(),
      builder: (_, value, __) {
        // TODO: use `value`
      },
    )

    Which would be strictly equivalent to the previous code – just smoother to write.
    But such simplification is not possible because builder is already taken.

Transition

This renaming could be done with a smooth transition using @deprecated.
In the v3.x, both builder and the new name could be on the widget.
builder would then be marked as @deprecated.

On the other hand, the other listed changes (fusing *Provider & *ProxyProvider or the Consumer shorthand) would not be part of the v3 but instead the v4 – which will also include loading.

Thoughts?

Feel free to 👍 or 👎 if you agree/disagree, or comment to make suggestions!

@RabbitKabong

This comment has been minimized.

Copy link

@RabbitKabong RabbitKabong commented Nov 12, 2019

initialValue lines up style-wise with the rest of the Flutter-verse (Stream/FutureBuilder.initialData, etc.). I like it.

@filiph

This comment has been minimized.

Copy link
Collaborator

@filiph filiph commented Nov 12, 2019

Good thinking. I'm generally in favor, with the following suggestions / comments:

  • I vote for keeping ProxyProviderN, to limit confusion. The Proxy* prefix is not great, but I, for one, can't think of any better. The important thing is that it clearly identifies this as a separate idea, and it's searchable (example).
  • I vote for Provider.initialValue.
  • It's not obvious what the following does and why it's called value. It seems like it would be easy to get lost in the value/initialValue/builder/... menu of options. I would avoid this.
ChangeNotifierProvider(
  initialValue: (_) => MyNotifier(),
  value: (context, notifier) {
    // Works like *ProxyProvider.builder
    // we can safely call `Provider.of` here
    return notifier
      ..foo = Provider.of<Foo>(context)
  },
);
  • I suggest ProxyProviderN.valueUpdater (or similar) instead of ProxyProviderN.builder. That gives users a nice pair of initalValue / valueUpdater.
  • --One issue I've repeatedly had in recent months is that I sometimes I'd love to update the value of a ChangeNotifierProvider without triggering rebuilds downstream. The solution for me was to just avoid ChangeNotifiers and instead have a more custom object provided simply with Provider. I'm not sure if this is in scope of this RFC — ignore if not. Just thought it's an interesting case that might inform naming choices.-- Moved to #260.
  • In general, I don't like renames, but this seems like a worthy cause.
  • This might be a great target for an auto-refactor tool. Not saying that Remi should author it, of course. Just throwing it out there. If I have time, I would love to try to tackle this myself, tbh.
@radzish

This comment has been minimized.

Copy link

@radzish radzish commented Nov 12, 2019

I vote for anything that accepts function should be *builder, not just value/initialValue.

@rrousselGit

This comment has been minimized.

Copy link
Owner Author

@rrousselGit rrousselGit commented Nov 12, 2019

@filiph Why would you say "In general, I don't like renames, but this seems like a worthy cause" but also suggest to not fuse *Provider and *ProxyProvider?

It is for the last suggestion of having syntax sugar for the Provider+Consumer, or something unrelated?

As for:

One issue I've repeatedly had in recent months is that I sometimes I'd love to update the value of a ChangeNotifierProvider without triggering rebuilds downstream. The solution for me was to just avoid ChangeNotifiers and instead have a more custom object provided simply with Provider. I'm not sure if this is in scope of this RFC — ignore if not. Just thought it's an interesting case that might inform naming choices.

Do you mind opening a new issue about that? I've seen a few other complaints about something similar, but have yet to fully understand the problem

@rrousselGit

This comment has been minimized.

Copy link
Owner Author

@rrousselGit rrousselGit commented Nov 12, 2019

Also, on Twitter someone suggested, "value" instead of "initialValue".

This would assume that we don't merge Provider and its Proxy variant, and I find it a bit confusing personally.
But I'll leave it there just in case others like the idea

@MaikuB

This comment has been minimized.

Copy link

@MaikuB MaikuB commented Nov 12, 2019

initialValue may still be a bit misleading as one might think the value changes since it has "initial" as part of the parameter's name, unless here scenarios where it can happen that I may have missed). value would be better IMO. It aligns better with how provider has been explained to work. Documentation talks about exposing a value and reading a value. The Provider and Consumer widgets make it easy to explain to others that your code that the value of data is being provided that could be consumed further down the widget tree. If you look at the Consumer API (https://pub.dev/documentation/provider/latest/provider/Consumer/Consumer.html), that value is retrieved from the builder property. The builder property itself is a function that passes the value through a parameter named value. Using value instead of initialValue would allow for alignment between the two.

Having said that I think @radzish has a good point that if one were to see a property named value or even initialValue then one would expect the property to be assigned a value rather than referencing a function

@filiph

This comment has been minimized.

Copy link
Collaborator

@filiph filiph commented Nov 12, 2019

@filiph Why would you say "In general, I don't like renames, but this seems like a worthy cause" but also suggest to not fuse *Provider and *ProxyProvider?

Sorry, I should have been clearer. I like the builder -> initialValue* rename. That to me is worthy, because it, in my opinion, makes it more clear what the parameter is. I'm not in favor of removing Proxy, though, because it (imho) makes it less clear what the widget is.

I will open a separate issue for the side comment.

@filiph

This comment has been minimized.

Copy link
Collaborator

@filiph filiph commented Nov 12, 2019

As for builder for function parameters — I understand the sentiment. I don't personally find it important enough to warrant the verboseness of initialValueBuilder — the type (and the IDE) gives me enough signals to understand this is not, in fact, the value itself. But yeah, strictly speaking, it is a builder.

@rrousselGit

This comment has been minimized.

Copy link
Owner Author

@rrousselGit rrousselGit commented Nov 12, 2019

For the "value" suggestion, I find it pretty confusing.

It's hard to differentiate between:

Provider(value: (_) => 42);

And:

Provider.value(value: 42);
@passsy

This comment has been minimized.

Copy link

@passsy passsy commented Nov 12, 2019

I'd like to propose create as an alternative name because it implies being called only once:

Provider<int>(
  create: (context) => 42,
);
ChangeNotifierProvider(
  create: (_) => MyNotifier(),
);

spawn might be another alternative. But I personally would vote for create.

@rrousselGit

This comment has been minimized.

Copy link
Owner Author

@rrousselGit rrousselGit commented Nov 12, 2019

"create" is quite interesting, combined with potentially "update".

We'd have:

ProxyProvider<Dep, Res>(
  create: (context) => 42,
  update: (context, dep, res) => res,
  dispose: (context, res) {},
)
@rrousselGit

This comment has been minimized.

Copy link
Owner Author

@rrousselGit rrousselGit commented Nov 12, 2019

Another alternative is "initValue".

This matches with "initState" and is clearer in the sense that it's not the value but a function that builds the value.

@felangel

This comment has been minimized.

Copy link

@felangel felangel commented Nov 13, 2019

I agree with @passsy that create or initValue makes it more clear that the method will only be invoked once. I have seen many developers have trouble understanding why builder is not called when the parent widget is rebuilt.

@searchy2

This comment has been minimized.

Copy link

@searchy2 searchy2 commented Nov 13, 2019

Amazing proposal! The builder parameter was a bit confusing for me coming from StreamBuilder.

I like the proposed attribute name of initialValue but please consider initialData as an option. StreamBuilder uses initialData so it would make sense as a drop in replacement.

Personally, I like initialValue the best though.

@hawkbee1

This comment has been minimized.

Copy link

@hawkbee1 hawkbee1 commented Nov 13, 2019

initialValue, create... I feel there's something fishy with "create" and would feel better with initiate despite being just a synonym.

@creativecreatorormaybenot

This comment has been minimized.

Copy link

@creativecreatorormaybenot creativecreatorormaybenot commented Nov 13, 2019

I think it should actually be init resembling initState as dispose already matches the state name.

Provider<Foo>(
  init: (context) => Foo()..init(context),
  dispose: (context, foo) => foo.dispose(context),
)

Edit

Just saw:

Another alternative is "initValue".

This matches with "initState" and is clearer in the sense that it's not the value but a function that builds the value.

Still think that omitting the "Value" suffix is better. For some reason "value" sounds like something that would only exist in a StreamBuilder et al., i.e. being part of some kind of data.

@rrousselGit

This comment has been minimized.

Copy link
Owner Author

@rrousselGit rrousselGit commented Nov 13, 2019

^ Agreed

Be it "init" or "create", having a verb may be better. It matches with dispose, and it's not like "xxValue" brings any extra useful information – "value" doesn't mean much.

I still like "create" more though. It's closer to what the function actually does and feels overall more natural IMO.
For example, we say CRUD, not IRUD.

@creativecreatorormaybenot

This comment has been minimized.

Copy link

@creativecreatorormaybenot creativecreatorormaybenot commented Nov 13, 2019

@rrousselGit It initializes the value or not? It is a variable assignment and an initialization of that variable, right?

I guess what you mean is that init would refer to the whole widget contrary to initValue.

@rrousselGit

This comment has been minimized.

Copy link
Owner Author

@rrousselGit rrousselGit commented Nov 13, 2019

For ChangeNotifierProxyProvider, it's usually an uninitialized creation.
The initialization is usually performed inside "builder":

ChangeNotifierProxyProvider<...>(
  initialBuilder: (_) => Foo(),
  builder: (_, value, foo) => foo..init(value),
)
@rrousselGit

This comment has been minimized.

Copy link
Owner Author

@rrousselGit rrousselGit commented Nov 13, 2019

There's surprisingly a lot of votes on tweeter to keep things as they are.
On the other hand, there's nobody complaining on this issue.

I'm not too sure what to do with that.

https://twitter.com/remi_rousselet/status/1194361858939117568?s=09

@creativecreatorormaybenot

This comment has been minimized.

Copy link

@creativecreatorormaybenot creativecreatorormaybenot commented Nov 13, 2019

@rrousselGit Well, I think that some of the options proposed here are better and you cannot expect people to select other (comment) when they are not presented with a better option.

@mdrideout

This comment has been minimized.

Copy link

@mdrideout mdrideout commented Nov 13, 2019

I like what's being discussed here much more than the twitter comments. After reading this, I really like "create"

@filiph

This comment has been minimized.

Copy link
Collaborator

@filiph filiph commented Nov 14, 2019

I think the general developer community will always be less likely to welcome change. For most existing users, it's just more work. They just want their apps to work. Plus there's the usual change aversion and churn (tutorials will get out of date, etc.), with no apparent counterweight.

In contrast, people who comment on github issues tend to be insiders. Power users.

I'd say the majority of existing provider users will not welcome the change. The question is if you are willing to let them suffer a bit for the benefit of the upcoming users, and for general clarity / elegance.

In this respect, it makes a lot of difference how easy it is to upgrade. This change will not be easily search-replace-able (e.g. Replace in Path >> done). On the other hand, thankfully it will not be too hard to upgrade (unlike, say, upgrade to Dart 2 or NNBD). This brings back the idea of an automated Large Scale Refactoring tool. In theory it shouldn't be too hard to write...

@jeroen-meijer

This comment has been minimized.

Copy link

@jeroen-meijer jeroen-meijer commented Nov 15, 2019

I think I'd prefer init, initialBuilder or something similar over initialValue. initialValue doesn't sound like it would take in a function but just a value (like StreamBuilder.initialValue).

@rrousselGit

This comment has been minimized.

Copy link
Owner Author

@rrousselGit rrousselGit commented Nov 15, 2019

Thinking about it, initialValue is already taken by StreamProvider

I guess that it's between "init" and "create".

@hawkbee1

This comment has been minimized.

Copy link

@hawkbee1 hawkbee1 commented Nov 18, 2019

init + create = initiate :D

@gloomie

This comment has been minimized.

Copy link

@gloomie gloomie commented Nov 22, 2019

Let me represent the newbies to flutter 😝

init/ initial : they both misleadingly imply that there will be a second assignment somewhen in the future.

value/initialValue : they are better names in their essence but we ideally want a verb (to match update and dispose which are rather non-negotiateable).

create : not pretty at all in the eyes of a newbie! Create as a verb means that you use some base elements to produce a resulting entity of other nature. It makes you think that some black-box magic is going to happen now, not a humble value assignment.

So, here comes the newbie proposal...

assign : it is a verb (assign, update, dispose), it doesnt imply a second assignment (as init does) and it is content-agnostic (value implies a primitive var type - for example a function is not a value but rather a methodology of assigning values). In the latter context you could even use reassign instead of update (but I prefer update as it better approaches the essence of a function here).

A second candidate triad could be: set, update, dispose. Simple, agnostic, intuitive.

Thanks for your patience 🤓

@creativecreatorormaybenot

This comment has been minimized.

Copy link

@creativecreatorormaybenot creativecreatorormaybenot commented Nov 23, 2019

init/ initial : they both misleadingly imply that there will be a second assignment somewhen in the future.

Disagree, see below reasoning.

assign : it is a verb (assign, update, dispose), it doesnt imply a second assignment (as init does) and it is content-agnostic (value implies a primitive var type - for example a function is not a value but rather a methodology of assigning values). In the latter context you could even use reassign instead of update (but I prefer update as it better approaches the essence of a function here).

Disagree because assign is something that happens all the time. In programming, a declaration and initialization happen once, however, assignment can happen as often as you want.


So someone having a programming background should understand the concept of init, however, assign would be misleading to them.

@gloomie

This comment has been minimized.

Copy link

@gloomie gloomie commented Nov 23, 2019

My understanding is that the constructor needs a name that at the same time:

  1. gives the meaning of a single and final assignment (in the case of a simple Provider)
  2. gives the meaning of a first assignment that will be later updated according to an attached function (in the case of a ProxyProvider)

'init' doesn't fully comply with case 1 as it carries meaning about implied future changes (which are however not going to happen in this case), whereas 'assign' and 'set' are both mutation-agnostic (they may or may not be changed later on).

@rrousselGit rrousselGit mentioned this issue Nov 26, 2019
@rrousselGit

This comment has been minimized.

Copy link
Owner Author

@rrousselGit rrousselGit commented Nov 26, 2019

Thanks for your input everyone!

I'm going forward with create & update and will soon publish a 3.2.0 that deprecates the previous names

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.