Bloc: [Proposal] Remove dispose from BlocProvider API

[felangel]

UPDATE 2
To provide an existing bloc without automatic disposal:

BlocProvider.fromContext<MyBloc>(
  child: MyChild(),
)

It would be equivalent to the following:

BlocProvider<MyBloc(
  builder: (context) => BlocProvider.of<MyBloc>(context),
  dispose: false,
  child: MyChild(),
)

To provide a new bloc with automatic disposal:

BlocProvider.create<MyBloc>(
  builder: (context) => MyBloc(),
  child: MyChild(),
)

---

UPDATE 1: After some discussions we've come up with the following. Thoughts?

To provide an existing bloc without automatic disposal:

BlocProvider.findExisting<MyBloc>(
  child: MyChild(),
)

To provide a new bloc with automatic disposal:

BlocProvider.create<MyBloc>(
  builder: (context) => MyBloc(),
  child: MyChild(),
)

---

Is your feature request related to a problem? Please describe.
It's a bit annoying/error prone to have to remember to set dispose to false in cases where a BlocProvider is used to provide an existing bloc to a new page route.

In the normal case a bloc would be provided like so:

BlocProvider(
  builder: (context) => MyBloc(),
  child: MyChild(),
)

In cases where an existing bloc is provided, we need to do:

BlocProvider(
  builder: (context) => MyBloc(),
  dispose: false,
  child: MyChild(),
)

Describe the solution you'd like
It might be better to have two options.

1. Provide a new bloc which is automatically disposed

BlocProvider(
  builder: (context) => MyBloc(),
  child: MyChild(),
)

1. Provide an existing bloc which is not automatically disposed

BlocProvider(
  bloc: BlocProvider.of<MyBloc>(context),
  child: MyChild(),
)

This would eliminate the need for the dispose property.

Please give this a 👍 if you like the proposal or a 👎 if you dislike the proposal.

If you hit dislike it would be awesome if you could provide some feedback/justification, thanks!

[axellebot]

What about if I'm using dispose when I'm using a bloc only for 1 specific widget.

For example :

class ProjectPage extends StatefulWidget {
  final String projectUuid;

  ProjectPage({
    Key key,
    @required this.projectUuid,
  })  : assert(projectUuid != null, 'No project id given'),
        super(key: key);

  @override
  _ProjectPageState createState() => _ProjectPageState();
}

class _ProjectPageState extends State<ProjectPage> {
  ProjectBloc _projectBloc;

  @override
  void initState() {
    super.initState();

    XRepository xRepository=
        Provider.of<XRepository>(context, listen: false);

    _projectBloc = ProjectBloc(
      xRepository: xRepository,
    );

    _projectBloc.dispatch(ProjectSelected(
      projectUuid: widget.projectUuid,,
    ));
  }

  @override
  void dispose() {
    _projectBloc.dispose();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
      /*omitted*/
  }  
}

BTW BlocProvider bloc property miss me when I want to fully control the life cycle of the bloc because of Bloc creation needs (e.g. injecting other bloc).

[felangel]

@axellebot you can just replace that with:

class ProjectPage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
      final projectBloc = BlocProvider.of<ProjectBloc>(context);
      /*omitted*/
  }  
}

Then you can just wrap the ProjectPage in BlocProvider like so:

Widget build(context) {
    return BlocProvider(
      builder: (context) => ProjectBloc()..dispatch(
          ProjectSelected(projectUuid: widget.projectUuid),
      ),
      child: ProjectPage(),
    );
}

Does that address your concern?

[axellebot]

@felangel Sorry I miss leaded you. My ProjectBloc is only used from the build method of this ProjectPage. It's only a BLoC only for a local use.

[felangel]

@axellebot that's fine because with the above snippet it's still scoped to only the ProjectPage.

[axellebot]

@felangel I see what you mean. The think is that I don't want to wrap my ProjectPage with a BlocProvider and I also don't want to provide this ProjectBloc to any children. I only want the ProjectBloc to work along ProjectPage.

[warriorCoder]

@axellebot Your page doesn't have any subwidgets? That might be a design mistake as you want to break your pages/widgets up into smaller bite-sized pieces. As your app grows it makes for really easy changes etc. So really it's better to be able to provide the bloc down the tree to the sub-nodes.

[axellebot]

@warriorCoder You wrongly understood : I DON'T want to provide the bloc the children.

[warriorCoder]

@felangel What about BlocProvider.useExisting instead of .findExisting because the BlocProvider isn't really finding anything and most likely the bloc provided would be done with BlocProvider.of<MyBloc>(context) anyway.

[felangel]

@warriorCoder I'm fine with useExisting. What do others think?

[axellebot]

Don't get the purpose of BlocProvider.findExisting, where the Bloc will come from ? Another BlocProvider, right ? Then why creating a new BlocProvider ?

[felangel]

@axellebot if you want to provide an existing bloc to a new page route you need to wrap the route in a new BlocProvider. You can check out the bloc access recipe for more details.

[warriorCoder]

Personally, I feel like the .fromExisting is very clear about where the bloc is coming from. I feel like the .fromContext muddies the water a bit because it is very possible to provide a bloc that wasn't passed with a context but created outside said context. With the former, it doesn't matter where the bloc was created and how it's being passed it already was and isn't being created.
My 1c.

[warriorCoder]

I'd like to also add that if you are doing anything with blocs and modals or sheets then you also need to pass the bloc to the modal/sheet. The reason is that Flutter sticks the new sheet into an overlay which gets added at the top of the widget tree.

[axellebot]

Why not keeping the old constructor ?

And create something like that :

BlocProvider({
    Key key,
    this.bloc,
    this.builder,
    this.child,
    this.dispose = true,
  }) : assert(bloc != null && builder == null),
       assert(bloc == null && builder != null),
       super(key: key);

  BlocProvider.useExistent<T extends Bloc>({
    Key key,
    this.child,
  }) : dispose = false,
       bloc = BlocProvider.of<T>(context),
       super(key: key);

[felangel]

@axellebot I would argue it's still confusing to people if they see bloc and builder as both being optional in a BlocProvider. I personally feel like we should try to make it hard to accidentally do something wrong and force developers to be intentional with the code they write.

[axellebot]

@felangel Yeah I do understand but most of classes that provide named constructor of widget (from my experience) are also providing the basic constructor (e.g. Image widget ). And I think developer can also RTFM or just read error like they already do with the Flutter SDK for the same purpose.

Developers can used the leaded constructor (with named constructor) or they can fully customize the widget with the basic constructor. My developer experience is better with this kind of "guidelines" and I think the lib should provide the same experience.

This developer experience is why I love flutter SDK and it should be the same with flutter libs.

EDIT :
The BlocProvider name is self explaining what it does. It provide a Bloc, if you check its properties you can see builder or bloc, by using this widget it makes sense and seems clear (for me) about the use of these properties. What will be the purpose of the builder property if it's not to build the Bloc ...
Or maybe the confusion is about dispose behavior ? But it will still be the same with named constructors.

[felangel]

This will potentially be addressed by #385 if everyone is okay with aligning with the provider api.

BlocProvider + New Bloc (automatic disposal)

BlocProvider<MyBloc>(
  builder: (context) => MyBloc(),
  child: MyChild(),
),

BlocProvider + Existing Bloc (no automatic disposal)

BlocProvider<MyBloc>.value(
  value: _myBloc,
  child: MyChild(),
),

Thoughts?

[axellebot]

@felangel It now looks like the provider package but it seems to be the best way to name it and to also avoiding confusion. (also if both packages are used) 👌

I will keep this name for a named constructor in my mind 👍

[felangel]

Closing since it has been addressed by #385 and will be included in flutter_bloc v0.19.0