Bloc: Close instances of `dart.core.Sink`.

[zeucxb]

Flutter bloc examples are showing info message:

Close instances of dart.core.Sink.

Do we need to call .close() somewhere?

[felangel]

Hi @zeucxb 👋
Thanks for opening an issue!

This is a false positive in the dart linter https://github.com/dart-lang/linter/issues/1381.
You can either directly use BlocProvider.of<ThemeBloc>(context)rather than creating a variable or you can disable the lint rule until the dart linter issue is fixed. I'll refactor the example to eliminate the warning.

Hope that helps 👍

[zeucxb]

Thank you @felangel

[felangel]

I've updated the example in https://github.com/felangel/bloc/commit/fc211bbcca4770f56a59d1efa56af6850a9d52f1. It should behave the same way but without triggering the linter warning. Also if everyone could give the dart linter issue a thumbsup it might help speed up the fix 🙏

[larssn]

Alternatively, you can create a file called analysis_options.yaml in the root of your project.

The flutter team uses the following rules, which might serve as inspiration: https://github.com/flutter/flutter/blob/master/analysis_options.yaml

Alternatively, you can leave it blank, to disable most of the lints.

Note that they have disabled the close_sinks rule as being "not reliable enough". 😁

[tapizquent]

Will calling .close() on the sink, even if it comes from BlocProvider cause any sort of unintended behavior?

[felangel]

@tapizquent not sure I understand the question but you should be calling close on the sink at some point. In most cases, it is recommended that you leave it up to BlocProvider but in some rare cases you might want to manage the bloc yourself via a StatefulWidget in which case you should manually call close on the bloc in the dispose override of your State.

[tapizquent]

@felangel I mean, in this situation when you are using variable to store BlocProvider.of<>.., is it bad that I call close on it even though I know BlocProvider takes care of it? just so that I don't get the warning

[felangel]

@tapizquent you should not call close manually when BlocProvider takes care of it because as soon as you call close the bloc will stop processing new events.

[AAAbramenko]

I solved this in the following way:

//ignore: close_sinks
          final SettingsBloc settingsBloc =
              BlocProvider.of<SettingsBloc>(context);

[rrousselGit]

The lint rule can hardly be fixed. The way bloc close the sink is hardly understandable statically.

IMO there's two solutions:

• don't implement Sink (it doesn't bring anything anyway)
• disable the "close_sinks" rule in your analysis_options.yaml

[agordeev]

disable the "close_sinks" rule in your analysis_options.yaml

I suppose this is not a good option. That linter check is there for a good reason.

[dustin-graham]

@rrousselGit brings a good point. Implementing Sink seemed like a curious change. It caused us to refactor a lot of code (dispatch to add), gave us this warning, and also as a result it made the code slightly less easy to understand for the uninitiated as "add" does not seem to clearly articulate what is happening as well as "dispatch" did. When dealing with a Sink, the semantics of "add this event to the sink" makes sense in the same way that "add this item to a collection" makes sense. And this is usually communicated well when you name your sink something like "mySink" and you call "mySink.add". But when we have a Bloc, we name it "myBloc" and call "myBloc.add" and the meaning is obscure. @felangel I'm interested to understand what the move to Sink brings to the table, but we love the library and appreciate all your efforts!

[felangel]

@dustin-graham we decided to implement Sink in order to further align bloc with the built-in Dart APIs which with developers are familiar in order to make the transition from managing your own streams/sinks to using the bloc library easier/more intuitive. While it definitely takes some getting used to, in my opinion myBloc.add(MyEvent()) reads a lot better because you're literally adding events to a queue which are being processed and later output as states. I want to spend some time over the next few weeks to revamp parts of the docs to incorporate better diagrams (maybe even some gifs) in order to illustrate how a bloc works. I wouldn't worry about the warning because it's clearly a bug in the dart linter; I would recommend disabling the close_sinks rule altogether until it's fixed as @rrousselGit mentioned. I really appreciate the feedback and am looking forward to hearing your thoughts, thanks! 🙏

[dustin-graham]

Thanks Felix. Since a Bloc is a Sink It’s certainly correct. I don’t feel
any confusion over it since I understand how it works. However, I wonder if
it would be just as well if the Sink and Stream semantics remained
basically as implementation details. The only time I will consume a bloc is
with a BlocBuilder which gives me a builder function. I’m not using a
StreamBuilder myself or listening to the stream explicitly in my own code.
Knowing that a Bloc is a stream hasn’t seemed inherently useful to know in
most cases as I typically won’t operate on my Bloc directly as I would any
other streams in my app. My usage of those Blocs is encapsulated by
flutter_bloc nicely. In the instances when I need my Bloc to process an
event I typically just get a reference to my bloc and give it my event.
Whether the Bloc is the Sink itself or if it has a Sink internally does not
affect my usage of the Bloc. Trying not to consider what I know about
Sinks, and trying instead to only think about my own business logic which
the Bloc manages, “add” seems to make less sense to me than “dispatch” did.
To understand why add makes more sense you’ve got to know about Sink, and
knowing about Sinks probably isn’t a strict requirement to using Bloc.

All of this is just to say that I don’t yet see a benefit of having the
Bloc be the Sink itself. And that is only important because I’d like to
avoid the warning in cases of accessing Bloc instances and I’d prefer not
to turn off that warning as it is a legitimate check for other typical uses
of Sink in other places in the app. I imagine that it may be difficult for
the linter to figure out that this is not a problem as the sink is getting
closed elsewhere. But hopefully there’s a way for them to do it.

All that said, this is minor to me. I really appreciate the solid work you
guys do. Keep it up!

On Fri, Dec 6, 2019 at 12:13 PM Felix Angelov [e-mail]
wrote:

@dustin-graham https://github.com/dustin-graham we decided to implement
Sink in order to further align bloc with the built-in Dart APIs which
with developers are familiar in order to make the transition from managing
your own streams/sinks to using the bloc library easier/more intuitive.
While it definitely takes some getting used to, in my opinion
myBloc.add(MyEvent()) reads a lot better because you're literally adding
events to a queue which are being processed and later output as states. I
want to spend some time over the next few weeks to revamp parts of the docs
to incorporate better diagrams (maybe even some gifs) in order to
illustrate how a bloc works. I wouldn't worry about the warning because
it's clearly a bug in the dart linter; I would recommend disabling the
close_sinks rule altogether until it's fixed as @rrousselGit
https://github.com/rrousselGit mentioned. I really appreciate the
feedback and am looking forward to hearing your thoughts, Thanks! 🙏

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/felangel/bloc/issues/587?email_source=notifications&email_token=AAHKLO55SFSYFT4GTJ45KJTQXKP63A5CNFSM4JCLIE7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGFCIFI#issuecomment-562701333,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAHKLO437V662LGBHGL5L2DQXKP63ANCNFSM4JCLIE7A
.

[felangel]

@dustin-graham you definitely make some good points! While I agree that Stream/Sink semantics could just be implementation details for common use-cases, there are valid cases when you may want to perform transformations on the data transformEvents/transformStates or listen directly to a bloc's stream. In those cases, it benefits users to understand how the bloc works (at a high level). Furthermore, a lot of these changes were done in an effort to conform to existing standards in an attempt to make it clear that the bloc library does in fact conform to the BLoC design pattern. I agree that the Dart linter warning is annoying and I'm going to try to see if I can directly contribute to address the issue over the next few days but the current implementation of the lint rule has many bugs (so many that the rule is disabled within flutter itself). I really value your feedback and hope we can work to address this issue as quickly as possible. Thanks again! 😄

[dustin-graham]

Of course. However the request is to find a way not to have to do that.
Which I understand depends on the platform at this point.

On Tue, Feb 25, 2020, 2:39 AM Avi Cohen [e-mail] wrote:

You can add an ignore remark above that line like this:

// ignore: close_sinks
final themeBloc = BlocProvider.of(context);

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/felangel/bloc/issues/587?email_source=notifications&email_token=AAHKLO3FLMP35G5JARZKFPLRETRNNA5CNFSM4JCLIE7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM3H62Q#issuecomment-590774122,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAHKLOYILDVAFLAUVSQP75TRETRNNANCNFSM4JCLIE7A
.

[felangel]

closing this since the issue is being tracked via https://github.com/dart-lang/linter/issues/1381