Benefits & dangers of re-using values in StatefulProvider

[andreashaese]

First of all, your StatefulProvider is a really neat method to deal with disposing blocs that live in inherited widgets. I'm certainly going to use this pattern! In the following I'm assuming it's used specifically to serve blocs.

I want to share a concern: The implementation allows to reuse a previous value by passing it to valueBuilder. I'm not sure if this actually does what I guess it's supposed to do, and worse, if it might even impose the risk of unwanted/dangerous side effects.

If valueBuilder was expected to be called multiple times on the same widget (the provider), and creation of the value is expensive, then I would see a clear benefit in re-using an old value. But does that actually happen? valueBuilder is called during initState, in which case previous will obviously be null for a new provider instance and a new state. It's also called during didUpdateWidget, but that should happen only if the provider widget itself changed. So according to my understanding, the original provider widget will never actually make use of this reuse mechanism. But then what's the point? Maybe to share a value between multiple different Providers? That would seem very fragile to me, given Flutter's particular update handling of Widgets, Elements, and States.

Example

Say there's Provider<Bloc> providerA that generates a _StatefulProviderState<Bloc> with _value = blocA. Now, under very specific circumstances providerA gets replaced by a new Provider<Bloc> providerB (same type, same key). Both providerA and providerB have the same valueProvider: (_, bloc) => bloc ?? Bloc(). Since Flutter sees the chance to re-use the already existing _StatefulProviderState<Bloc>, it wires it to providerB. Now, providerB works with blocA instead of a fresh instance of Bloc. As the package user, I would not expect this behavior.

Clearly I might be overlooking an important detail or my assumptions could be wrong – could you please explain why you think this specific implementation of re-using an old value is beneficial, and if my concern regarding side effects is unwarranted?

[rrousselGit]

Hello!

I may have misunderstood what you explained, so feel free to correct me:

Basically, you're saying that you want to dispose the previous Bloc and create a new one but since the previous value is passed you cannot?

StatefulProvider is voluntarily limited, to stay simple. If you want very complex state management, you may want to create your own StatefulWidget and use Provider directly.

But from my understanding of your issue, it makes sense for providerA and providerB have different keys.

[andreashaese]

Thanks for the fast response! No, not quite. I'm wondering why the valueBuilder callback has the previous parameter at all, because I can't see under which circumstances previous != null would hold true. My question is: What is the useful thing that you can do by having previous handed to valueBuilder? In which concrete situations could this be useful for the framework user?

Frankly, I actually have run into one situation where previous != null: during hot reloads. But I wonder if this was the intention behind this design? I'm basically just curious, very possible I'm not yet seeing the full picture, and that bugs me 😄

[rrousselGit]

You're right, it isn't very useful at the moment. I originally wanted to allow custom state behaviors, but after a step back this is too limited to do anything.

I'm planning on deprecating the current StatefulProvider once I'm done with https://github.com/rrousselGit/flutter_hooks and introduce a new constructor with a cleaner API

The linked library will be combined with this one to introduce a HookProvider:

return HookProvider<int>(
 builder: (HookContext context) => context.useState<int>(initialData: 0).value,
 child: child,
);

There's an article planned for next week, stay in touch. :)

[andreashaese]

Thanks for the clarification. Now this other project looks intriguing, looking forward to it!

[andreashaese]

Not sure how much it still matters to you given that you want to deprecate this plugin, but I've coincidentally run into an example where the unwanted side effects I was explaining in theory can be seen in action:

class HomePage extends StatefulWidget {
  @override createState() => _HomePageState();
}

class _HomePageState extends State<HomePage> {
  int _pageIndex = 0; // just for demo purposes

  @override build(context) => Scaffold(
    body: IndexPage(index: _pageIndex),
    bottomNavigationBar: BottomNavigationBar(
      currentIndex: _pageIndex,
      items: List<BottomNavigationBarItem>.generate(3, (i) => BottomNavigationBarItem(
        icon: Icon(Icons.close),
        title: Text('Page $i'),
      )),
      onTap: (index) => setState(() { _pageIndex = index; }),
    ),
  );
}

class IndexPage extends StatelessWidget {
  final int index;

  IndexPage({@required this.index}) : assert(index != null), super();

  @override build(context) => StatefulProvider<String>(
    valueBuilder: (_, value) => /*value ??*/ 'Page $index', // <-- (1)
    child: Builder(
      builder: (context) => Scaffold(
        body: Center(
          child: Text(Provider.of<String>(context)),
        ),
      ),
    ),
  );
}

The left image is the correct result as per the above code, the right image is when value ?? in the line marked with (1) isn't commented out.

Maybe somebody finds this piece of info useful.

[rrousselGit]

What's the point of a StatefulProvider here? A simple provider is more likely desired.

[andreashaese]

Sure, but that's not the point of the example — it merely shows that the danger is real. Imagine a bottom navigation with similar but different content views that each handle their specific setup of a common bloc type, e.g. a news feed app with multiple pages for different feeds. The demonstrated issue would be the same.

For my own projects, I have derived a plugin based on your code that just doesn't pass the value to valueBuilder, in order to prevent this trap. That being said, I love this "disposable inherited widget" approach and don't think it should be discarded even if more powerful alternatives emerge.

[rrousselGit]

I don't get how this is an issue. If you don't need the old value, don't use it right?

[rrousselGit]

I'll have some time to work on Provider again.

Do you have any suggestions for this library? :)

[andreashaese]

To me the library seems technically sound.

It could be clearer how to "not use it in unsafe ways" though. The API, looking innocent enough, encourages re-use of previously generated values. That's a potential benefit in certain conditions, and a potential side effect threat in other conditions. Even with a solid understanding of the Widget and State lifecycle (which isn't exactly a trivial aspect of Flutter), it's hard to reason about the origin of the bug in the above code: With no explicit counter measures, state leaks between multiple pages. That's not specific to the library, but I think it could be made more user friendly by accounting for this in the design.

Here are some suggestions: 1) Like Dismissible, require a non-null Key in the constructor to make sure the user understands what they're doing 2) Not forward old values to valueBuilder, thereby "lazily" circumventing the issue (at the cost of a useful feature?) 3) Discuss potential side effects of value in the documentation and examples

Regarding "real world relevance": I've seen an otherwise great YouTube video demonstrating Provider with subtle inconsistencies (custom implemented), and what started for me as a code smell there, turned into me opening this issue here. If this topic isn't significant enough – and I'm not sure it is for someone else – I won't mind if you close it. If nothing else, I've learned a ton about Flutter in the process.

[rrousselGit]

I'll go with the easy road, and deprecate StatefulProvider in favor of HookProvider.

That one allows a much more powerful state management