fluttercommunity / dart_sealed_unions

Sealed Unions for Dart. Maintainer: @nodinosaur
https://pub.dev/packages/sealed_unions
Apache License 2.0
66 stars 7 forks source link

Allow to filter which sealed unions you are listening #12

Open juliocbcotta opened 5 years ago

juliocbcotta commented 5 years ago

I know this is particular to flutter, but maybe it makes sense to you too. Currently, we are able to call .join to listen all states, but sometimes we need a special handling for just one or two states. For instance the code below. I had to use multiple empty closures to listen to the error state and be able to show a SnackBar. Sadly, in flutter, we can't just return a SnackBar to the view tree. The same for Alerts and to navigation.

I see two options here. to create siblings methods to .join to allow filtering of the current state or to expose the current state type being emitted. I know this library is meant to allow a centralized state management, but it looks like that it is not always possible in Flutter.

body: BlocListener<LoginBloc, LoginState>(
            listener: (context, state) {
              state.join((_) {}, (_) {}, (_) {}, (error) {
                _scaffoldKey.currentState.showSnackBar(
                  SnackBar(
                    content: Text('Fuuuuuuuuuuuu $error'),
                  ),
                );
              });
            },
            child: BlocBuilder<LoginBloc, LoginState>(
              builder: (context, state) {
                return Stack(
                  children: <Widget>[
                    _buildLoginForm(),
                    state.join(
                        (initial) => Container(),
                        (loading) => Positioned.fill(
                            child: Center(child: CircularProgressIndicator())),
                        (logged) => Container(),
                        (error) => Container()),
                  ],
                );
              },
            ),
          )

Am I missing something? Is there any other way to handle this? Thanks.

cc: @felangel , maybe you have some input to this.

Edit: Simplified code.

nodinosaur commented 5 years ago

Hi, I understand that you might see this as a useful tool, but the approach would be an anti-pattern.

By implementing an only listen to these scenario to work around this mechanism would defeat the purpose of this library and leave your code open to error.

What Sealed Unions gives you is a Closed Inheritance, that means that you are only limited to all the inheritance types that are the defined in this file and none else which means that when the compiler goes to them it knows exactly that there has to be four different ones and that gives you some guarantees and that is good for development.

The purpose of using this type of library is so that you are assured that you have in-fact covered all possible cases.

juliocbcotta commented 5 years ago

Well, then... Is this the wrong tool to my needs in the sample above? Because it does not look right having to place all those empty closures.

The real problem I see is that I don't have access to which Union I received without using join method. Being able to match all possible child classes is great, but should we be limited by that?

andrewackerman commented 4 years ago

It may be an anti-pattern to sealed classes, but there are still legitimate use cases for this. Another example might be an event bus listener that only listens for particular types of events. Having to provide a default implementation for every other kind of event just increases the already slightly annoying amount of boilerplate. As such, there's an argument for having a joinOrDefault method for when you're using sealed classes but don't need to exhaustively implement handling for each case everywhere you reference them.

antholeole commented 3 years ago

Did the developers of this package come to a conclusion on this? Based on the non-action, it seems like developers went the purist route, but I'm not certain because the issue isn't closed.

If we're open to a PR for this I would be happy to look into it - I have a union5 with a listener that looks like this:

BlocListener<MyBloc, MyState>(
   listener: (_, state) => state.join(
       (_) => null,
       (_) => null,
       (_) => doSomething(),
       (_) => null,
       (_) => null,
   )
)

another unfortunate side effect hold the boilerplate is that it's crushing my test coverage - that's 4 uncovered paths that don't do anything at all.

If the maintainers are open to a PR for something like joinOrDefault or otherwise, then let me know. Even in the inspiration language Rust, you can specify a default argument as opposed to needing to explicitly handle all states.

cc @nodinosaur - if you're open to a PR let me know!