flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
163.65k stars 26.91k forks source link

[Discussion] Scope spanning multiple screens #30062

Open Norbert515 opened 5 years ago

Norbert515 commented 5 years ago

Having a way to manage scopes in Flutter. image (From https://proandroiddev.com/dagger-2-part-ii-custom-scopes-component-dependencies-subcomponents-697c1fa1cfc)

An example:

The login is implemented using multiple pages (which are managed by the Navigator). Each login page wants to access the LoginBloc, once the login is completed the LoginBloc is no longer needed and should be disposed.

Possible implementations right now:

  1. Nested navigators

This solves the issue as each Navigator can hold the necessary state, though this approach introduces quite a few problems because Navigators are not really intedned to be nested this way.

  1. Having the Bloc above the Navigator and only exposing them to the necessary routes

Two problems with this - nesting these dependencies can become pretty cumbersome because each route needs to be explicitly wrapped in all of the used dependencies (with dependencies I mean some kind of InheritedWidget which exposes the actual data).

And, even though other routes have no access to the data, the data is not disposed, which leads to a memory leak and leaves the bloc in an "undifined" state.

(Bloc in these examples could be replaced with any kind of business logic/ data storage)

The main problem is that each route is completely separate and there doesn't seem to be an easy way to accomplish this.

I was wondering if this is anything that should be in the framework or should be provided as a separate package.

I've talked to @rrousselGit about this, he had a few ieads how this could be solved but we'd like to hear other thoughts.

rrousselGit commented 5 years ago

My idea about this issue is to create a new kind of widget:

A mixture between an InheritedWidget and a StatefulWidget It's an InheritedWidget with a state (but no build method).

The principle is that the state of our new widget is created only when there's at least one listener.

This means a few things:


Such widget would allow us to have the following architecture:

class _Foo extends InheritedStatefulWidget {
  _Foo({Widget child}): super(child: child);

  _FooState createState() => _FooState();
}

class _FooState extends InheritedState<_Foo> {
 Foo foo;

  initState() {
    foo = Foo();
  }
}

// ...

_Foo(
  child: MaterialApp(
    routes: {
      '/': (context) {
        final fooState = context.inheritFromStateOfExactType(_FooState) as _FooState;
        return Provider.value(value: fooState.foo, child: Home());
      },
      '/settings': (context) => Settings(),
    },
  ),
)

In such configuration, only the route / can read the current state through Provider<Foo>.of(context).

And if we go to /settings with a pushReplacement or by disabling maintainState on Route, then this would dispose _FooState

Note that I used a new method on BuildContext. It is not the same as for using InheritedWidgets.

Hixie commented 5 years ago

The usual solution to this is to have the InheritedWidget provide an object that can be subscribed to, and for the places that need to "enable" the feature to get that object and subscribe to it while they're up.

rrousselGit commented 5 years ago

The usual solution to this is to have the InheritedWidget provide an object that can be subscribed to, and for the places that need to "enable" the feature to get that object and subscribe to it while they're up.

That sounds very contradictory to all existing Flutter state management architectures.

Whether they provide an immutable object (redux, flutter_bloc), a mutable one (mobx, bloc), or a mix of both (provider), none of them do that.

That makes things needlessly complicated in most cases, as it involves nesting two builders when not everybody cares about this feature.

abstract class Bloc {
  Stream<int> get counter;
}

class Foo extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    final Stream<Bloc> blocStream = Provider.of(context);

    return StreamBuilder<Bloc>(
      stream: blocStream,
      builder: (_, snapshot) {
        final bloc = snapshot.data;
        return StreamBuilder(
          stream: bloc.counter,
          builder: (_, snapshot) {
            return Text(snapshot.data.toString());
          },
        );
      },
    );
  }
}
escamoteur commented 4 years ago

I guess after the birth of provider I can close this here

rrousselGit commented 4 years ago

Provider does not solve this

escamoteur commented 4 years ago

Why wasn't there any more conversation since then?

rrousselGit commented 4 years ago

I've been doing some work on it, like #33213, although it's rejected

That's something I'd like provider to solve in the future. I have some ideas that don't require changes to the framework in mind, but I'm not there yet

Hixie commented 4 years ago

I don't really understand why this is a problem. As I say in the comment above, the screens that need access to the common object can get the common object from the application's data model. It doesn't really seem to matter if they're widgets in the same route, different routes, etc.

The important thing to remember is that application state shouldn't be in the widget tree, what should be in the widget tree is just references to the application state.

rrousselGit commented 4 years ago

Routes do not matter here, this is more of a common example.

What matters is: "Is the UI using a piece of information or not?", such that we can pause/dispose of expensive resources, or create them on demand.

The important thing to remember is that application state shouldn't be in the widget tree, what should be in the widget tree is just references to the application state.

That's not what most, if not all state management existing solutions do. It is very convenient to create the application state in the widget tree because of the widget declarativeness. For example, by doing so the act of creating, updating and disposing of the resources becomes automatic.

What are the benefits of not doing so?

Hixie commented 3 years ago

One benefit is you don't have this bug. Another is it allows for easy reuse of widgets (e.g. you can hoist your entire app into a widget of a bigger app and everything still works), or abstraction (e.g. moving logic from one app to another, or having the same logic on the server and the client, or in a Flutter app and a non-Flutter app, like a command line or non-Flutter Web app). Also Widgets are supposed to be very fast and nimble objects that deal with UI; while the app state is where the business logic is and would exist independent of the UI.

Flutter was designed to have app logic outside the Widget tree. We didn't do a good job of conveying that at the start of Flutter's existence because we were focused on UI and didn't think to explain how to write apps, but there we go. It's why we recommend Provider -- it lets you inject the app state from your app business logic library into your widget hierarchy such that the UI widgets can communicate with the business logic.

rrousselGit commented 3 years ago

It's why we recommend Provider

Provider is the main target of this issue though

rrousselGit commented 3 years ago

More specifically, with provider we would want this test to pass:

class Counter extends ChangeNotifier {
  int _count = 0;
  int get count => _count;

  void increment() {
    _count++;
    notifyListeners();
  }
}

testWidgets('destroys objects when no-longer used', (tester) async {
  await tester.pumpWidget(
    MaterialApp(
      home: Scaffold(
        body: ChangeNotifierProvider<Counter>(
          autoDispose: true,
          create: (context) => Counter(),
          builder: (context, child) {
            final Counter counter = context.watch<Counter>();
            return RaisedButton(
              onPressed: counter.increment,
              child: Text('${counter.count}'),
            );
          },
        ),
      ),
    ),
  );

  expect(find.text('0'), findsOneWidget);
  expect(find.text('1'), findsNothing);

  await tester.tap(find.byType(RaisedButton));
  await tester.pump();

  expect(find.text('1'), findsOneWidget);
  expect(find.text('0'), findsNothing);

  await tester.pumpWidget(
    MaterialApp(
      home: Scaffold(
        body: ChangeNotifierProvider<Counter>(
          autoDispose: true,
          create: (context) => Counter(),
          child: Container(),
        ),
      ),
    ),
  );

  await tester.pumpWidget(
    MaterialApp(
      home: Scaffold(
        body: ChangeNotifierProvider<Counter>(
          autoDispose: true,
          create: (context) => Counter(),
          builder: (context, child) {
            final Counter counter = context.watch<Counter>();
            return Text('${counter.count}');
          },
        ),
      ),
    ),
  );

  expect(find.text('0'), findsOneWidget);
  expect(find.text('1'), findsNothing);
});

Such behavior is currently impossible to implement because of this issue

The logic is still extracted from the widget tree as you mentioned. That is standard Provider code. But still, we can't implement such a feature.

Hixie commented 3 years ago

I don't understand. Why is that impossible?

rrousselGit commented 3 years ago

There is no way to know when an InheritedWidget is no-longer used.

That's why I opened https://github.com/flutter/flutter/pull/33213 a long time ago.

The discussion around this PR was that you recommended using Builders instead. But that means we can't do context.watch<MyObject>() anymore, and need a ValueListenableBuilder/... – which circles back to #51752 about Builders being unreadable because of nesting.

Hixie commented 3 years ago

I don't see why that matters here. Instead of replacing the builder with a child that doesn't use the counter, just remove the entire ChangeNotifierProvider.

rrousselGit commented 3 years ago

The solution to "how to allow provider implement such feature" shouldn't be "remove provider".

rrousselGit commented 3 years ago

Maybe you were confused by the test. Here's the same test but using Navigator instead, to hide and show the counter:

class Counter extends ChangeNotifier {
  int _count = 0;
  int get count => _count;

  void increment() {
    _count++;
    notifyListeners();
  }
}

testWidgets('AutoDispose', (tester) async {
  await tester.pumpWidget(
    ChangeNotifierProvider(
      autoDispose: true,
      create: (_) => Counter(),
      child: MaterialApp(
        initialRoute: '/counter',
        routes: {
          '/counter': (context) {
            final counter = context.watch<Counter>();
            return Scaffold(
              appBar: AppBar(title: Text('counter')),
              body: RaisedButton(
                onPressed: counter.increment,
                child: Text('${counter.count}'),
              ),
            );
          },
        },
        home: Text('home'),
      ),
    ),
  );

  final navigator = tester.state<NavigatorState>(find.byType(Navigator));

  expect(find.text('counter'), findsOneWidget);
  expect(find.text('0'), findsOneWidget);

  await tester.tap(find.byType(RaisedButton));
  await tester.pump();

  expect(find.text('counter'), findsOneWidget);
  expect(find.text('1'), findsOneWidget);

  navigator.pop();
  await tester.pumpAndSettle();

  expect(find.text('home'), findsOneWidget);

  navigator.pushNamed('/counter');
  await tester.pumpAndSettle();

  expect(find.text('home'), findsNothing);
  expect(find.text('counter'), findsOneWidget);
  expect(find.text('0'), findsOneWidget);
});

This test is about the autoDispose flag, which destroys the state of the provider when it is still mounted but no-longer listened.

Hixie commented 3 years ago

Wrapping the Scaffold in a Builder and having the context.watch call inside the builder callback would solve that one, FWIW.

That said I don't really understand how this relates to @Norbert515's original bug description (which I don't really understand). Wouldn't each login page independently register with the app state and ask for the LoginBloc, unregistering when it goes away?

rrousselGit commented 3 years ago

This issue is about destroying an ambiant state when it is no longer in use. Then recreating it when it is used again

It relates so #51752 in that to make this work, we need a listener mechanism on our object, and keep track on whether there is a listener registered or not.

But that forces us from using a Builder. We can't simply rely on context.dependOn anymore Which means this reduces readability.

esDotDev commented 3 years ago

I believe this is the key line from the OP: "once the login is completed the LoginBloc is no longer needed and should be disposed."

They are seeking some sort of automatic teardown and setup of common-ancestor state, based on whether any descendant is currently bound to it. Register/Unregister would work, but is a manual process. They would like to not have to use a builder in order to get this automatic cleanup.

The data is not disposed, which leads to a memory leak and leaves the bloc in an "undifined" state.

So after all 3 login pages have made use of the ancestor state, which exists at the top of the tree, there is some additional manual step to get that state cleaned-up (and re-created should we ever log out again).

I don't really understand why it's an issue to have a tiny piece of memory stick around for the life of the app, but I guess if you had tons of shared Blocs it could add up?

Norbert515 commented 3 years ago

This isn't a huge issue, and manually cleaning up/ resetting works just as expected. But I felt like having implicit lifespans would feel more "fluttery" (as opposed to manually resetting and cleaning up on some event).

But right now, there is no way to achieve such a lifespan for multiple routes.

rrousselGit commented 3 years ago

Agreed

It's not critical, but the declarative nature of Flutter combined with Provider/Co could bring some simplification to this pattern

I've implemented it in Riverpod, and the feature is appreciated quite a bit. The only issue is, it forces users to either use hooks or Builders to consume Inheritedwidgets

Norbert515 commented 3 years ago

I have another example that might help understand the problem.

Let's say we have an application that has projects a user can work on, each project has multiple tasks the user can work on.

This results in 3 main screens:

(Additionally, there are screens such as CreateNewTaskScreen, etc.)

In a diagram form, it looks like this:

d (1)

The ProjectSelectionScreen then enriches that information with the fact that a specific project is selected. One way to propagate this information to the whole app is to imperatively set setCurrentlySelectedProject on the object which is provided to the whole app.

This option can be quite awkward when working with some state-management solutions such as the bloc library as it would require to nest blocs inside each other.

Another option is to provide the currently selected project to the subtree. If everything is on the same route, this works without problems but routes only have one common parent. The problem is:

If the Project currentProject is provided at the very top, then it is in an undefined state when no project is selected yet and the ProjectSelectionScreen has to imperatively set it.

d (2)

Another way, which currently isn't as easy to do as it requires nested navigators is to push a new "scope" which implicitly contains this information. Just take a look at this diagram, this should explain it well.

d (3)

This way, any screen which is in the context of a project has access to that project. Closing that project (and operations related to that such as saving, disposing of resources, etc.) is done in the dispose method of that scope.

@Hixie Does this help to clarify the original issue?

Do others have opinions about this? How do most people deal with such scoped information?

esDotDev commented 3 years ago

Normally here I would just have the selected projectId in a ChangeNotifier or something, higher up the tree. Set it, and then change routes. The new route can just look up selected project using provider or some simple service locator like GetIt.

I think this is more about where you may have a 4 page onboarding flow, that all shares some temporary state (form details, preference, things that are chosen, yet not 'saved'), you can navigate back and forth in those routes, but when they all go away, the 'parent state' is cleaned up. I don't think this is particularly easy to do in a declarative way right now, you basically have to manually manage it (init state when you enter any of those routes, cleanup when switching to any other routes) or use a nested navigator.

This option can be quite awkward when working with some state-management solutions such as the bloc library as it would require to nest blocs inside each other.

I'm not sure the issue here, maybe Bloc specific? Using something like Provider, Get, GetIt or IW I just lookup what I need, and get it, it is no problem to go up multiple levels. I would set AppModel.SelectedProject, and anyone who wants to could read it.

Hixie commented 3 years ago

I would use this structure (sorry, ASCII art):

            Provide App State
                    |
                    |
               MaterialApp
                    |
     +--------------+--------------+
     |              |              |
  Project         Task         Workspace
 Selection      Selection        Screen
  Screen         Screen

...and when Project Selection Screen wants to change the project, it tells App State "hey, new project is X" by calling some method on the App State object, and App State notifies relevant clients, e.g. by having a ValueListenable<Project> exposing the project as one of its fields, whose value is initially null. Workspace Screen can then just subscribe to the App State's Project and Task listenables, and each of those objects can itself have lots of listenables for whatever information it wants to expose (or maybe they're just Listenables that notify clients when anything changes on them, it really depends on how fine-grained you want to be with your app model's notifications).

rrousselGit commented 3 years ago

I think this discussion will face the same difficulties as the discussion around hooks/syntax sugar for builders. It's not so much that we cannot do things differently. But we want to centralize the logic in one place, rather than duplicate the logic in all the involved placed.

The problem isn't about "hey, new project is X", but rather "When X is no longer in use, dispose of all the associated resources".

For that, we would need a subscription-based mechanism so that we can check hasListener. But that implies that we cannot simply rely on context.dependOn to listen to an Object (like what InheritedNotifier does), which means we need a Builder to listen to the object – which circles back to "Builders have a readability issue / it's hard to reuse logic without nesting".

rrousselGit commented 3 years ago

That's why I originally made https://github.com/flutter/flutter/pull/33213

This PR allows using context.dependOn while also implementing the hasListener logic. This means we can implement the subscription-based mechanism without forcing developers to consume the object with a Builder – which solves the readability issue.

But in the end, solving https://github.com/flutter/flutter/issues/51752 is probably more valuable as it would solve this issue too.

Hixie commented 3 years ago

we want to centralize the logic in one place, rather than duplicate the logic in all the involved placed

Right, that's consistent with the model of putting all the logic in the app state.

which means we need a Builder to listen to the object – which circles back to "Builders have a readability issue / it's hard to reuse logic without nesting"

Let's discuss that over in the other bug and leave this bug to the aspects of this that @Norbert515 does not view as being subsumed by #51752.

rrousselGit commented 3 years ago

Let's discuss that over in the other bug and leave this bug to the aspects of this that @Norbert515 does not view as being subsumed by #51752.

We came up with this issue together. This issue has been a major concern for the Provider community since forever.

This issue is a non-issue if we could afford to use Builders for consuming objects. A big part of the problem is that people do not want to give up on Provider's context.watch to listen to an object without the nesting.

masterwok commented 2 years ago

@rrousselGit have there been any developments on scoping or a recommended approach with Provider? Currently I'm using Provider + GetIt scopes but I'm finding it cumbersome to manually push/pop scopes when navigating as @esDotDev mentioned.

rrousselGit commented 2 years ago

With Provider, no.

This issue is one of the reasons Riverpod came to be. Riverpod solves this