Repository navigation
Conversation
Because we never specified the actual type, but let the compiler infer it and we cast the `model` parameter to `ChangeNotifier`, searching for the global state was always returning `null` because the widget tree didn't contain an instance of `_GlobalStateProvider<ChangeNotifier>`.
Code Coverage Report - 117 of 354 lines covered ( ⛔ 33.05%)
|
Code Coverage Report - 117 of 354 lines covered ( ⛔ 33.05%)
|
tooke24
left a comment
tooke24
left a comment
There was a problem hiding this comment.
Tested and approving — this fixes it.
I built a throwaway app against ref: PR and ref: main with the same test:
class Counter extends ChangeNotifier { int value = 7; }
testWidgets('getGlobalState returns the model', (tester) async {
await tester.pumpWidget(
StandardApp<Counter>(title: 'T', model: Counter(), body: const Probe()),
);
expect(find.text('got 7'), findsOneWidget);
});Passes on PR, fails on main. The _GlobalStateProvider<T> type argument is the fix.
Worth noting it also fixes a crash the title doesn't mention. On main, StandardApp<Counter> with no model throws:
type 'Null' is not a subtype of type 'ChangeNotifier' in type cast
because null is! T is true for a non-nullable T, so it enters the branch and casts a null. Guarding on model != null instead of on the type is the right call and fixes both.
One ask before merging: a test that would catch this coming back. Nothing in the repo calls getGlobalState — the only test change here updates an existing smoke test's generic, so this could silently regress. The two cases above (model present → returns it; model absent → null, no throw) would be enough.
Minor: narrowing the bound from <T extends ChangeNotifier?> to <T extends ChangeNotifier> is source-breaking for anyone who wrote StandardApp<Foo?> — your own test file needed updating for exactly that. Under the X.Y scheme being discussed that might argue for a minor rather than a patch bump.
If anyone used There was no way to fix the issue with the |
Good catch about bumping the minor number. I'll leave this as-is but keep it in mind for future changes. The person that requested this feature has yet to use it so this won't break any apps. |
Make sure the global state support works: - If no global state type is provided, asking for the global state returns `null` - If a type that extends ChangeNotifier is given, it is returned when asking for global state - If a type that doesn't extend ChangeNotifier is given, it won't compile
Code Coverage Report - 121 of 354 lines covered ( ⛔ 34.18%)
|
3 participants
This fixes the bug reported by @tooke24.