rrousselGit / provider

InheritedWidgets, but simple
https://pub.dev/packages/provider
MIT License
5.11k stars 512 forks source link

Issue with ChangeNotifierProvider.value on tests when passing Providers to different Routes #600

Closed FranRiadigos closed 3 years ago

FranRiadigos commented 3 years ago

Hi, thanks for your great work. I'm switching to riverpod now, but as I wrote a test and made some findings, I thought to post it here and see whether this is the intended behaviour or an error. This may be a Flutter issue though, if any, since the class causing the issue is ChangeNotifier.

Describe the bug

The Flow:

  1. A is a page that creates a ChangeNotifier with ChangeNotifierProvider()
  2. When moving from page A to B, I pass the ChangeNotifier with ChangeNotifierProvider.value
  3. Inside B I may or may not do 'things'
  4. Back to A all widgets have been updated (Nice!)

This is a quite usual use case where you edit values on a details page and when back to the previous screen you see all up-to-date, so you avoid reloading everything or wasting server resources.

The error:

This approach works well for me, no issues on dev environment. But when running a widget test, it breaks if the test exits on B.

As a workaround, I am popping back at the end of each tests to the A page.

Diving into the code, I found out that ChangeNotifier calls unmount on every instance alived, however it does so on a FIFO order instead of LIFO. This causes the original created ChangeNotifier instance to unmount and dispose, which removes all listeners, and hence the next instance crashes in debug due to _debugAssertNotDisposed.

To Reproduce

Test ```dart import 'package:flutter_test/flutter_test.dart'; import 'package:flutter_widget_test/tests/provider_failing_between_routes.dart' as app; void main() { testWidgets('ChangeNotifier throws between routes', (WidgetTester tester) async { await tester.pumpWidget(app.MyApp2()); expect(find.byType(app.A), findsOneWidget); await tester.tap(find.byKey(app.buttonKey)); await tester.pumpAndSettle(); expect(find.byType(app.B), findsOneWidget); }); } ```
Widgets ```dart import 'package:flutter/material.dart'; import 'package:flutter/widgets.dart'; import 'package:provider/provider.dart'; void main() { WidgetsFlutterBinding.ensureInitialized(); runApp(MyApp2()); } class MyApp2 extends StatelessWidget { @override Widget build(BuildContext context) { return MaterialApp( title: 'Flutter', home: Scaffold(body: A()), ); } } class TestFailingProviderAcrossRoutes extends ChangeNotifier {} final buttonKey = UniqueKey(); final button = Center(child: Text('button1')); class A extends StatelessWidget { @override Widget build(BuildContext context) { return ChangeNotifierProvider( create: (context) => TestFailingProviderAcrossRoutes(), child: Consumer( builder: (context, provider, child) => TextButton( key: buttonKey, onPressed: () { Navigator.push( context, MaterialPageRoute( builder: (context) { return ChangeNotifierProvider.value(value: provider, child: B()); }, )); }, child: button), )); } } class B extends StatelessWidget { @override Widget build(BuildContext context) { context.watch(); return Text(''); } } ```

Expected behavior Pass the testing environment 😄

This was simplified, I know you mentioned somewhere to initialise the Provider at the top of MaterialApp. But I can't do this, since A is actually a page with a ListView that creates on demand a ChangeNotifier per item.

If I go to the details page of one of the items, and I edit things there, I expect to go back and find all widgets refreshed with the updates.

(Any better approach please, let me know, as I haven't found any example on this anywhere)

rrousselGit commented 3 years ago

Yeah that's a known behavior and one of the reasons Riverpod exists

Scoping providers is generally unreliable because of too many factors.

I'd advise against scoping providers and instead place them above MaterialApp.

If you're looking for the advanced use cases such as "destroy the state when the route is popped", Riverpod is the way to go. It's "autoDispose" mechanism does the same thing without relying on scoping, making it significantly more reliable

rrousselGit commented 3 years ago

There's nothing we can do here. It's just a gimmick of the widget tree. So I'll close this 🙂

FranRiadigos commented 3 years ago

Hi, thanks for your reply and sorry for posting again.

I'm trying the same behaviour with riverpod, in an attempt to see if it works using ProviderScope and ScopedProvider. I'm trying to override a ScopedProvider that stores a StateNotifier, so that changes in the StateNotifier rebuild widgets. Slightly different from your marvel example.

However, I don't really know how to achieve this, or if there is another approach you would recommend.

Could you point me on the right path? I would really appreciate help on this.

rrousselGit commented 3 years ago

No as I said, you shouldn't rely on scoping

Don't scope the provider at all. Declare it as a global