From 0f384f0679860bb807acba5c3b387c95d303290e Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 17 Mar 2024 15:52:40 +0100 Subject: [PATCH 01/27] Refactor pause mechanism --- .../lib/src/core/consumer.dart | 6 +++ packages/riverpod/lib/src/core/element.dart | 47 ++++++++++--------- .../riverpod/lib/src/core/foundation.dart | 2 +- .../lib/src/core/modifiers/select.dart | 6 +++ .../lib/src/core/provider/provider.dart | 12 ++--- .../lib/src/core/provider_subscription.dart | 38 ++++++++++++++- .../src/core/proxy_provider_listenable.dart | 16 +++++-- packages/riverpod/lib/src/core/ref.dart | 2 +- packages/riverpod/lib/src/framework.dart | 1 + packages/riverpod/test/src/core/ref_test.dart | 3 +- 10 files changed, 96 insertions(+), 37 deletions(-) diff --git a/packages/flutter_riverpod/lib/src/core/consumer.dart b/packages/flutter_riverpod/lib/src/core/consumer.dart index 88dd30462..0971d4080 100644 --- a/packages/flutter_riverpod/lib/src/core/consumer.dart +++ b/packages/flutter_riverpod/lib/src/core/consumer.dart @@ -528,4 +528,10 @@ final class _ListenManual extends ProviderSubscription { @override T read() => _subscription.read(); + + @override + void pause() => _subscription.pause(); + + @override + void resume() => _subscription.resume(); } diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index cc2ea4fb0..1545b0b99 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -42,7 +42,9 @@ void Function()? debugCanModifyProviders; /// {@endtemplate} @internal @optionalTypeArgs -abstract class ProviderElement implements WrappedNode { +abstract class ProviderElement + with _OnPauseMixin + implements WrappedNode { /// {@macro riverpod.provider_element_base} ProviderElement(this.pointer); @@ -76,17 +78,17 @@ abstract class ProviderElement implements WrappedNode { /// - all of its listeners are "weak" (i.e. created with `listen(weak: true)`) /// /// See also [_mayNeedDispose], called when [isActive] may have changed. - bool get isActive => - (_dependents?.isNotEmpty ?? false) || _watchDependents.isNotEmpty; + bool get isActive => !_isPaused && _activeListenerCount > 0; + + int get _activeListenerCount => + (_dependents?.length ?? 0) + _watchDependents.length; /// Whether this [ProviderElement] is currently listened to or not. /// /// This maps to listeners added with `listen` and `watch`, /// excluding `listen(weak: true)`. bool get hasListeners => - (_dependents?.isNotEmpty ?? false) || - _watchDependents.isNotEmpty || - _weakDependents.isNotEmpty; + _activeListenerCount > 0 || _weakDependents.isNotEmpty; var _dependencies = HashMap(); HashMap? _previousDependencies; @@ -107,8 +109,6 @@ abstract class ProviderElement implements WrappedNode { bool _dependencyMayHaveChanged = false; bool _didChangeDependency = false; - var _didCancelOnce = false; - /// Whether the assert that prevents [requireState] from returning /// if the state was not set before is enabled. @visibleForOverriding @@ -335,8 +335,8 @@ This could mean a few things: // Unsubscribe to everything that a provider no longer depends on. for (final sub in previousDependencies.entries) { sub.key - .._watchDependents.remove(this) - .._onRemoveListener(); + .._onRemoveListener(weak: false) + .._watchDependents.remove(this); } _previousDependencies = null; } @@ -600,19 +600,24 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu return result; } - void _onListen() { + @override + void _onResume() => ref?._onResumeListeners?.forEach(runGuarded); + + @override + void _onCancel() => ref?._onCancelListeners?.forEach(runGuarded); + + void _onListen({required bool weak}) { + if (!weak && _isPaused && _activeListenerCount == 0) resume(); + ref?._onAddListeners?.forEach(runGuarded); - if (_didCancelOnce && !hasListeners) { - ref?._onResumeListeners?.forEach(runGuarded); - } } - void _onRemoveListener() { - ref?._onRemoveListeners?.forEach(runGuarded); - if (!hasListeners) { - _didCancelOnce = true; - ref?._onCancelListeners?.forEach(runGuarded); + void _onRemoveListener({required bool weak}) { + if (!weak && _activeListenerCount == 1) { + pause(); } + + ref?._onRemoveListeners?.forEach(runGuarded); _mayNeedDispose(); } @@ -672,7 +677,7 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu ref._onRemoveListeners = null; ref._onChangeSelfListeners = null; ref._onErrorSelfListeners = null; - _didCancelOnce = false; + _pauseCount = 0; assert( ref._keepAliveLinks == null || ref._keepAliveLinks!.isEmpty, @@ -700,8 +705,8 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu ref = null; for (final sub in _dependencies.entries) { + sub.key._onRemoveListener(weak: false); sub.key._watchDependents.remove(this); - sub.key._onRemoveListener(); } _dependencies.clear(); } diff --git a/packages/riverpod/lib/src/core/foundation.dart b/packages/riverpod/lib/src/core/foundation.dart index 17b26f39c..0dd5c1d92 100644 --- a/packages/riverpod/lib/src/core/foundation.dart +++ b/packages/riverpod/lib/src/core/foundation.dart @@ -174,7 +174,7 @@ String shortHash(Object? object) { mixin ProviderListenable implements ProviderListenableOrFamily { /// Starts listening to this transformer ProviderSubscription addListener( - Node node, + Node source, void Function(StateT? previous, StateT next) listener, { required void Function(Object error, StackTrace stackTrace)? onError, required void Function()? onDependencyMayHaveChanged, diff --git a/packages/riverpod/lib/src/core/modifiers/select.dart b/packages/riverpod/lib/src/core/modifiers/select.dart index 4cce5cc42..d8f2bd73b 100644 --- a/packages/riverpod/lib/src/core/modifiers/select.dart +++ b/packages/riverpod/lib/src/core/modifiers/select.dart @@ -216,4 +216,10 @@ final class _SelectorSubscription return _read(); } + + @override + void pause() => _internalSub.pause(); + + @override + void resume() => _internalSub.resume(); } diff --git a/packages/riverpod/lib/src/core/provider/provider.dart b/packages/riverpod/lib/src/core/provider/provider.dart index cdd6fa09f..cbe3ea558 100644 --- a/packages/riverpod/lib/src/core/provider/provider.dart +++ b/packages/riverpod/lib/src/core/provider/provider.dart @@ -52,22 +52,22 @@ abstract base class ProviderBase extends ProviderOrFamily @override ProviderSubscription addListener( - Node node, + Node source, void Function(StateT? previous, StateT next) listener, { required void Function(Object error, StackTrace stackTrace)? onError, required void Function()? onDependencyMayHaveChanged, required bool fireImmediately, }) { assert( - !fireImmediately || !node.weak, + !fireImmediately || !source.weak, 'Cannot use fireImmediately with weak listeners', ); onError ??= Zone.current.handleUncaughtError; - final element = node.readProviderElement(this); + final element = source.readProviderElement(this); - if (!node.weak) element.flush(); + if (!source.weak) element.flush(); if (fireImmediately) { _handleFireImmediately( @@ -80,10 +80,10 @@ abstract base class ProviderBase extends ProviderOrFamily // Calling before initializing the subscription, // to ensure that "hasListeners" represents the state _before_ // the listener is added - element._onListen(); + element._onListen(weak: source.weak); return _ProviderStateSubscription( - node, + source, listenedElement: element, listener: (prev, next) => listener(prev as StateT?, next as StateT), onError: onError, diff --git a/packages/riverpod/lib/src/core/provider_subscription.dart b/packages/riverpod/lib/src/core/provider_subscription.dart index ec31da396..cadb3254a 100644 --- a/packages/riverpod/lib/src/core/provider_subscription.dart +++ b/packages/riverpod/lib/src/core/provider_subscription.dart @@ -22,6 +22,9 @@ abstract base class ProviderSubscription { bool get closed => _closed; var _closed = false; + void pause(); + void resume(); + /// Obtain the latest value emitted by the provider. /// /// This method throws if [closed] is true. @@ -42,10 +45,35 @@ abstract base class ProviderSubscription { } } +mixin _OnPauseMixin { + bool get _isPaused => _pauseCount > 0; + var _pauseCount = 0; + + @mustCallSuper + void pause() { + if (_pauseCount == 0) { + _onCancel(); + } + _pauseCount = max(_pauseCount + 1, 0); + } + + @mustCallSuper + void resume() { + if (_pauseCount == 1) { + _onResume(); + } + _pauseCount = min(_pauseCount - 1, 0); + } + + void _onResume(); + + void _onCancel(); +} + /// When a provider listens to another provider using `listen` @optionalTypeArgs final class _ProviderStateSubscription - extends ProviderSubscription { + extends ProviderSubscription with _OnPauseMixin { _ProviderStateSubscription( super.source, { required this.listenedElement, @@ -80,17 +108,23 @@ final class _ProviderStateSubscription @override void close() { if (!closed) { + listenedElement._onRemoveListener(weak: source.weak); switch (source) { case WeakNode(): listenedElement._weakDependents.remove(this); case _: listenedElement._dependents?.remove(this); } - listenedElement._onRemoveListener(); } super.close(); } + + @override + void _onCancel() => listenedElement.pause(); + + @override + void _onResume() => listenedElement.resume(); } /// Deals with the internals of synchronously calling the listeners diff --git a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart index 0ae223b57..2d3d4e5bc 100644 --- a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart +++ b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart @@ -31,6 +31,12 @@ final class _ProxySubscription extends ProviderSubscription { super.close(); } + + @override + void pause() => innerSubscription.pause(); + + @override + void resume() => innerSubscription.resume(); } /// An internal utility for reading alternate values of a provider. @@ -76,13 +82,13 @@ class ProviderElementProxy @override ProviderSubscription addListener( - Node node, + Node source, void Function(OutputT? previous, OutputT next) listener, { required void Function(Object error, StackTrace stackTrace)? onError, required void Function()? onDependencyMayHaveChanged, required bool fireImmediately, }) { - final element = node.readProviderElement(provider); + final element = source.readProviderElement(provider); // While we don't care about changes to the element, calling _listenElement // is necessary to tell the listened element that it is being listened. @@ -90,7 +96,7 @@ class ProviderElementProxy // a listener to the notifier. // This avoids the listener from being immediately notified of a new // future when adding the listener refreshes the future. - final innerSub = node.listen( + final innerSub = source.listen( provider, (prev, next) {}, fireImmediately: false, @@ -118,9 +124,9 @@ class ProviderElementProxy ); return _ProxySubscription( - node, + source, removeListener, - () => read(node), + () => read(source), innerSubscription: innerSub, ); } diff --git a/packages/riverpod/lib/src/core/ref.dart b/packages/riverpod/lib/src/core/ref.dart index 282b28556..0414e284d 100644 --- a/packages/riverpod/lib/src/core/ref.dart +++ b/packages/riverpod/lib/src/core/ref.dart @@ -567,7 +567,7 @@ final = Provider(dependencies: []); } element - .._onListen() + .._onListen(weak: false) .._watchDependents.add(_element); return Object(); diff --git a/packages/riverpod/lib/src/framework.dart b/packages/riverpod/lib/src/framework.dart index ac3c940ab..8cdb57e05 100644 --- a/packages/riverpod/lib/src/framework.dart +++ b/packages/riverpod/lib/src/framework.dart @@ -2,6 +2,7 @@ library framework; import 'dart:async'; import 'dart:collection'; +import 'dart:math'; import 'package:collection/collection.dart'; import 'package:meta/meta.dart'; diff --git a/packages/riverpod/test/src/core/ref_test.dart b/packages/riverpod/test/src/core/ref_test.dart index 1131b30cc..237bac189 100644 --- a/packages/riverpod/test/src/core/ref_test.dart +++ b/packages/riverpod/test/src/core/ref_test.dart @@ -2600,7 +2600,8 @@ void main() { group('.onCancel', () { test( 'is called when dependent is invalidated and was the only listener', - skip: 'Waiting for "clear dependencies after futureprovider rebuilds"', + // TODO deal with now that we have onPause + skip: 'Waiting for "clear dependencies after FutureProvider rebuilds"', () async { // final container = ProviderContainer.test(); From b736550126fabee24c77200026ad3967836ce7dd Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 17 Mar 2024 16:01:30 +0100 Subject: [PATCH 02/27] Changelog --- packages/riverpod/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/riverpod/CHANGELOG.md b/packages/riverpod/CHANGELOG.md index 4c89b9d4e..2dafa1877 100644 --- a/packages/riverpod/CHANGELOG.md +++ b/packages/riverpod/CHANGELOG.md @@ -22,6 +22,8 @@ - An error is now thrown when trying to override a provider twice in the same `ProviderContainer`. - Disposing a `ProviderContainer` now disposes of all of its sub `ProviderContainers` too. +- Added `ProviderSubscription.pause()`/`.resume()`. + This enables temporarily stopping the subscription to a provider, without it possibly loosing its state when using `autoDispose`. - Added `ProviderContainer.test()`. This is a custom constructor for testing purpose. It is meant to replace the `createContainer` utility. - Added `NotifierProvider.overrideWithBuild`, to override `Notifier.build` without From a57f4a0fa602af50448b4c2229f67aea84effa13 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 17 Mar 2024 16:15:33 +0100 Subject: [PATCH 03/27] Coverage --- .../test/{ => src/core}/consumer_test.dart | 37 +++++++++++++++++++ .../test/src/core/modifiers/select_test.dart | 24 ++++++++++++ .../src/core/provider_subscription_test.dart | 25 +++++++++++++ .../core/proxy_provider_listenable_test.dart | 28 ++++++++++++++ 4 files changed, 114 insertions(+) rename packages/flutter_riverpod/test/{ => src/core}/consumer_test.dart (95%) create mode 100644 packages/riverpod/test/src/core/provider_subscription_test.dart create mode 100644 packages/riverpod/test/src/core/proxy_provider_listenable_test.dart diff --git a/packages/flutter_riverpod/test/consumer_test.dart b/packages/flutter_riverpod/test/src/core/consumer_test.dart similarity index 95% rename from packages/flutter_riverpod/test/consumer_test.dart rename to packages/flutter_riverpod/test/src/core/consumer_test.dart index 831f05a6d..f21cdfb06 100644 --- a/packages/flutter_riverpod/test/consumer_test.dart +++ b/packages/flutter_riverpod/test/src/core/consumer_test.dart @@ -6,6 +6,43 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:riverpod/legacy.dart'; void main() { + group('_ListenManual', () { + testWidgets('handles pause/resume', (tester) async { + late WidgetRef ref; + late ProviderSubscription sub; + final provider = Provider((ref) => 0); + + await tester.pumpWidget( + ProviderScope( + child: Consumer( + builder: (context, r, child) { + ref = r; + sub = ref.listenManual( + provider, + (_, __) {}, + ); + return const SizedBox(); + }, + ), + ), + ); + + final container = ProviderScope.containerOf(ref.context); + // ignore: invalid_use_of_internal_member + final element = container.readProviderElement(provider); + + expect(element.isActive, true); + + sub.pause(); + + expect(element.isActive, false); + + sub.resume(); + + expect(element.isActive, true); + }); + }); + testWidgets('Riverpod test', (tester) async { // Regression test for https://github.com/rrousselGit/riverpod/pull/3156 diff --git a/packages/riverpod/test/src/core/modifiers/select_test.dart b/packages/riverpod/test/src/core/modifiers/select_test.dart index a0b4d9eda..a3ed9d935 100644 --- a/packages/riverpod/test/src/core/modifiers/select_test.dart +++ b/packages/riverpod/test/src/core/modifiers/select_test.dart @@ -153,4 +153,28 @@ void main() { }); }); }); + + group('_ProviderSelector', () { + test('handles pause/resume', () { + final container = ProviderContainer.test(); + final provider = Provider((ref) => 0); + + final element = container.readProviderElement(provider); + + final sub = container.listen( + provider.select((value) => null), + (previous, next) {}, + ); + + expect(element.isActive, true); + + sub.pause(); + + expect(element.isActive, false); + + sub.resume(); + + expect(element.isActive, true); + }); + }); } diff --git a/packages/riverpod/test/src/core/provider_subscription_test.dart b/packages/riverpod/test/src/core/provider_subscription_test.dart new file mode 100644 index 000000000..2fc7c799f --- /dev/null +++ b/packages/riverpod/test/src/core/provider_subscription_test.dart @@ -0,0 +1,25 @@ +import 'package:riverpod/riverpod.dart'; +import 'package:test/test.dart'; + +void main() { + group('_ProxySubscription', () { + test('handles pause/resume', () { + final container = ProviderContainer.test(); + final provider = FutureProvider((ref) => 0); + + final element = container.readProviderElement(provider); + + final sub = container.listen(provider.future, (previous, next) {}); + + expect(element.isActive, true); + + sub.pause(); + + expect(element.isActive, false); + + sub.resume(); + + expect(element.isActive, true); + }); + }); +} diff --git a/packages/riverpod/test/src/core/proxy_provider_listenable_test.dart b/packages/riverpod/test/src/core/proxy_provider_listenable_test.dart new file mode 100644 index 000000000..91b29bbec --- /dev/null +++ b/packages/riverpod/test/src/core/proxy_provider_listenable_test.dart @@ -0,0 +1,28 @@ +import 'package:riverpod/riverpod.dart'; +import 'package:test/test.dart'; + +void main() { + group('_ProviderSelector', () { + test('handles pause/resume', () { + final container = ProviderContainer.test(); + final provider = Provider((ref) => 0); + + final element = container.readProviderElement(provider); + + final sub = container.listen( + provider.select((value) => null), + (previous, next) {}, + ); + + expect(element.isActive, true); + + sub.pause(); + + expect(element.isActive, false); + + sub.resume(); + + expect(element.isActive, true); + }); + }); +} From 1aaaea8424898f107dfa5ca0a920c29e56c30cec Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 17 Mar 2024 16:26:27 +0100 Subject: [PATCH 04/27] Changelog --- packages/riverpod/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/riverpod/CHANGELOG.md b/packages/riverpod/CHANGELOG.md index 2dafa1877..e515dfd4b 100644 --- a/packages/riverpod/CHANGELOG.md +++ b/packages/riverpod/CHANGELOG.md @@ -15,6 +15,9 @@ - `Stream/FutureProvider.overrideWithValue` was added back. - **Breaking**: `Notifier` and variants are now recreated whenever the provider rebuilds. This enables using `Ref.mounted` to check dispose. +- **Breaking**: A provider is now considered "paused" if all + of its listeners are also paused. So if a provider `A` is watched _only_ by a provider `B`, and `B` is currently unused, + then `A` will be paused. - Added `Ref.listen(..., weak: true)`. When specifying `weak: true`, the listener will not cause the provider to be initialized. This is useful when wanting to react to changes to a provider, From c52514de7d5a8c044239ccebeadf1e9f4f7c9bf5 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 17 Mar 2024 16:37:44 +0100 Subject: [PATCH 05/27] Refactor _mount to use notifyListeners --- packages/riverpod/lib/src/core/element.dart | 99 +++++-------------- packages/riverpod/test/src/core/ref_test.dart | 19 ++++ 2 files changed, 44 insertions(+), 74 deletions(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index 1545b0b99..656bdaf9b 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -227,71 +227,12 @@ This could mean a few things: final ref = this.ref = Ref._(this); buildState(ref); - // TODO refactor to use notifyListeners(); - switch (_stateResult!) { - case final ResultData newState: - final onChangeSelfListeners = ref._onChangeSelfListeners; - if (onChangeSelfListeners != null) { - for (var i = 0; i < onChangeSelfListeners.length; i++) { - Zone.current.runBinaryGuarded( - onChangeSelfListeners[i], - null, - newState.state, - ); - } - } - - final listeners = _weakDependents.toList(growable: false); - for (var i = 0; i < listeners.length; i++) { - final listener = listeners[i]; - if (listener is _ProviderStateSubscription) { - Zone.current.runBinaryGuarded( - listener.listener, - null, - newState.state, - ); - } - } - - for (final observer in container.observers) { - runTernaryGuarded( - observer.didAddProvider, - origin, - newState.state, - container, - ); - } - - case final ResultError newState: - final onErrorSelfListeners = ref._onErrorSelfListeners; - if (onErrorSelfListeners != null) { - for (var i = 0; i < onErrorSelfListeners.length; i++) { - Zone.current.runBinaryGuarded( - onErrorSelfListeners[i], - newState.error, - newState.stackTrace, - ); - } - } - - for (final observer in container.observers) { - runTernaryGuarded( - observer.didAddProvider, - origin, - null, - container, - ); - } - for (final observer in container.observers) { - runQuaternaryGuarded( - observer.providerDidFail, - origin, - newState.error, - newState.stackTrace, - container, - ); - } - } + _notifyListeners( + _stateResult!, + null, + isMount: true, + checkUpdateShouldNotify: false, + ); } /// Called when the override of a provider changes. @@ -461,8 +402,9 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu Result newState, Result? previousStateResult, { bool checkUpdateShouldNotify = true, + bool isMount = false, }) { - if (kDebugMode) _debugAssertNotificationAllowed(); + if (kDebugMode && !isMount) _debugAssertNotificationAllowed(); final previousState = previousStateResult?.stateOrNull; @@ -503,7 +445,7 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu return; } - final listeners = [..._weakDependents, ...?_dependents]; + final listeners = [..._weakDependents, if (!isMount) ...?_dependents]; switch (newState) { case final ResultData newState: for (var i = 0; i < listeners.length; i++) { @@ -535,13 +477,22 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu } for (final observer in container.observers) { - runQuaternaryGuarded( - observer.didUpdateProvider, - origin, - previousState, - newState.stateOrNull, - container, - ); + if (isMount) { + runTernaryGuarded( + observer.didAddProvider, + origin, + newState.stateOrNull, + container, + ); + } else { + runQuaternaryGuarded( + observer.didUpdateProvider, + origin, + previousState, + newState.stateOrNull, + container, + ); + } } for (final observer in container.observers) { diff --git a/packages/riverpod/test/src/core/ref_test.dart b/packages/riverpod/test/src/core/ref_test.dart index 237bac189..8511b2c25 100644 --- a/packages/riverpod/test/src/core/ref_test.dart +++ b/packages/riverpod/test/src/core/ref_test.dart @@ -828,6 +828,25 @@ void main() { }); group('listen', () { + test('does not invoke the listener if paused', () { + final container = ProviderContainer.test(); + final listener = Listener(); + late Ref ref; + final provider = Provider((r) { + ref = r; + return 0; + }); + + final sub = container.listen(provider, listener.call); + sub.pause(); + + verifyZeroInteractions(listener); + + ref.notifyListeners(); + + verifyZeroInteractions(listener); + }); + group('weak', () { test('Mounts the element but does not initialize the provider', () { final container = ProviderContainer.test(); From ed0a372c898d1a5e560ade47bbbc6142ee0bcaa2 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 17 Mar 2024 17:09:28 +0100 Subject: [PATCH 06/27] Don't invoke paused listeners --- packages/riverpod/lib/src/core/element.dart | 5 +- .../test/src/core/provider_element_test.dart | 2 +- packages/riverpod/test/src/core/ref_test.dart | 46 ++++++++++++++++--- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index 656bdaf9b..3cbffd877 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -450,7 +450,7 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu case final ResultData newState: for (var i = 0; i < listeners.length; i++) { final listener = listeners[i]; - if (listener is _ProviderStateSubscription) { + if (listener is _ProviderStateSubscription && !listener._isPaused) { Zone.current.runBinaryGuarded( listener.listener, previousState, @@ -461,7 +461,8 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu case final ResultError newState: for (var i = 0; i < listeners.length; i++) { final listener = listeners[i]; - if (listener is _ProviderStateSubscription) { + if (listener is _ProviderStateSubscription && + !listener._isPaused) { Zone.current.runBinaryGuarded( listener.onError, newState.error, diff --git a/packages/riverpod/test/src/core/provider_element_test.dart b/packages/riverpod/test/src/core/provider_element_test.dart index 631820e92..9dba1fddc 100644 --- a/packages/riverpod/test/src/core/provider_element_test.dart +++ b/packages/riverpod/test/src/core/provider_element_test.dart @@ -34,7 +34,7 @@ void main() { // schedules `dep` and `dependent` to rebuild // Will build `dependent` before `dep` because `dependent` doesn't depend on `dep` yet - // And since nothing is watchin `dep` at the moment, then `dependent` will + // And since nothing is watching `dep` at the moment, then `dependent` will // rebuild before `dep` even though `dep` is its ancestor. // This is fine since nothing is listening to `dep` yet, but it should // not cause certain assertions to trigger diff --git a/packages/riverpod/test/src/core/ref_test.dart b/packages/riverpod/test/src/core/ref_test.dart index 8511b2c25..79ce4eb2b 100644 --- a/packages/riverpod/test/src/core/ref_test.dart +++ b/packages/riverpod/test/src/core/ref_test.dart @@ -3,12 +3,7 @@ import 'dart:async'; import 'package:mockito/mockito.dart'; import 'package:riverpod/legacy.dart'; import 'package:riverpod/riverpod.dart'; -import 'package:riverpod/src/internals.dart' - show - CircularDependencyError, - ProviderContainerTest, - ProviderElement, - UnmountedRefException; +import 'package:riverpod/src/framework.dart'; import 'package:test/test.dart'; import '../utils.dart'; @@ -828,7 +823,7 @@ void main() { }); group('listen', () { - test('does not invoke the listener if paused', () { + test('does not invoke value listeners if paused', () { final container = ProviderContainer.test(); final listener = Listener(); late Ref ref; @@ -847,6 +842,43 @@ void main() { verifyZeroInteractions(listener); }); + test('does not invoke error listeners if paused', () { + final container = ProviderContainer.test(); + final listener = ErrorListener(); + late Ref ref; + var throws = false; + final provider = Provider((r) { + ref = r; + if (throws) throw StateError('err'); + }); + + final errors = []; + final sub = container.listen( + provider, + (a, b) {}, + onError: listener.call, + ); + + sub.pause(); + + verifyZeroInteractions(listener); + + throws = true; + runZonedGuarded( + () { + try { + container.refresh(provider); + } catch (e) { + // We just want to trigger onError listener + } + }, + (error, stack) => errors.add(error), + ); + verifyZeroInteractions(listener); + + expect(errors, [isA()]); + }); + group('weak', () { test('Mounts the element but does not initialize the provider', () { final container = ProviderContainer.test(); From eadd075be7f6d188b2c5f750acf851526ce80545 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Fri, 22 Mar 2024 13:38:07 +0100 Subject: [PATCH 07/27] W --- .../lib/src/core/consumer.dart | 3 + packages/riverpod/lib/src/core/element.dart | 194 ++++++++++++------ .../lib/src/core/modifiers/select.dart | 3 + .../lib/src/core/provider/provider.dart | 14 +- .../lib/src/core/provider_subscription.dart | 41 ++-- .../src/core/proxy_provider_listenable.dart | 3 + packages/riverpod/lib/src/core/ref.dart | 57 +++-- .../test/src/core/provider_element_test.dart | 77 ++++++- .../src/core/provider_subscription_test.dart | 4 + packages/riverpod/test/src/core/ref_test.dart | 32 +-- .../src/providers/stream_notifier_test.dart | 4 + 11 files changed, 298 insertions(+), 134 deletions(-) diff --git a/packages/flutter_riverpod/lib/src/core/consumer.dart b/packages/flutter_riverpod/lib/src/core/consumer.dart index 0971d4080..161f7fb02 100644 --- a/packages/flutter_riverpod/lib/src/core/consumer.dart +++ b/packages/flutter_riverpod/lib/src/core/consumer.dart @@ -514,6 +514,9 @@ class ConsumerStatefulElement extends StatefulElement implements WidgetRef { final class _ListenManual extends ProviderSubscription { _ListenManual(super.source, this._subscription, this._element); + @override + bool get isPaused => _subscription.isPaused; + final ProviderSubscription _subscription; final ConsumerStatefulElement _element; diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index 3cbffd877..e94e2e8dc 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -42,9 +42,7 @@ void Function()? debugCanModifyProviders; /// {@endtemplate} @internal @optionalTypeArgs -abstract class ProviderElement - with _OnPauseMixin - implements WrappedNode { +abstract class ProviderElement implements WrappedNode { /// {@macro riverpod.provider_element_base} ProviderElement(this.pointer); @@ -78,20 +76,23 @@ abstract class ProviderElement /// - all of its listeners are "weak" (i.e. created with `listen(weak: true)`) /// /// See also [_mayNeedDispose], called when [isActive] may have changed. - bool get isActive => !_isPaused && _activeListenerCount > 0; + bool get isActive => (_listenerCount - _pausedActiveSubscriptionCount) > 0; - int get _activeListenerCount => - (_dependents?.length ?? 0) + _watchDependents.length; + int get _listenerCount => _dependents?.length ?? 0; + // (_dependents?.length ?? 0) + _watchDependents.length; + + var _pausedActiveSubscriptionCount = 0; + var _didCancelOnce = false; /// Whether this [ProviderElement] is currently listened to or not. /// /// This maps to listeners added with `listen` and `watch`, /// excluding `listen(weak: true)`. - bool get hasListeners => - _activeListenerCount > 0 || _weakDependents.isNotEmpty; + bool get hasListeners => _listenerCount > 0 || _weakDependents.isNotEmpty; - var _dependencies = HashMap(); - HashMap? _previousDependencies; + // var _dependencies = HashMap(); + // HashMap? _previousDependencies; + List? _previousSubscriptions; List? _subscriptions; List? _dependents; @@ -102,8 +103,8 @@ abstract class ProviderElement /// - They may be reused between two instances of a [ProviderElement]. final _weakDependents = []; - /// The element of the providers that depends on this provider. - final _watchDependents = []; + // /// The element of the providers that depends on this provider. + // final _watchDependents = []; bool _mustRecomputeState = false; bool _dependencyMayHaveChanged = false; @@ -248,12 +249,12 @@ This could mean a few things: /// After a provider is initialized, this function takes care of unsubscribing /// to dependencies that are no-longer used. void _performBuild() { - assert( - _previousDependencies == null, - 'Bad state: _performBuild was called twice', - ); - final previousDependencies = _previousDependencies = _dependencies; - _dependencies = HashMap(); + // assert( + // _previousDependencies == null, + // 'Bad state: _performBuild was called twice', + // ); + // final previousDependencies = _previousDependencies = _dependencies; + // _dependencies = HashMap(); runOnDispose(); final ref = this.ref = Ref._(this); @@ -273,13 +274,20 @@ This could mean a few things: if (kDebugMode) _debugSkipNotifyListenersAsserts = false; } - // Unsubscribe to everything that a provider no longer depends on. - for (final sub in previousDependencies.entries) { - sub.key - .._onRemoveListener(weak: false) - .._watchDependents.remove(this); + final previousSubscriptions = _previousSubscriptions; + if (previousSubscriptions != null) { + _closeSubscriptions(previousSubscriptions); } - _previousDependencies = null; + + // // Unsubscribe to everything that a provider no longer depends on. + // for (final sub in previousDependencies.entries) { + // sub.key._onRemoveListener( + // () => sub.key._watchDependents.remove(this), + // weak: false, + // isPaused: !isActive, + // ); + // } + // _previousDependencies = null; } /// Initialize a provider. @@ -473,9 +481,11 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu default: } - for (var i = 0; i < _watchDependents.length; i++) { - _watchDependents[i].invalidateSelf(asReload: true); - } + // if (!isMount) { + // for (var i = 0; i < _watchDependents.length; i++) { + // _watchDependents[i].invalidateSelf(asReload: true); + // } + // } for (final observer in container.observers) { if (isMount) { @@ -515,6 +525,7 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu _dependencyMayHaveChanged = true; visitChildren( + // TODO skip paused subscriptions elementVisitor: (element) => element._markDependencyMayHaveChanged(), listenableVisitor: (notifier) => notifier.notifyDependencyMayHaveChanged(), @@ -552,25 +563,74 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu return result; } - @override - void _onResume() => ref?._onResumeListeners?.forEach(runGuarded); - - @override - void _onCancel() => ref?._onCancelListeners?.forEach(runGuarded); + SubT _onListen(SubT Function() add) { + print('listen $origin'); + final wasActive = isActive; + ref?._onAddListeners?.forEach(runGuarded); - void _onListen({required bool weak}) { - if (!weak && _isPaused && _activeListenerCount == 0) resume(); + final sub = add(); + if (_didCancelOnce && !wasActive && isActive) { + _onResume(); + } - ref?._onAddListeners?.forEach(runGuarded); + return sub; } - void _onRemoveListener({required bool weak}) { - if (!weak && _activeListenerCount == 1) { - pause(); - } + void _onRemoveListener(ProviderSubscription Function() remove) { + print('unsub $origin'); + final wasActive = isActive; - ref?._onRemoveListeners?.forEach(runGuarded); + final removedSub = remove(); + if (removedSub.isPaused && !removedSub.source.weak) + ref?._onRemoveListeners?.forEach(runGuarded); _mayNeedDispose(); + + if (wasActive && !isActive) _onCancel(); + } + + void _onSubscriptionPause({required bool weak}) { + // Weak listeners are not counted towards isActive, so we don't want to change + // _pausedActiveSubscriptionCount + if (weak) return; + + print('on pause sub at $origin'); + final wasActive = isActive; + _pausedActiveSubscriptionCount++; + + if (wasActive && !isActive) _onCancel(); + } + + void _onSubscriptionResume({ + required bool weak, + }) { + // Weak listeners are not counted towards isActive, so we don't want to change + // _pausedActiveSubscriptionCount + if (weak) return; + + print('on resume sub at $origin'); + final wasActive = isActive; + _pausedActiveSubscriptionCount = max(0, _pausedActiveSubscriptionCount - 1); + + if (!wasActive && isActive) _onResume(); + } + + void _onResume() { + print('resume $origin'); + ref?._onResumeListeners?.forEach(runGuarded); + + visitAncestors((element) { + element._onSubscriptionResume(weak: false); + }); + } + + void _onCancel() { + print('cancel $origin'); + _didCancelOnce = true; + ref?._onCancelListeners?.forEach(runGuarded); + + visitAncestors((element) { + element._onSubscriptionPause(weak: false); + }); } /// Life-cycle for when a listener is removed. @@ -584,6 +644,12 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu } } + void _closeSubscriptions(List subscriptions) { + for (var i = 0; i < subscriptions.length; i++) { + subscriptions[i].close(); + } + } + /// Executes the [Ref.onDispose] listeners previously registered, then clear /// the list of listeners. @protected @@ -595,23 +661,11 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu ref._mounted = false; - final subscriptions = _subscriptions; + final subscriptions = _previousSubscriptions = _subscriptions; + _subscriptions = null; if (subscriptions != null) { - while (subscriptions.isNotEmpty) { - late int debugPreviousLength; - if (kDebugMode) { - debugPreviousLength = subscriptions.length; - } - - final sub = subscriptions.first; - sub.close(); - - if (kDebugMode) { - assert( - subscriptions.length < debugPreviousLength, - 'ProviderSubscription.close did not remove the subscription', - ); - } + for (var i = 0; i < subscriptions.length; i++) { + subscriptions[i].pause(); } } @@ -629,7 +683,8 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu ref._onRemoveListeners = null; ref._onChangeSelfListeners = null; ref._onErrorSelfListeners = null; - _pauseCount = 0; + _pausedActiveSubscriptionCount = 0; + _didCancelOnce = false; assert( ref._keepAliveLinks == null || ref._keepAliveLinks!.isEmpty, @@ -656,11 +711,20 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu _stateResult = null; ref = null; - for (final sub in _dependencies.entries) { - sub.key._onRemoveListener(weak: false); - sub.key._watchDependents.remove(this); + final previousSubscriptions = _previousSubscriptions; + if (previousSubscriptions != null) { + _closeSubscriptions(previousSubscriptions); } - _dependencies.clear(); + + // // TODO share with _performBuild + // for (final sub in _dependencies.entries) { + // sub.key._onRemoveListener( + // () => sub.key._watchDependents.remove(this), + // weak: false, + // isPaused: !sub.key.isActive, + // ); + // } + // _dependencies.clear(); } @override @@ -699,9 +763,9 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu required void Function(ProxyElementValueListenable element) listenableVisitor, }) { - for (var i = 0; i < _watchDependents.length; i++) { - elementVisitor(_watchDependents[i]); - } + // for (var i = 0; i < _watchDependents.length; i++) { + // elementVisitor(_watchDependents[i]); + // } Iterable children = _weakDependents; if (_dependents case final dependents?) { @@ -730,7 +794,7 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu void visitAncestors( void Function(ProviderElement element) visitor, ) { - _dependencies.keys.forEach(visitor); + // _dependencies.keys.forEach(visitor); final subscriptions = _subscriptions; if (subscriptions != null) { diff --git a/packages/riverpod/lib/src/core/modifiers/select.dart b/packages/riverpod/lib/src/core/modifiers/select.dart index d8f2bd73b..3571dd8d0 100644 --- a/packages/riverpod/lib/src/core/modifiers/select.dart +++ b/packages/riverpod/lib/src/core/modifiers/select.dart @@ -191,6 +191,9 @@ final class _SelectorSubscription this.onClose, }); + @override + bool get isPaused => _internalSub.isPaused; + final ProviderSubscription _internalSub; final Output Function() _read; final void Function()? onClose; diff --git a/packages/riverpod/lib/src/core/provider/provider.dart b/packages/riverpod/lib/src/core/provider/provider.dart index cbe3ea558..d1c73aaac 100644 --- a/packages/riverpod/lib/src/core/provider/provider.dart +++ b/packages/riverpod/lib/src/core/provider/provider.dart @@ -80,13 +80,13 @@ abstract base class ProviderBase extends ProviderOrFamily // Calling before initializing the subscription, // to ensure that "hasListeners" represents the state _before_ // the listener is added - element._onListen(weak: source.weak); - - return _ProviderStateSubscription( - source, - listenedElement: element, - listener: (prev, next) => listener(prev as StateT?, next as StateT), - onError: onError, + return element._onListen( + () => _ProviderStateSubscription( + source, + listenedElement: element, + listener: (prev, next) => listener(prev as StateT?, next as StateT), + onError: onError!, + ), ); } diff --git a/packages/riverpod/lib/src/core/provider_subscription.dart b/packages/riverpod/lib/src/core/provider_subscription.dart index cadb3254a..721d89f05 100644 --- a/packages/riverpod/lib/src/core/provider_subscription.dart +++ b/packages/riverpod/lib/src/core/provider_subscription.dart @@ -12,16 +12,18 @@ abstract base class ProviderSubscription { } } + /// Whether the subscription is closed. + bool get closed => _closed; + var _closed = false; + + bool get isPaused; + /// The object that listens to the associated [ProviderListenable]. /// /// This is typically a [ProviderElement] or a [ProviderContainer], /// but may be other values in the future. final Node source; - /// Whether the subscription is closed. - bool get closed => _closed; - var _closed = false; - void pause(); void resume(); @@ -54,7 +56,7 @@ mixin _OnPauseMixin { if (_pauseCount == 0) { _onCancel(); } - _pauseCount = max(_pauseCount + 1, 0); + _pauseCount++; } @mustCallSuper @@ -89,6 +91,9 @@ final class _ProviderStateSubscription } } + @override + bool get isPaused => _isPaused; + // Why can't this be typed correctly? final void Function(Object? prev, Object? state) listener; final ProviderElement listenedElement; @@ -108,23 +113,31 @@ final class _ProviderStateSubscription @override void close() { if (!closed) { - listenedElement._onRemoveListener(weak: source.weak); - switch (source) { - case WeakNode(): - listenedElement._weakDependents.remove(this); - case _: - listenedElement._dependents?.remove(this); - } + // if (isPaused) { + // listenedElement._onSubscriptionResume(weak: source.weak); + // } + listenedElement._onRemoveListener( + () { + switch (source) { + case WeakNode(): + listenedElement._weakDependents.remove(this); + case _: + listenedElement._dependents?.remove(this); + } + + return this; + }, + ); } super.close(); } @override - void _onCancel() => listenedElement.pause(); + void _onCancel() => listenedElement._onSubscriptionPause(weak: source.weak); @override - void _onResume() => listenedElement.resume(); + void _onResume() => listenedElement._onSubscriptionResume(weak: source.weak); } /// Deals with the internals of synchronously calling the listeners diff --git a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart index 2d3d4e5bc..406a2380b 100644 --- a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart +++ b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart @@ -8,6 +8,9 @@ final class _ProxySubscription extends ProviderSubscription { required this.innerSubscription, }); + @override + bool get isPaused => innerSubscription.isPaused; + final ProviderSubscription innerSubscription; final void Function() _removeListeners; final StateT Function() _read; diff --git a/packages/riverpod/lib/src/core/ref.dart b/packages/riverpod/lib/src/core/ref.dart index 0414e284d..b7e6fce77 100644 --- a/packages/riverpod/lib/src/core/ref.dart +++ b/packages/riverpod/lib/src/core/ref.dart @@ -540,44 +540,39 @@ final = Provider(dependencies: []); /// ``` T watch(ProviderListenable listenable) { _throwIfInvalidUsage(); - if (listenable is! ProviderBase) { - final sub = _element.listen( - listenable, - (prev, value) => invalidateSelf(asReload: true), - onError: (err, stack) => invalidateSelf(asReload: true), - onDependencyMayHaveChanged: _element._markDependencyMayHaveChanged, - ); + // if (listenable is! ProviderBase) { + final sub = _element.listen( + listenable, + (prev, value) => invalidateSelf(asReload: true), + onError: (err, stack) => invalidateSelf(asReload: true), + onDependencyMayHaveChanged: _element._markDependencyMayHaveChanged, + ); - return sub.read(); - } + return sub.read(); + // } - final element = container.readProviderElement(listenable); - _element._dependencies.putIfAbsent(element, () { - final previousSub = _element._previousDependencies?.remove(element); - if (previousSub != null) { - return previousSub; - } + // final targetElement = container.readProviderElement(listenable); + // _element._dependencies.putIfAbsent(targetElement, () { + // final previousSub = _element._previousDependencies?.remove(targetElement); + // if (previousSub != null) { + // return previousSub; + // } - if (kDebugMode) { - // Flushing the provider before adding a new dependency - // as otherwise this could cause false positives with certain asserts. - // It's done only in debug mode since `readSelf` will flush the value - // again anyway, and the only value of this flush is to not break asserts. - element.flush(); - } + // targetElement + // .._onListen( + // weak: false, + // isPaused: !_element.isActive, + // ) + // .._watchDependents.add(_element); - element - .._onListen(weak: false) - .._watchDependents.add(_element); + // return Object(); + // }); - return Object(); - }); - - final result = element.readSelf(); + // final result = targetElement.readSelf(); - if (kDebugMode) _debugAssertCanDependOn(listenable); + // if (kDebugMode) _debugAssertCanDependOn(listenable); - return result; + // return result; } /// {@template riverpod.listen} diff --git a/packages/riverpod/test/src/core/provider_element_test.dart b/packages/riverpod/test/src/core/provider_element_test.dart index 9dba1fddc..62dbd24ec 100644 --- a/packages/riverpod/test/src/core/provider_element_test.dart +++ b/packages/riverpod/test/src/core/provider_element_test.dart @@ -128,6 +128,65 @@ void main() { }); group('isActive', () { + test('handles adding a listener from an already paused provider', () { + final provider = Provider((ref) => 0); + final dep = Provider((ref) { + ref.watch(provider); + }); + final container = ProviderContainer.test(); + + expect(container.readProviderElement(provider).isActive, false); + + // Using read, "dep" should still be considered as paused. + container.read(dep); + + expect(container.readProviderElement(provider).isActive, false); + }); + + test('Is paused if all watchers are paused', () { + final container = ProviderContainer.test(); + final provider = Provider(name: 'foo', (ref) => 0); + final dep = Provider(name: 'dep', (ref) => ref.watch(provider)); + final dep2 = Provider(name: 'dep2', (ref) => ref.watch(provider)); + + final depSub = container.listen(dep, (a, b) {}); + final dep2Sub = container.listen(dep2, (a, b) {}); + + final element = container.readProviderElement(provider); + final depElement = container.readProviderElement(dep); + final depElement2 = container.readProviderElement(dep2); + + expect(element.isActive, true); + + depSub.close(); + + expect(element.isActive, true); + + dep2Sub.close(); + + expect(element.isActive, false); + }); + + test('Is paused if all subscriptions are paused', () { + final container = ProviderContainer.test(); + final provider = Provider((ref) => 0); + + final element = container.readProviderElement(provider); + + final sub = container.listen(provider, (_, __) {}); + final sub2 = container.listen(provider, (_, __) {}); + + expect(element.isActive, true); + + sub.pause(); + + expect(element.isActive, true); + + sub2.pause(); + + expect(element.isActive, false); + }); + test('rejects weak listeners', () { final provider = Provider((ref) => 0); final container = ProviderContainer.test(); @@ -164,7 +223,7 @@ void main() { expect(container.readProviderElement(provider).isActive, false); - container.read(dep); + container.listen(dep, (_, __) {}); expect(container.readProviderElement(provider).isActive, true); }); @@ -235,6 +294,22 @@ void main() { }); }); + test('does not notify listeners twice when using fireImmediately', + () async { + final container = ProviderContainer.test(); + final listener = Listener(); + + final dep = StateProvider((ref) => 0); + final provider = Provider((ref) { + ref.watch(dep); + return ref.state = 0; + }); + + container.listen(provider, listener.call, fireImmediately: true); + + verifyOnly(listener, listener(null, 0)); + }); + test('does not notify listeners when rebuilding the state', () async { final container = ProviderContainer.test(); final listener = Listener(); diff --git a/packages/riverpod/test/src/core/provider_subscription_test.dart b/packages/riverpod/test/src/core/provider_subscription_test.dart index 2fc7c799f..93d097a1e 100644 --- a/packages/riverpod/test/src/core/provider_subscription_test.dart +++ b/packages/riverpod/test/src/core/provider_subscription_test.dart @@ -21,5 +21,9 @@ void main() { expect(element.isActive, true); }); + + test('closing a paused subscription unpause the element', () { + throw UnimplementedError(); + }); }); } diff --git a/packages/riverpod/test/src/core/ref_test.dart b/packages/riverpod/test/src/core/ref_test.dart index 79ce4eb2b..ba8f8f16b 100644 --- a/packages/riverpod/test/src/core/ref_test.dart +++ b/packages/riverpod/test/src/core/ref_test.dart @@ -852,7 +852,6 @@ void main() { if (throws) throw StateError('err'); }); - final errors = []; final sub = container.listen( provider, (a, b) {}, @@ -864,19 +863,12 @@ void main() { verifyZeroInteractions(listener); throws = true; - runZonedGuarded( - () { - try { - container.refresh(provider); - } catch (e) { - // We just want to trigger onError listener - } - }, - (error, stack) => errors.add(error), - ); + try { + container.refresh(provider); + } catch (e) { + // We just want to trigger onError listener + } verifyZeroInteractions(listener); - - expect(errors, [isA()]); }); group('weak', () { @@ -2382,6 +2374,8 @@ void main() { ref.watch(dep); + // TODO changelog breaking: Calling ref.watch multiple times calls ref.onListen everytime + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); @@ -2738,19 +2732,21 @@ void main() { final container = ProviderContainer.test(); final listener = OnCancelMock(); final listener2 = OnCancelMock(); - final dep = Provider((ref) { + final dep = Provider(name: 'dep', (ref) { ref.onCancel(listener.call); ref.onCancel(listener2.call); }); var watching = true; - final provider = Provider((ref) { + final provider = Provider(name: 'provider', (ref) { if (watching) ref.watch(dep); }); - final provider2 = Provider((ref) { + final provider2 = Provider(name: 'provider2', (ref) { if (watching) ref.watch(dep); }); + print('a'); container.read(provider); + print('b'); container.read(provider2); verifyZeroInteractions(listener); @@ -2758,12 +2754,16 @@ void main() { watching = false; // remove the dependency provider<>dep + print('c'); container.refresh(provider); + print('d'); verifyZeroInteractions(listener2); // remove the dependency provider2<>dep + print('e'); container.refresh(provider2); + print('f'); verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); diff --git a/packages/riverpod/test/src/providers/stream_notifier_test.dart b/packages/riverpod/test/src/providers/stream_notifier_test.dart index 89e4e40a5..f0aa149af 100644 --- a/packages/riverpod/test/src/providers/stream_notifier_test.dart +++ b/packages/riverpod/test/src/providers/stream_notifier_test.dart @@ -29,6 +29,10 @@ void main() { }); streamNotifierProviderFactory.createGroup((factory) { + test('Pauses the Stream when the provider is paused', () { + throw UnimplementedError(); + }); + test('Cannot share a Notifier instance between providers ', () { final container = ProviderContainer.test(); final notifier = factory.deferredNotifier((ref) => Stream.value(0)); From bb8a0b2f7e7cd8aa669891e4354c8d4c774b11b1 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sat, 31 Aug 2024 15:20:07 +0200 Subject: [PATCH 08/27] Fix some tests --- packages/riverpod/lib/src/core/element.dart | 11 ++--------- .../riverpod/lib/src/core/provider_subscription.dart | 3 --- packages/riverpod/lib/src/core/scheduler.dart | 2 +- .../test/old/framework/auto_dispose_test.dart | 4 ++-- packages/riverpod/test/old/utils.dart | 9 +++++++++ 5 files changed, 14 insertions(+), 15 deletions(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index e94e2e8dc..2e21d901c 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -564,7 +564,6 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu } SubT _onListen(SubT Function() add) { - print('listen $origin'); final wasActive = isActive; ref?._onAddListeners?.forEach(runGuarded); @@ -577,12 +576,10 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu } void _onRemoveListener(ProviderSubscription Function() remove) { - print('unsub $origin'); final wasActive = isActive; - final removedSub = remove(); - if (removedSub.isPaused && !removedSub.source.weak) - ref?._onRemoveListeners?.forEach(runGuarded); + remove(); + ref?._onRemoveListeners?.forEach(runGuarded); _mayNeedDispose(); if (wasActive && !isActive) _onCancel(); @@ -593,7 +590,6 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu // _pausedActiveSubscriptionCount if (weak) return; - print('on pause sub at $origin'); final wasActive = isActive; _pausedActiveSubscriptionCount++; @@ -607,7 +603,6 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu // _pausedActiveSubscriptionCount if (weak) return; - print('on resume sub at $origin'); final wasActive = isActive; _pausedActiveSubscriptionCount = max(0, _pausedActiveSubscriptionCount - 1); @@ -615,7 +610,6 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu } void _onResume() { - print('resume $origin'); ref?._onResumeListeners?.forEach(runGuarded); visitAncestors((element) { @@ -624,7 +618,6 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu } void _onCancel() { - print('cancel $origin'); _didCancelOnce = true; ref?._onCancelListeners?.forEach(runGuarded); diff --git a/packages/riverpod/lib/src/core/provider_subscription.dart b/packages/riverpod/lib/src/core/provider_subscription.dart index 721d89f05..327678e6c 100644 --- a/packages/riverpod/lib/src/core/provider_subscription.dart +++ b/packages/riverpod/lib/src/core/provider_subscription.dart @@ -113,9 +113,6 @@ final class _ProviderStateSubscription @override void close() { if (!closed) { - // if (isPaused) { - // listenedElement._onSubscriptionResume(weak: source.weak); - // } listenedElement._onRemoveListener( () { switch (source) { diff --git a/packages/riverpod/lib/src/core/scheduler.dart b/packages/riverpod/lib/src/core/scheduler.dart index dcba47849..80476dac8 100644 --- a/packages/riverpod/lib/src/core/scheduler.dart +++ b/packages/riverpod/lib/src/core/scheduler.dart @@ -125,7 +125,7 @@ class ProviderScheduler { if ((links != null && links.isNotEmpty) || element.container._disposed || - element.isActive) { + element.hasListeners) { continue; } diff --git a/packages/riverpod/test/old/framework/auto_dispose_test.dart b/packages/riverpod/test/old/framework/auto_dispose_test.dart index 3e3e39a5d..3efe13deb 100644 --- a/packages/riverpod/test/old/framework/auto_dispose_test.dart +++ b/packages/riverpod/test/old/framework/auto_dispose_test.dart @@ -96,12 +96,12 @@ Future main() async { test('unsub to A then make B sub to A then unsub to B disposes B before A', () async { final container = ProviderContainer.test(); - final aDispose = OnDisposeMock(); + final aDispose = OnDisposeMock('a'); final a = Provider.autoDispose((ref) { ref.onDispose(aDispose.call); return 42; }); - final bDispose = OnDisposeMock(); + final bDispose = OnDisposeMock('b'); final b = Provider.autoDispose((ref) { ref.onDispose(bDispose.call); ref.watch(a); diff --git a/packages/riverpod/test/old/utils.dart b/packages/riverpod/test/old/utils.dart index c29018eaf..f7839184e 100644 --- a/packages/riverpod/test/old/utils.dart +++ b/packages/riverpod/test/old/utils.dart @@ -36,7 +36,16 @@ class OnBuildMock extends Mock { } class OnDisposeMock extends Mock { + OnDisposeMock([this.label]); + + final String? label; + void call(); + + @override + String toString() { + return 'OnDisposeMock($label)'; + } } class OnCancelMock extends Mock { From e9f132d4dce2f83f808023db03c642a88d85344e Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sat, 31 Aug 2024 17:11:52 +0200 Subject: [PATCH 09/27] More tests --- packages/riverpod/lib/src/core/element.dart | 35 ++----------------- .../lib/src/core/modifiers/future.dart | 1 - .../test/old/legacy/framework_test.dart | 2 +- packages/riverpod/test/src/core/ref_test.dart | 6 ---- 4 files changed, 4 insertions(+), 40 deletions(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index 2e21d901c..bac191623 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -79,7 +79,6 @@ abstract class ProviderElement implements WrappedNode { bool get isActive => (_listenerCount - _pausedActiveSubscriptionCount) > 0; int get _listenerCount => _dependents?.length ?? 0; - // (_dependents?.length ?? 0) + _watchDependents.length; var _pausedActiveSubscriptionCount = 0; var _didCancelOnce = false; @@ -249,13 +248,6 @@ This could mean a few things: /// After a provider is initialized, this function takes care of unsubscribing /// to dependencies that are no-longer used. void _performBuild() { - // assert( - // _previousDependencies == null, - // 'Bad state: _performBuild was called twice', - // ); - // final previousDependencies = _previousDependencies = _dependencies; - // _dependencies = HashMap(); - runOnDispose(); final ref = this.ref = Ref._(this); final previousStateResult = _stateResult; @@ -278,16 +270,6 @@ This could mean a few things: if (previousSubscriptions != null) { _closeSubscriptions(previousSubscriptions); } - - // // Unsubscribe to everything that a provider no longer depends on. - // for (final sub in previousDependencies.entries) { - // sub.key._onRemoveListener( - // () => sub.key._watchDependents.remove(this), - // weak: false, - // isPaused: !isActive, - // ); - // } - // _previousDependencies = null; } /// Initialize a provider. @@ -704,20 +686,9 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu _stateResult = null; ref = null; - final previousSubscriptions = _previousSubscriptions; - if (previousSubscriptions != null) { - _closeSubscriptions(previousSubscriptions); - } - - // // TODO share with _performBuild - // for (final sub in _dependencies.entries) { - // sub.key._onRemoveListener( - // () => sub.key._watchDependents.remove(this), - // weak: false, - // isPaused: !sub.key.isActive, - // ); - // } - // _dependencies.clear(); + if (_previousSubscriptions case final subs?) _closeSubscriptions(subs); + if (_dependents case final subs?) _closeSubscriptions(subs); + _closeSubscriptions(_weakDependents); } @override diff --git a/packages/riverpod/lib/src/core/modifiers/future.dart b/packages/riverpod/lib/src/core/modifiers/future.dart index 5a1a8981e..2a6ea31b5 100644 --- a/packages/riverpod/lib/src/core/modifiers/future.dart +++ b/packages/riverpod/lib/src/core/modifiers/future.dart @@ -411,7 +411,6 @@ mixin FutureModifierElement on ProviderElement> { /// Listens to a [Future] and transforms it into an [AsyncValue]. void _handleAsync( - // Stream Function({required void Function(T) fireImmediately}) create, CancelAsyncSubscription? Function({ required void Function(StateT) data, required void Function(Object, StackTrace) error, diff --git a/packages/riverpod/test/old/legacy/framework_test.dart b/packages/riverpod/test/old/legacy/framework_test.dart index c896f1cf2..3108ddd89 100644 --- a/packages/riverpod/test/old/legacy/framework_test.dart +++ b/packages/riverpod/test/old/legacy/framework_test.dart @@ -74,7 +74,7 @@ void main() { final container = ProviderContainer.test(); expect( () => container.read(provider), - throwsA(isA()), + throwsA(isA()), ); }); diff --git a/packages/riverpod/test/src/core/ref_test.dart b/packages/riverpod/test/src/core/ref_test.dart index ba8f8f16b..66c874699 100644 --- a/packages/riverpod/test/src/core/ref_test.dart +++ b/packages/riverpod/test/src/core/ref_test.dart @@ -2744,9 +2744,7 @@ void main() { if (watching) ref.watch(dep); }); - print('a'); container.read(provider); - print('b'); container.read(provider2); verifyZeroInteractions(listener); @@ -2754,16 +2752,12 @@ void main() { watching = false; // remove the dependency provider<>dep - print('c'); container.refresh(provider); - print('d'); verifyZeroInteractions(listener2); // remove the dependency provider2<>dep - print('e'); container.refresh(provider2); - print('f'); verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); From 2615056dad02b51c7d6cdff483b5d535e342b2a8 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 1 Sep 2024 16:51:38 +0200 Subject: [PATCH 10/27] W --- packages/riverpod/lib/src/core/element.dart | 88 +++++++++---------- .../lib/src/core/modifiers/select.dart | 4 + .../lib/src/core/provider/provider.dart | 16 ++-- .../lib/src/core/provider_subscription.dart | 26 +++--- .../src/core/proxy_provider_listenable.dart | 2 +- packages/riverpod/lib/src/core/ref.dart | 27 +----- .../test/src/core/modifiers/future_test.dart | 2 +- .../test/src/core/modifiers/select_test.dart | 4 +- .../src/core/provider_subscription_test.dart | 20 ++++- packages/riverpod/test/src/core/ref_test.dart | 20 +++-- 10 files changed, 104 insertions(+), 105 deletions(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index bac191623..fb0234f35 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -75,7 +75,7 @@ abstract class ProviderElement implements WrappedNode { /// - it has no listeners /// - all of its listeners are "weak" (i.e. created with `listen(weak: true)`) /// - /// See also [_mayNeedDispose], called when [isActive] may have changed. + /// See also [mayNeedDispose], called when [isActive] may have changed. bool get isActive => (_listenerCount - _pausedActiveSubscriptionCount) > 0; int get _listenerCount => _dependents?.length ?? 0; @@ -87,10 +87,8 @@ abstract class ProviderElement implements WrappedNode { /// /// This maps to listeners added with `listen` and `watch`, /// excluding `listen(weak: true)`. - bool get hasListeners => _listenerCount > 0 || _weakDependents.isNotEmpty; + bool get hasListeners => _listenerCount > 0; - // var _dependencies = HashMap(); - // HashMap? _previousDependencies; List? _previousSubscriptions; List? _subscriptions; List? _dependents; @@ -102,9 +100,6 @@ abstract class ProviderElement implements WrappedNode { /// - They may be reused between two instances of a [ProviderElement]. final _weakDependents = []; - // /// The element of the providers that depends on this provider. - // final _watchDependents = []; - bool _mustRecomputeState = false; bool _dependencyMayHaveChanged = false; bool _didChangeDependency = false; @@ -374,7 +369,7 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu _mustRecomputeState = true; runOnDispose(); - _mayNeedDispose(); + mayNeedDispose(); container.scheduler.scheduleProviderRefresh(this); // We don't call this._markDependencyMayHaveChanged here because we voluntarily @@ -550,66 +545,71 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu ref?._onAddListeners?.forEach(runGuarded); final sub = add(); - if (_didCancelOnce && !wasActive && isActive) { - _onResume(); - } + if (sub.isPaused) onSubscriptionPause(weak: sub.source.weak); + + if (_didCancelOnce && !wasActive && isActive) _onResume(); return sub; } - void _onRemoveListener(ProviderSubscription Function() remove) { + void onChangeSubscription(void Function() apply) { final wasActive = isActive; + final previousListenerCount = _listenerCount; + apply(); - remove(); - ref?._onRemoveListeners?.forEach(runGuarded); - _mayNeedDispose(); + switch ((wasActive: wasActive, isActive: isActive)) { + case (wasActive: false, isActive: true) when _didCancelOnce: + ref?._onResumeListeners?.forEach(runGuarded); + visitAncestors((element) => element.onSubscriptionResume(weak: false)); - if (wasActive && !isActive) _onCancel(); - } + case (wasActive: true, isActive: false): + _didCancelOnce = true; + ref?._onCancelListeners?.forEach(runGuarded); + visitAncestors((element) => element.onSubscriptionPause(weak: false)); - void _onSubscriptionPause({required bool weak}) { - // Weak listeners are not counted towards isActive, so we don't want to change - // _pausedActiveSubscriptionCount - if (weak) return; + default: + // No state change, so do nothing + } - final wasActive = isActive; - _pausedActiveSubscriptionCount++; + if (_listenerCount < previousListenerCount) { + _onRemoveListener(); + } else if (_listenerCount > previousListenerCount) { + _onAddListener(); + } + } - if (wasActive && !isActive) _onCancel(); + void _onRemoveListener() { + ref?._onRemoveListeners?.forEach(runGuarded); + mayNeedDispose(); } - void _onSubscriptionResume({ - required bool weak, - }) { + void _onAddListener() => ref?._onAddListeners?.forEach(runGuarded); + + void onSubscriptionPause({required bool weak}) { // Weak listeners are not counted towards isActive, so we don't want to change // _pausedActiveSubscriptionCount if (weak) return; - final wasActive = isActive; - _pausedActiveSubscriptionCount = max(0, _pausedActiveSubscriptionCount - 1); - - if (!wasActive && isActive) _onResume(); - } - - void _onResume() { - ref?._onResumeListeners?.forEach(runGuarded); - - visitAncestors((element) { - element._onSubscriptionResume(weak: false); + onChangeSubscription(() { + _pausedActiveSubscriptionCount++; }); } - void _onCancel() { - _didCancelOnce = true; - ref?._onCancelListeners?.forEach(runGuarded); + void onSubscriptionResume({required bool weak}) { + // Weak listeners are not counted towards isActive, so we don't want to change + // _pausedActiveSubscriptionCount + if (weak) return; - visitAncestors((element) { - element._onSubscriptionPause(weak: false); + onChangeSubscription(() { + _pausedActiveSubscriptionCount = max( + 0, + _pausedActiveSubscriptionCount - 1, + ); }); } /// Life-cycle for when a listener is removed. - void _mayNeedDispose() { + void mayNeedDispose() { if (provider.isAutoDispose) { final links = ref?._keepAliveLinks; diff --git a/packages/riverpod/lib/src/core/modifiers/select.dart b/packages/riverpod/lib/src/core/modifiers/select.dart index 3571dd8d0..35f1e93b8 100644 --- a/packages/riverpod/lib/src/core/modifiers/select.dart +++ b/packages/riverpod/lib/src/core/modifiers/select.dart @@ -128,12 +128,14 @@ class _ProviderSelector with ProviderListenable { required void Function()? onDependencyMayHaveChanged, required bool fireImmediately, }) { + print('setup selector on ${this.provider}'); onError ??= Zone.current.handleUncaughtError; Result? lastSelectedValue; final sub = node.listen( provider, (prev, input) { + print('change for ${provider}'); _selectOnChange( newState: input, lastSelectedValue: lastSelectedValue, @@ -146,6 +148,8 @@ class _ProviderSelector with ProviderListenable { onError: onError, ); + print(sub.isPaused); + if (!node.weak) lastSelectedValue = _select(Result.guard(sub.read)); if (fireImmediately) { diff --git a/packages/riverpod/lib/src/core/provider/provider.dart b/packages/riverpod/lib/src/core/provider/provider.dart index d1c73aaac..c66e4aaa1 100644 --- a/packages/riverpod/lib/src/core/provider/provider.dart +++ b/packages/riverpod/lib/src/core/provider/provider.dart @@ -81,12 +81,14 @@ abstract base class ProviderBase extends ProviderOrFamily // to ensure that "hasListeners" represents the state _before_ // the listener is added return element._onListen( - () => _ProviderStateSubscription( - source, - listenedElement: element, - listener: (prev, next) => listener(prev as StateT?, next as StateT), - onError: onError!, - ), + () { + return _ProviderStateSubscription( + source, + listenedElement: element, + listener: (prev, next) => listener(prev as StateT?, next as StateT), + onError: onError!, + ); + }, ); } @@ -97,7 +99,7 @@ abstract base class ProviderBase extends ProviderOrFamily element.flush(); // In case `read` was called on a provider that has no listener - element._mayNeedDispose(); + element.mayNeedDispose(); return element.requireState; } diff --git a/packages/riverpod/lib/src/core/provider_subscription.dart b/packages/riverpod/lib/src/core/provider_subscription.dart index 327678e6c..6a71fc535 100644 --- a/packages/riverpod/lib/src/core/provider_subscription.dart +++ b/packages/riverpod/lib/src/core/provider_subscription.dart @@ -106,35 +106,31 @@ final class _ProviderStateSubscription 'called ProviderSubscription.read on a subscription that was closed', ); } - listenedElement._mayNeedDispose(); + listenedElement.mayNeedDispose(); return listenedElement.readSelf(); } @override void close() { if (!closed) { - listenedElement._onRemoveListener( - () { - switch (source) { - case WeakNode(): - listenedElement._weakDependents.remove(this); - case _: - listenedElement._dependents?.remove(this); - } - - return this; - }, - ); + listenedElement.onChangeSubscription(() { + switch (source) { + case WeakNode(): + listenedElement._weakDependents.remove(this); + case _: + listenedElement._dependents?.remove(this); + } + }); } super.close(); } @override - void _onCancel() => listenedElement._onSubscriptionPause(weak: source.weak); + void _onCancel() => listenedElement.onSubscriptionPause(weak: source.weak); @override - void _onResume() => listenedElement._onSubscriptionResume(weak: source.weak); + void _onResume() => listenedElement.onSubscriptionResume(weak: source.weak); } /// Deals with the internals of synchronously calling the listeners diff --git a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart index 406a2380b..4fa432986 100644 --- a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart +++ b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart @@ -138,7 +138,7 @@ class ProviderElementProxy OutputT read(Node node) { final element = node.readProviderElement(provider); element.flush(); - element._mayNeedDispose(); + element.mayNeedDispose(); return _lense(element).value; } diff --git a/packages/riverpod/lib/src/core/ref.dart b/packages/riverpod/lib/src/core/ref.dart index b7e6fce77..eadd37721 100644 --- a/packages/riverpod/lib/src/core/ref.dart +++ b/packages/riverpod/lib/src/core/ref.dart @@ -162,7 +162,7 @@ final = Provider(dependencies: []); late KeepAliveLink link; link = KeepAliveLink._(() { if (links.remove(link)) { - if (links.isEmpty) _element._mayNeedDispose(); + if (links.isEmpty) _element.mayNeedDispose(); } }); links.add(link); @@ -540,7 +540,6 @@ final = Provider(dependencies: []); /// ``` T watch(ProviderListenable listenable) { _throwIfInvalidUsage(); - // if (listenable is! ProviderBase) { final sub = _element.listen( listenable, (prev, value) => invalidateSelf(asReload: true), @@ -549,30 +548,6 @@ final = Provider(dependencies: []); ); return sub.read(); - // } - - // final targetElement = container.readProviderElement(listenable); - // _element._dependencies.putIfAbsent(targetElement, () { - // final previousSub = _element._previousDependencies?.remove(targetElement); - // if (previousSub != null) { - // return previousSub; - // } - - // targetElement - // .._onListen( - // weak: false, - // isPaused: !_element.isActive, - // ) - // .._watchDependents.add(_element); - - // return Object(); - // }); - - // final result = targetElement.readSelf(); - - // if (kDebugMode) _debugAssertCanDependOn(listenable); - - // return result; } /// {@template riverpod.listen} diff --git a/packages/riverpod/test/src/core/modifiers/future_test.dart b/packages/riverpod/test/src/core/modifiers/future_test.dart index daedb4fe5..9a64606ae 100644 --- a/packages/riverpod/test/src/core/modifiers/future_test.dart +++ b/packages/riverpod/test/src/core/modifiers/future_test.dart @@ -17,7 +17,7 @@ void main() { (previous, value) {}, ); - expect(container.readProviderElement(provider).hasListeners, true); + expect(container.readProviderElement(provider).hasListeners, false); sub.close(); diff --git a/packages/riverpod/test/src/core/modifiers/select_test.dart b/packages/riverpod/test/src/core/modifiers/select_test.dart index a3ed9d935..1dbe8b6f1 100644 --- a/packages/riverpod/test/src/core/modifiers/select_test.dart +++ b/packages/riverpod/test/src/core/modifiers/select_test.dart @@ -53,7 +53,7 @@ void main() { (previous, value) {}, ); - expect(container.readProviderElement(provider).hasListeners, true); + expect(container.readProviderElement(provider).hasListeners, false); sub.close(); @@ -78,6 +78,8 @@ void main() { (previous, value) {}, ); + print(element.hasListeners); + expect(sub.read(), 0); verifyZeroInteractions(onDispose); diff --git a/packages/riverpod/test/src/core/provider_subscription_test.dart b/packages/riverpod/test/src/core/provider_subscription_test.dart index 93d097a1e..6c8ed053b 100644 --- a/packages/riverpod/test/src/core/provider_subscription_test.dart +++ b/packages/riverpod/test/src/core/provider_subscription_test.dart @@ -22,8 +22,24 @@ void main() { expect(element.isActive, true); }); - test('closing a paused subscription unpause the element', () { - throw UnimplementedError(); + test('closing a paused subscription unpauses the element', () { + final container = ProviderContainer.test(); + final provider = FutureProvider((ref) => 0); + + final element = container.readProviderElement(provider); + + final sub = container.listen(provider.future, (previous, next) {}); + + expect(element.isActive, true); + + sub.pause(); + + expect(element.isActive, false); + + sub.close(); + container.listen(provider.future, (previous, next) {}); + + expect(element.isActive, true); }); }); } diff --git a/packages/riverpod/test/src/core/ref_test.dart b/packages/riverpod/test/src/core/ref_test.dart index 66c874699..e85d46cdc 100644 --- a/packages/riverpod/test/src/core/ref_test.dart +++ b/packages/riverpod/test/src/core/ref_test.dart @@ -963,18 +963,22 @@ void main() { expect(buildCount, 1); }); - test('closing the subscription updated element.hasListeners', () { + test('closing the subscription removes the listener', () { final container = ProviderContainer.test(); - final provider = Provider((ref) => 0); - - final sub = - container.listen(provider, weak: true, (previous, value) {}); - - expect(container.readProviderElement(provider).hasListeners, true); + final provider = Provider((ref) => Object()); + final listener = Listener(); + final sub = container.listen( + provider, + weak: true, + listener.call, + ); sub.close(); - expect(container.readProviderElement(provider).hasListeners, false); + container.read(provider); + container.refresh(provider); + + verifyZeroInteractions(listener); }); test('does not count towards the pause mechanism', () async { From 8723f8f324d9146e14d09797690573be135cb609 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sat, 7 Sep 2024 19:15:22 +0200 Subject: [PATCH 11/27] Restore most tests --- packages/riverpod/lib/src/core/element.dart | 231 +++++++++--------- .../riverpod/lib/src/core/foundation.dart | 6 +- .../lib/src/core/modifiers/select.dart | 111 ++------- .../lib/src/core/modifiers/select_async.dart | 31 ++- .../lib/src/core/provider/provider.dart | 37 +-- .../lib/src/core/provider_container.dart | 23 +- .../lib/src/core/provider_subscription.dart | 206 +++++++++++----- .../src/core/proxy_provider_listenable.dart | 65 +++-- packages/riverpod/lib/src/core/ref.dart | 16 +- packages/riverpod/lib/src/core/scheduler.dart | 4 +- .../test/old/framework/auto_dispose_test.dart | 7 +- .../test/old/framework/modifiers_test.dart | 12 +- .../test/old/framework/ref_watch_test.dart | 10 +- packages/riverpod/test/old/utils.dart | 4 +- .../test/src/core/modifiers/future_test.dart | 2 +- .../test/src/core/modifiers/select_test.dart | 4 +- .../src/core/provider_container_test.dart | 107 +------- .../test/src/core/provider_element_test.dart | 14 +- packages/riverpod/test/src/core/ref_test.dart | 128 ++++------ .../src/providers/stream_notifier_test.dart | 4 +- packages/riverpod/test/src/utils.dart | 4 +- 21 files changed, 475 insertions(+), 551 deletions(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index fb0234f35..a2e04dbef 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -42,7 +42,7 @@ void Function()? debugCanModifyProviders; /// {@endtemplate} @internal @optionalTypeArgs -abstract class ProviderElement implements WrappedNode { +abstract class ProviderElement implements Node { /// {@macro riverpod.provider_element_base} ProviderElement(this.pointer); @@ -55,8 +55,7 @@ abstract class ProviderElement implements WrappedNode { var _debugSkipNotifyListenersAsserts = false; /// The provider associated with this [ProviderElement], before applying overrides. - // Not typed as because of https://github.com/rrousselGit/riverpod/issues/1100 - ProviderBase get origin => pointer.origin; + ProviderBase get origin => pointer.origin as ProviderBase; /// The provider associated with this [ProviderElement], after applying overrides. ProviderBase get provider; @@ -78,7 +77,7 @@ abstract class ProviderElement implements WrappedNode { /// See also [mayNeedDispose], called when [isActive] may have changed. bool get isActive => (_listenerCount - _pausedActiveSubscriptionCount) > 0; - int get _listenerCount => _dependents?.length ?? 0; + int get _listenerCount => dependents?.length ?? 0; var _pausedActiveSubscriptionCount = 0; var _didCancelOnce = false; @@ -86,19 +85,21 @@ abstract class ProviderElement implements WrappedNode { /// Whether this [ProviderElement] is currently listened to or not. /// /// This maps to listeners added with `listen` and `watch`, - /// excluding `listen(weak: true)`. - bool get hasListeners => _listenerCount > 0; + /// including `listen(weak: true)`. + bool get hasListeners => _listenerCount > 0 || weakDependents.isNotEmpty; - List? _previousSubscriptions; - List? _subscriptions; - List? _dependents; + @visibleForTesting + List? subscriptions; + @visibleForTesting + List? dependents; /// "listen(weak: true)" pointing to this provider. /// - /// Those subscriptions are separate from [ProviderElement._dependents] for a few reasons: + /// Those subscriptions are separate from [ProviderElement.dependents] for a few reasons: /// - They do not count towards [ProviderElement.isActive]. /// - They may be reused between two instances of a [ProviderElement]. - final _weakDependents = []; + @visibleForTesting + final weakDependents = []; bool _mustRecomputeState = false; bool _dependencyMayHaveChanged = false; @@ -260,11 +261,6 @@ This could mean a few things: if (kDebugMode) _debugSkipNotifyListenersAsserts = false; } - - final previousSubscriptions = _previousSubscriptions; - if (previousSubscriptions != null) { - _closeSubscriptions(previousSubscriptions); - } } /// Initialize a provider. @@ -430,40 +426,33 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu return; } - final listeners = [..._weakDependents, if (!isMount) ...?_dependents]; + final listeners = [...weakDependents, if (!isMount) ...?dependents]; switch (newState) { case final ResultData newState: for (var i = 0; i < listeners.length; i++) { final listener = listeners[i]; - if (listener is _ProviderStateSubscription && !listener._isPaused) { - Zone.current.runBinaryGuarded( - listener.listener, - previousState, - newState.state, - ); - } + if (listener.closed) continue; + + Zone.current.runBinaryGuarded( + listener._notify, + previousState, + newState.state, + ); } case final ResultError newState: for (var i = 0; i < listeners.length; i++) { final listener = listeners[i]; - if (listener is _ProviderStateSubscription && - !listener._isPaused) { - Zone.current.runBinaryGuarded( - listener.onError, - newState.error, - newState.stackTrace, - ); - } + if (listener.closed) continue; + + Zone.current.runBinaryGuarded( + listener._notifyError, + newState.error, + newState.stackTrace, + ); } default: } - // if (!isMount) { - // for (var i = 0; i < _watchDependents.length; i++) { - // _watchDependents[i].invalidateSelf(asReload: true); - // } - // } - for (final observer in container.observers) { if (isMount) { runTernaryGuarded( @@ -514,7 +503,6 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu return container.readProviderElement(provider); } - @override ProviderSubscription listen( ProviderListenable listenable, void Function(T? previous, T value) listener, { @@ -527,32 +515,60 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu final ref = this.ref!; ref._throwIfInvalidUsage(); - final result = listenable.addListener( - weak ? WeakNode(this) : this, + final sub = listenable.addListener( + this, listener, fireImmediately: fireImmediately, onError: onError, + weak: weak, onDependencyMayHaveChanged: onDependencyMayHaveChanged, ); + switch (sub) { + case ProviderSubscriptionImpl(): + sub._listenedElement.addDependentSubscription(sub); + } + if (kDebugMode) ref._debugAssertCanDependOn(listenable); - return result; + return sub; } - SubT _onListen(SubT Function() add) { - final wasActive = isActive; - ref?._onAddListeners?.forEach(runGuarded); + void addDependentSubscription(ProviderSubscriptionImpl sub) { + _onChangeSubscription(sub, () { + if (sub.weak) { + weakDependents.add(sub); + } else { + final dependents = this.dependents ??= []; + dependents.add(sub); + } + + if (sub.source case final ProviderElement element) { + final subs = element.subscriptions ??= []; + subs.add(sub); + } + }); + } - final sub = add(); - if (sub.isPaused) onSubscriptionPause(weak: sub.source.weak); + void removeDependentSubscription(ProviderSubscription sub) { + while (sub.isPaused) { + sub.resume(); + } - if (_didCancelOnce && !wasActive && isActive) _onResume(); + _onChangeSubscription(sub, () { + if (sub.weak) { + weakDependents.remove(sub); + } else { + dependents?.remove(sub); + } - return sub; + if (sub.source case final ProviderElement element) { + element.subscriptions?.remove(sub); + } + }); } - void onChangeSubscription(void Function() apply) { + void _onChangeSubscription(ProviderSubscription sub, void Function() apply) { final wasActive = isActive; final previousListenerCount = _listenerCount; apply(); @@ -560,47 +576,48 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu switch ((wasActive: wasActive, isActive: isActive)) { case (wasActive: false, isActive: true) when _didCancelOnce: ref?._onResumeListeners?.forEach(runGuarded); - visitAncestors((element) => element.onSubscriptionResume(weak: false)); + + // _subscriptions?.forEach((sub) => sub.resume()); case (wasActive: true, isActive: false): _didCancelOnce = true; ref?._onCancelListeners?.forEach(runGuarded); - visitAncestors((element) => element.onSubscriptionPause(weak: false)); + // _subscriptions?.forEach((sub) => sub.pause()); default: // No state change, so do nothing } if (_listenerCount < previousListenerCount) { - _onRemoveListener(); + if (ref?._onRemoveListeners case final listeners?) { + for (final listener in listeners) { + runUnaryGuarded(listener, sub); + } + } + mayNeedDispose(); } else if (_listenerCount > previousListenerCount) { - _onAddListener(); + if (ref?._onAddListeners case final listeners?) { + for (final listener in listeners) { + runUnaryGuarded(listener, sub); + } + } } } - void _onRemoveListener() { - ref?._onRemoveListeners?.forEach(runGuarded); - mayNeedDispose(); - } - - void _onAddListener() => ref?._onAddListeners?.forEach(runGuarded); - - void onSubscriptionPause({required bool weak}) { + void onSubscriptionPause(ProviderSubscription sub) { // Weak listeners are not counted towards isActive, so we don't want to change // _pausedActiveSubscriptionCount - if (weak) return; + if (sub.weak) return; - onChangeSubscription(() { - _pausedActiveSubscriptionCount++; - }); + _onChangeSubscription(sub, () => _pausedActiveSubscriptionCount++); } - void onSubscriptionResume({required bool weak}) { + void onSubscriptionResume(ProviderSubscription sub) { // Weak listeners are not counted towards isActive, so we don't want to change // _pausedActiveSubscriptionCount - if (weak) return; + if (sub.weak) return; - onChangeSubscription(() { + _onChangeSubscription(sub, () { _pausedActiveSubscriptionCount = max( 0, _pausedActiveSubscriptionCount - 1, @@ -620,8 +637,9 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu } void _closeSubscriptions(List subscriptions) { - for (var i = 0; i < subscriptions.length; i++) { - subscriptions[i].close(); + final subs = subscriptions.toList(); + for (var i = 0; i < subs.length; i++) { + subs[i].close(); } } @@ -636,13 +654,8 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu ref._mounted = false; - final subscriptions = _previousSubscriptions = _subscriptions; - _subscriptions = null; - if (subscriptions != null) { - for (var i = 0; i < subscriptions.length; i++) { - subscriptions[i].pause(); - } - } + if (subscriptions case final subs?) _closeSubscriptions(subs); + subscriptions = null; ref._onDisposeListeners?.forEach(runGuarded); @@ -658,7 +671,6 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu ref._onRemoveListeners = null; ref._onChangeSelfListeners = null; ref._onErrorSelfListeners = null; - _pausedActiveSubscriptionCount = 0; _didCancelOnce = false; assert( @@ -667,6 +679,21 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu ); } + /// Clears the state of a [ProviderElement]. + /// + /// This is in-between [dispose] and [runOnDispose]. + /// It is used to clear the state of a provider, without unsubscribing + /// [weakDependents]. + @mustCallSuper + void clearState() { + runOnDispose(); + _didMount = false; + _stateResult = null; + ref = null; + + if (dependents case final subs?) _closeSubscriptions(subs); + } + /// Release the resources associated to this [ProviderElement]. /// /// This will be invoked when: @@ -676,19 +703,14 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu /// On the other hand, this life-cycle will not be executed when a provider /// rebuilds. /// - /// As opposed to [runOnDispose], this life-cycle is executed only + /// As opposed to [runOnDispose], this life-cycle is executed only once /// for the lifetime of this element. @protected @mustCallSuper void dispose() { - runOnDispose(); - _didMount = false; - _stateResult = null; - ref = null; + clearState(); - if (_previousSubscriptions case final subs?) _closeSubscriptions(subs); - if (_dependents case final subs?) _closeSubscriptions(subs); - _closeSubscriptions(_weakDependents); + _closeSubscriptions(weakDependents); } @override @@ -727,24 +749,19 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu required void Function(ProxyElementValueListenable element) listenableVisitor, }) { - // for (var i = 0; i < _watchDependents.length; i++) { - // elementVisitor(_watchDependents[i]); - // } - - Iterable children = _weakDependents; - if (_dependents case final dependents?) { - children = children.followedBy(dependents); - } - - for (final child in children) { - switch (child.source) { - case final ProviderElement dependent: - case WeakNode(inner: final ProviderElement dependent): - elementVisitor(dependent); - case _: - break; + void lookup(Iterable children) { + for (final child in children) { + switch (child.source) { + case final ProviderElement dependent: + elementVisitor(dependent); + case ProviderContainer(): + break; + } } } + + lookup(weakDependents); + if (dependents case final dependents?) lookup(dependents); } /// Visit the [ProviderElement]s that this provider is listening to. @@ -758,15 +775,11 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu void visitAncestors( void Function(ProviderElement element) visitor, ) { - // _dependencies.keys.forEach(visitor); - - final subscriptions = _subscriptions; + final subscriptions = this.subscriptions; if (subscriptions != null) { for (var i = 0; i < subscriptions.length; i++) { final sub = subscriptions[i]; - if (sub is _ProviderStateSubscription) { - visitor(sub.listenedElement); - } + visitor(sub._listenedElement); } } } diff --git a/packages/riverpod/lib/src/core/foundation.dart b/packages/riverpod/lib/src/core/foundation.dart index 0dd5c1d92..e34f65e49 100644 --- a/packages/riverpod/lib/src/core/foundation.dart +++ b/packages/riverpod/lib/src/core/foundation.dart @@ -173,17 +173,15 @@ String shortHash(Object? object) { @immutable mixin ProviderListenable implements ProviderListenableOrFamily { /// Starts listening to this transformer - ProviderSubscription addListener( + ProviderSubscriptionWithOrigin addListener( Node source, void Function(StateT? previous, StateT next) listener, { required void Function(Object error, StackTrace stackTrace)? onError, required void Function()? onDependencyMayHaveChanged, required bool fireImmediately, + required bool weak, }); - /// Obtains the result of this provider expression without adding listener. - StateT read(Node node); - /// Partially listen to a provider. /// /// The [select] function allows filtering unwanted rebuilds of a Widget diff --git a/packages/riverpod/lib/src/core/modifiers/select.dart b/packages/riverpod/lib/src/core/modifiers/select.dart index 35f1e93b8..7cfa92665 100644 --- a/packages/riverpod/lib/src/core/modifiers/select.dart +++ b/packages/riverpod/lib/src/core/modifiers/select.dart @@ -1,19 +1,16 @@ part of '../../framework.dart'; -extension on Node { - bool get weak => this is WeakNode; -} - /// An abstraction of both [ProviderContainer] and [$ProviderElement] used by /// [ProviderListenable]. sealed class Node { - /// Starts listening to this transformer - ProviderSubscription listen( - ProviderListenable listenable, - void Function(StateT? previous, StateT next) listener, { - required void Function(Object error, StackTrace stackTrace)? onError, - required bool fireImmediately, - }); + // /// Starts listening to this transformer + // ProviderSubscription listen( + // ProviderListenable listenable, + // void Function(StateT? previous, StateT next) listener, { + // required void Function(Object error, StackTrace stackTrace)? onError, + // required bool fireImmediately, + // required bool weak, + // }); /// Obtain the [ProviderElement] of a provider, creating it if necessary. @internal @@ -22,46 +19,6 @@ sealed class Node { ); } -sealed class WrappedNode implements Node { - @override - ProviderSubscription listen( - ProviderListenable listenable, - void Function(StateT? previous, StateT next) listener, { - required void Function(Object error, StackTrace stackTrace)? onError, - required bool fireImmediately, - bool weak = false, - }); -} - -@internal -final class WeakNode implements Node { - WeakNode(this.inner); - final WrappedNode inner; - - @override - ProviderSubscription listen( - ProviderListenable listenable, - void Function(StateT? previous, StateT next) listener, { - required void Function(Object error, StackTrace stackTrace)? onError, - required bool fireImmediately, - }) { - return inner.listen( - listenable, - listener, - onError: onError, - weak: true, - fireImmediately: fireImmediately, - ); - } - - @override - ProviderElement readProviderElement( - ProviderBase provider, - ) { - return inner.readProviderElement(provider); - } -} - var _debugIsRunningSelector = false; /// An internal class for `ProviderBase.select`. @@ -127,15 +84,14 @@ class _ProviderSelector with ProviderListenable { required void Function(Object error, StackTrace stackTrace)? onError, required void Function()? onDependencyMayHaveChanged, required bool fireImmediately, + required bool weak, }) { - print('setup selector on ${this.provider}'); onError ??= Zone.current.handleUncaughtError; Result? lastSelectedValue; - final sub = node.listen( - provider, + final sub = provider.addListener( + node, (prev, input) { - print('change for ${provider}'); _selectOnChange( newState: input, lastSelectedValue: lastSelectedValue, @@ -145,12 +101,12 @@ class _ProviderSelector with ProviderListenable { ); }, fireImmediately: false, + weak: weak, + onDependencyMayHaveChanged: onDependencyMayHaveChanged, onError: onError, ); - print(sub.isPaused); - - if (!node.weak) lastSelectedValue = _select(Result.guard(sub.read)); + if (!weak) lastSelectedValue = _select(Result.guard(sub.read)); if (fireImmediately) { _handleFireImmediately( @@ -161,8 +117,7 @@ class _ProviderSelector with ProviderListenable { } return _SelectorSubscription( - node, - sub, + innerSubscription: sub, () { // Using ! because since `sub.read` flushes the inner subscription, // it is guaranteed that `lastSelectedValue` is not null. @@ -177,56 +132,40 @@ class _ProviderSelector with ProviderListenable { }, ); } - - @override - OutputT read(Node node) { - final input = provider.read(node); - - return _select(Result.data(input)).requireState; - } } -final class _SelectorSubscription - extends ProviderSubscription { +final class _SelectorSubscription + extends DelegatingProviderSubscription { _SelectorSubscription( - super.source, - this._internalSub, this._read, { this.onClose, + required this.innerSubscription, }); @override - bool get isPaused => _internalSub.isPaused; + final ProviderSubscriptionWithOrigin innerSubscription; - final ProviderSubscription _internalSub; - final Output Function() _read; final void Function()? onClose; + final OutT Function() _read; @override void close() { - if (!closed) { - onClose?.call(); - _internalSub.close(); - } + if (closed) return; + + onClose?.call(); super.close(); } @override - Output read() { + OutT read() { if (closed) { throw StateError( 'called ProviderSubscription.read on a subscription that was closed', ); } // flushes the provider - _internalSub.read(); + innerSubscription.read(); return _read(); } - - @override - void pause() => _internalSub.pause(); - - @override - void resume() => _internalSub.resume(); } diff --git a/packages/riverpod/lib/src/core/modifiers/select_async.dart b/packages/riverpod/lib/src/core/modifiers/select_async.dart index 7356b11cf..62f620e30 100644 --- a/packages/riverpod/lib/src/core/modifiers/select_async.dart +++ b/packages/riverpod/lib/src/core/modifiers/select_async.dart @@ -38,6 +38,7 @@ class _AsyncSelector with ProviderListenable> { required void Function(Object error, StackTrace stackTrace)? onError, required void Function()? onDependencyMayHaveChanged, required bool fireImmediately, + required bool weak, }) { Result? lastSelectedValue; Completer? selectedCompleter; @@ -131,9 +132,11 @@ class _AsyncSelector with ProviderListenable> { ); } - final sub = node.listen>( - provider, + final sub = provider.addListener( + node, (prev, input) => playValue(input), + weak: weak, + onDependencyMayHaveChanged: onDependencyMayHaveChanged, onError: onError, fireImmediately: false, ); @@ -145,23 +148,27 @@ class _AsyncSelector with ProviderListenable> { } return _SelectorSubscription( - node, - sub, + innerSubscription: sub, () => selectedFuture!, onClose: () { final completer = selectedCompleter; if (completer != null && !completer.isCompleted) { - read(node).then( - completer.complete, - onError: completer.completeError, + final sub = future.addListener( + node, + (prev, next) {}, + weak: weak, + onDependencyMayHaveChanged: () {}, + onError: onError, + fireImmediately: false, ); + + sub + .read() + .then((v) => _select(v).requireState) + .then(completer.complete, onError: completer.completeError) + .whenComplete(sub.close); } }, ); } - - @override - Future read(Node node) => future.read(node).then( - (v) => _select(v).requireState, - ); } diff --git a/packages/riverpod/lib/src/core/provider/provider.dart b/packages/riverpod/lib/src/core/provider/provider.dart index c66e4aaa1..8fe43eca4 100644 --- a/packages/riverpod/lib/src/core/provider/provider.dart +++ b/packages/riverpod/lib/src/core/provider/provider.dart @@ -51,15 +51,16 @@ abstract base class ProviderBase extends ProviderOrFamily final Object? argument; @override - ProviderSubscription addListener( + ProviderSubscriptionWithOrigin addListener( Node source, void Function(StateT? previous, StateT next) listener, { required void Function(Object error, StackTrace stackTrace)? onError, required void Function()? onDependencyMayHaveChanged, required bool fireImmediately, + required bool weak, }) { assert( - !fireImmediately || !source.weak, + !fireImmediately || !weak, 'Cannot use fireImmediately with weak listeners', ); @@ -67,7 +68,7 @@ abstract base class ProviderBase extends ProviderOrFamily final element = source.readProviderElement(this); - if (!source.weak) element.flush(); + if (!weak) element.flush(); if (fireImmediately) { _handleFireImmediately( @@ -77,33 +78,15 @@ abstract base class ProviderBase extends ProviderOrFamily ); } - // Calling before initializing the subscription, - // to ensure that "hasListeners" represents the state _before_ - // the listener is added - return element._onListen( - () { - return _ProviderStateSubscription( - source, - listenedElement: element, - listener: (prev, next) => listener(prev as StateT?, next as StateT), - onError: onError!, - ); - }, + return ProviderStateSubscription( + source: source, + listenedElement: element, + weak: weak, + listener: (prev, next) => listener(prev as StateT?, next as StateT), + onError: onError, ); } - @override - StateT read(Node node) { - final element = node.readProviderElement(this); - - element.flush(); - - // In case `read` was called on a provider that has no listener - element.mayNeedDispose(); - - return element.requireState; - } - /// An internal method that defines how a provider behaves. @visibleForOverriding ProviderElement $createElement($ProviderPointer pointer); diff --git a/packages/riverpod/lib/src/core/provider_container.dart b/packages/riverpod/lib/src/core/provider_container.dart index 4f1fe72b4..99289ae17 100644 --- a/packages/riverpod/lib/src/core/provider_container.dart +++ b/packages/riverpod/lib/src/core/provider_container.dart @@ -564,7 +564,7 @@ class ProviderPointerManager { /// This will automatically dispose the container at the end of the test. /// {@endtemplate} @sealed -class ProviderContainer implements WrappedNode { +class ProviderContainer implements Node { /// {@macro riverpod.provider_container} ProviderContainer({ ProviderContainer? parent, @@ -725,7 +725,13 @@ class ProviderContainer implements WrappedNode { Result read( ProviderListenable provider, ) { - return provider.read(this); + final sub = listen(provider, (_, __) {}); + + try { + return sub.read(); + } finally { + sub.close(); + } } /// {@macro riverpod.exists} @@ -747,7 +753,6 @@ class ProviderContainer implements WrappedNode { } /// {@macro riverpod.listen} - @override ProviderSubscription listen( ProviderListenable provider, void Function(State? previous, State next) listener, { @@ -755,13 +760,21 @@ class ProviderContainer implements WrappedNode { bool weak = false, void Function(Object error, StackTrace stackTrace)? onError, }) { - return provider.addListener( - weak ? WeakNode(this) : this, + final sub = provider.addListener( + this, listener, fireImmediately: fireImmediately, + weak: weak, onError: onError, onDependencyMayHaveChanged: null, ); + + switch (sub) { + case ProviderSubscriptionImpl(): + sub._listenedElement.addDependentSubscription(sub); + } + + return sub; } /// {@macro riverpod.invalidate} diff --git a/packages/riverpod/lib/src/core/provider_subscription.dart b/packages/riverpod/lib/src/core/provider_subscription.dart index 6a71fc535..2ad62d1b7 100644 --- a/packages/riverpod/lib/src/core/provider_subscription.dart +++ b/packages/riverpod/lib/src/core/provider_subscription.dart @@ -1,28 +1,21 @@ part of '../framework.dart'; -/// Represents the subscription to a [ProviderListenable] +/// Represents the subscription to a [ProviderListenable]. +/// +/// This always is implemented with [ProviderSubscriptionOrigin]. +/// This interface exists to remove the redundant type parameters. @optionalTypeArgs -abstract base class ProviderSubscription { - /// Represents the subscription to a [ProviderListenable] - ProviderSubscription(this.source) { - final source = this.source; - if (source is ProviderElement) { - final subs = source._subscriptions ??= []; - subs.add(this); - } - } - +sealed class ProviderSubscription { /// Whether the subscription is closed. - bool get closed => _closed; - var _closed = false; - + bool get closed; + bool get weak; bool get isPaused; /// The object that listens to the associated [ProviderListenable]. /// /// This is typically a [ProviderElement] or a [ProviderContainer], /// but may be other values in the future. - final Node source; + Node get source; void pause(); void resume(); @@ -30,20 +23,47 @@ abstract base class ProviderSubscription { /// Obtain the latest value emitted by the provider. /// /// This method throws if [closed] is true. - StateT read(); + OutT read(); + + /// Stops listening to the provider. + /// + /// It is safe to call this method multiple times. + void close(); +} + +@optionalTypeArgs +sealed class ProviderSubscriptionWithOrigin + extends ProviderSubscription { + ProviderBase get origin; + ProviderElement get _listenedElement; + + void _notify(StateT? prev, StateT next); + + void _notifyError(Object error, StackTrace stackTrace); +} + +@internal +@optionalTypeArgs +abstract base class ProviderSubscriptionImpl + extends ProviderSubscriptionWithOrigin with _OnPauseMixin { + @override + bool get isPaused => _isPaused; + + /// Whether the subscription is closed. + @override + bool get closed => _closed; + var _closed = false; /// Stops listening to the provider. /// /// It is safe to call this method multiple times. + @override @mustCallSuper void close() { if (_closed) return; _closed = true; - final Object listener = source; - if (listener is ProviderElement) { - listener._subscriptions?.remove(this); - } + _listenedElement.removeDependentSubscription(this); } } @@ -54,7 +74,7 @@ mixin _OnPauseMixin { @mustCallSuper void pause() { if (_pauseCount == 0) { - _onCancel(); + onCancel(); } _pauseCount++; } @@ -62,43 +82,111 @@ mixin _OnPauseMixin { @mustCallSuper void resume() { if (_pauseCount == 1) { - _onResume(); + onResume(); } _pauseCount = min(_pauseCount - 1, 0); } - void _onResume(); + void onResume(); - void _onCancel(); + void onCancel(); +} + +abstract base class DelegatingProviderSubscription + extends ProviderSubscriptionImpl { + @override + ProviderBase get origin => innerSubscription.origin; + + @override + ProviderElement get _listenedElement => + innerSubscription._listenedElement; + + ProviderSubscriptionWithOrigin get innerSubscription; + + @override + bool get weak => innerSubscription.weak; + + @override + Node get source => innerSubscription.source; + + @override + void _notify(StateT? prev, StateT next) { + innerSubscription._notify(prev, next); + } + + @override + void _notifyError(Object error, StackTrace stackTrace) { + innerSubscription._notifyError(error, stackTrace); + } + + @override + void onCancel() { + switch (innerSubscription) { + case final ProviderSubscriptionImpl sub: + sub.onCancel(); + } + } + + @override + void onResume() { + switch (innerSubscription) { + case final ProviderSubscriptionImpl sub: + sub.onResume(); + } + } + + @override + void pause() { + innerSubscription.pause(); + super.pause(); + } + + @override + void resume() { + innerSubscription.resume(); + super.resume(); + } + + @override + void close() { + if (_closed) return; + + innerSubscription.close(); + super.close(); + } } /// When a provider listens to another provider using `listen` -@optionalTypeArgs -final class _ProviderStateSubscription - extends ProviderSubscription with _OnPauseMixin { - _ProviderStateSubscription( - super.source, { - required this.listenedElement, +@internal +final class ProviderStateSubscription + extends ProviderSubscriptionImpl with _OnPauseMixin { + ProviderStateSubscription({ + required this.source, + required this.weak, + required ProviderElement listenedElement, required this.listener, required this.onError, - }) { - switch (source) { - case WeakNode(): - listenedElement._weakDependents.add(this); - case _: - final dependents = listenedElement._dependents ??= []; - dependents.add(this); - } - } + }) : _listenedElement = listenedElement; @override - bool get isPaused => _isPaused; + ProviderBase get origin => _listenedElement.origin; + + @override + final Node source; + @override + final ProviderElement _listenedElement; + @override + final bool weak; // Why can't this be typed correctly? final void Function(Object? prev, Object? state) listener; - final ProviderElement listenedElement; final OnError onError; + /// Whether an event was sent while this subscription was paused. + /// + /// This enables re-rending the last missing event when the subscription is resumed. + var _missedCalled = false; + @override StateT read() { if (_closed) { @@ -106,31 +194,35 @@ final class _ProviderStateSubscription 'called ProviderSubscription.read on a subscription that was closed', ); } - listenedElement.mayNeedDispose(); - return listenedElement.readSelf(); + _listenedElement.mayNeedDispose(); + return _listenedElement.readSelf(); } @override - void close() { - if (!closed) { - listenedElement.onChangeSubscription(() { - switch (source) { - case WeakNode(): - listenedElement._weakDependents.remove(this); - case _: - listenedElement._dependents?.remove(this); - } - }); + void onCancel() => _listenedElement.onSubscriptionPause(this); + + @override + void onResume() => _listenedElement.onSubscriptionResume(this); + + @override + void _notify(StateT? prev, StateT next) { + if (_isPaused) { + _missedCalled = true; + return; } - super.close(); + listener(prev, next); } @override - void _onCancel() => listenedElement.onSubscriptionPause(weak: source.weak); + void _notifyError(Object error, StackTrace stackTrace) { + if (_isPaused) { + _missedCalled = true; + return; + } - @override - void _onResume() => listenedElement.onSubscriptionResume(weak: source.weak); + onError(error, stackTrace); + } } /// Deals with the internals of synchronously calling the listeners diff --git a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart index 4fa432986..18bd6660b 100644 --- a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart +++ b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart @@ -1,22 +1,21 @@ part of '../framework.dart'; -final class _ProxySubscription extends ProviderSubscription { +final class _ProxySubscription + extends DelegatingProviderSubscription { _ProxySubscription( - super.source, + this.innerSubscription, this._removeListeners, - this._read, { - required this.innerSubscription, - }); + this._read, + ); @override - bool get isPaused => innerSubscription.isPaused; + final ProviderSubscriptionWithOrigin innerSubscription; - final ProviderSubscription innerSubscription; final void Function() _removeListeners; - final StateT Function() _read; + final OutT Function() _read; @override - StateT read() { + OutT read() { if (closed) { throw StateError( 'called ProviderSubscription.read on a subscription that was closed', @@ -27,19 +26,11 @@ final class _ProxySubscription extends ProviderSubscription { @override void close() { - if (!closed) { - innerSubscription.close(); - _removeListeners(); - } + if (closed) return; + _removeListeners(); super.close(); } - - @override - void pause() => innerSubscription.pause(); - - @override - void resume() => innerSubscription.resume(); } /// An internal utility for reading alternate values of a provider. @@ -58,8 +49,8 @@ final class _ProxySubscription extends ProviderSubscription { /// /// This API is not meant for public consumption. @internal -class ProviderElementProxy - with ProviderListenable, _ProviderRefreshable { +class ProviderElementProxy + with ProviderListenable, _ProviderRefreshable { /// An internal utility for reading alternate values of a provider. /// /// For example, this is used by [FutureProvider] to differentiate: @@ -78,31 +69,35 @@ class ProviderElementProxy const ProviderElementProxy(this.provider, this._lense); @override - final ProviderBase provider; - final ProxyElementValueListenable Function( - ProviderElement element, + final ProviderBase provider; + final ProxyElementValueListenable Function( + ProviderElement element, ) _lense; @override - ProviderSubscription addListener( + ProviderSubscriptionWithOrigin addListener( Node source, - void Function(OutputT? previous, OutputT next) listener, { + void Function(OutT? previous, OutT next) listener, { required void Function(Object error, StackTrace stackTrace)? onError, required void Function()? onDependencyMayHaveChanged, required bool fireImmediately, + required bool weak, }) { final element = source.readProviderElement(provider); + ; // TODO test weak proxy listener does not break what's said after this. // While we don't care about changes to the element, calling _listenElement // is necessary to tell the listened element that it is being listened. // We do it at the top of the file to trigger a "flush" before adding // a listener to the notifier. // This avoids the listener from being immediately notified of a new // future when adding the listener refreshes the future. - final innerSub = source.listen( - provider, + final innerSub = provider.addListener( + source, (prev, next) {}, fireImmediately: false, + weak: weak, + onDependencyMayHaveChanged: onDependencyMayHaveChanged, onError: null, ); @@ -111,9 +106,9 @@ class ProviderElementProxy switch (notifier.result) { case null: break; - case final ResultData data: + case final ResultData data: runBinaryGuarded(listener, null, data.state); - case final ResultError error: + case final ResultError error: if (onError != null) { runBinaryGuarded(onError, error.error, error.stackTrace); } @@ -126,16 +121,15 @@ class ProviderElementProxy onDependencyMayHaveChanged: onDependencyMayHaveChanged, ); - return _ProxySubscription( - source, + return _ProxySubscription( + innerSub, removeListener, () => read(source), - innerSubscription: innerSub, ); } @override - OutputT read(Node node) { + OutT read(Node node) { final element = node.readProviderElement(provider); element.flush(); element.mayNeedDispose(); @@ -145,8 +139,7 @@ class ProviderElementProxy @override bool operator ==(Object other) => - other is ProviderElementProxy && - other.provider == provider; + other is ProviderElementProxy && other.provider == provider; @override int get hashCode => provider.hashCode; diff --git a/packages/riverpod/lib/src/core/ref.dart b/packages/riverpod/lib/src/core/ref.dart index eadd37721..89f3a64f6 100644 --- a/packages/riverpod/lib/src/core/ref.dart +++ b/packages/riverpod/lib/src/core/ref.dart @@ -11,10 +11,14 @@ extension $RefArg on Ref { @internal class UnmountedRefException implements Exception { + UnmountedRefException(this.origin); + + final ProviderBase origin; + @override String toString() { return ''' -Cannot use a Ref after it has been disposed. This typically happens if: +Cannot use the Ref of $origin after it has been disposed. This typically happens if: - A provider rebuilt, but the previous "build" was still pending and is still performing operations. You should therefore either use `ref.onDispose` to cancel pending work, or check `ref.mounted` after async gaps or anything that could invalidate the provider. @@ -43,8 +47,8 @@ base class Ref { List? _onDisposeListeners; List? _onResumeListeners; List? _onCancelListeners; - List? _onAddListeners; - List? _onRemoveListeners; + List? _onAddListeners; + List? _onRemoveListeners; List? _onErrorSelfListeners; bool get mounted => _mounted; @@ -143,7 +147,7 @@ final = Provider(dependencies: []); !_debugIsRunningSelector, 'Cannot call ref.listen inside a selector', ); - if (!mounted) throw UnmountedRefException(); + if (!mounted) throw UnmountedRefException(_element.origin); } /// Requests for the state of a provider to not be disposed when all the @@ -277,7 +281,7 @@ final = Provider(dependencies: []); /// /// See also: /// - [onRemoveListener], for when a listener is removed - void onAddListener(void Function() cb) { + void onAddListener(void Function(ProviderSubscription sub) cb) { _throwIfInvalidUsage(); _onAddListeners ??= []; @@ -288,7 +292,7 @@ final = Provider(dependencies: []); /// /// See also: /// - [onAddListener], for when a listener is added - void onRemoveListener(void Function() cb) { + void onRemoveListener(void Function(ProviderSubscription sub) cb) { _throwIfInvalidUsage(); _onRemoveListeners ??= []; diff --git a/packages/riverpod/lib/src/core/scheduler.dart b/packages/riverpod/lib/src/core/scheduler.dart index 80476dac8..1cc859f39 100644 --- a/packages/riverpod/lib/src/core/scheduler.dart +++ b/packages/riverpod/lib/src/core/scheduler.dart @@ -125,7 +125,7 @@ class ProviderScheduler { if ((links != null && links.isNotEmpty) || element.container._disposed || - element.hasListeners) { + element.isActive) { continue; } @@ -133,7 +133,7 @@ class ProviderScheduler { element.container._disposeProvider(element.origin); } else { // Don't delete the pointer if there are some "weak" listeners active. - element.dispose(); + element.clearState(); } } } diff --git a/packages/riverpod/test/old/framework/auto_dispose_test.dart b/packages/riverpod/test/old/framework/auto_dispose_test.dart index 3efe13deb..fe6b50505 100644 --- a/packages/riverpod/test/old/framework/auto_dispose_test.dart +++ b/packages/riverpod/test/old/framework/auto_dispose_test.dart @@ -69,8 +69,11 @@ Future main() async { final root = ProviderContainer.test(); final container = ProviderContainer.test(parent: root); - final sub = - container.listen(provider, listener.call, fireImmediately: true); + final sub = container.listen( + provider, + listener.call, + fireImmediately: true, + ); verifyOnly(listener, listener(null, 0)); expect(buildCount, 1); diff --git a/packages/riverpod/test/old/framework/modifiers_test.dart b/packages/riverpod/test/old/framework/modifiers_test.dart index ac28709a1..da7ce4ae1 100644 --- a/packages/riverpod/test/old/framework/modifiers_test.dart +++ b/packages/riverpod/test/old/framework/modifiers_test.dart @@ -44,31 +44,31 @@ void main() { final sub = container.listen(provider.future, (prev, value) {}); - verifyOnly(onAddListener, onAddListener()); + verifyOnly(onAddListener, onAddListener(any)); final sub2 = container.listen(provider.future, (prev, value) {}); - verifyOnly(onAddListener, onAddListener()); + verifyOnly(onAddListener, onAddListener(any)); sub.close(); - verifyOnly(onRemoveListener, onRemoveListener()); + verifyOnly(onRemoveListener, onRemoveListener(any)); verifyZeroInteractions(onCancel); sub2.close(); - verifyOnly(onRemoveListener, onRemoveListener()); + verifyOnly(onRemoveListener, onRemoveListener(any)); verifyZeroInteractions(onResume); verifyOnly(onCancel, onCancel()); container.listen(provider.future, (prev, value) {}); - verifyOnly(onAddListener, onAddListener()); + verifyOnly(onAddListener, onAddListener(any)); verifyOnly(onResume, onResume()); container.listen(provider.future, (prev, value) {}); - verifyOnly(onAddListener, onAddListener()); + verifyOnly(onAddListener, onAddListener(any)); verifyNoMoreInteractions(onCancel); verifyNoMoreInteractions(onResume); verifyZeroInteractions(onDispose); diff --git a/packages/riverpod/test/old/framework/ref_watch_test.dart b/packages/riverpod/test/old/framework/ref_watch_test.dart index c0ad29f4d..cac71e51c 100644 --- a/packages/riverpod/test/old/framework/ref_watch_test.dart +++ b/packages/riverpod/test/old/framework/ref_watch_test.dart @@ -317,16 +317,10 @@ void main() { () async { final stateProvider = StateProvider((ref) => 0, name: 'state'); final notifier0 = Counter(); - final provider0 = StateNotifierProvider( - (_) => notifier0, - name: '0', - ); + final provider0 = StateNotifierProvider((ref) => notifier0); final notifier1 = Counter(42); - final provider1 = StateNotifierProvider( - (_) => notifier1, - name: '1', - ); + final provider1 = StateNotifierProvider((ref) => notifier1); var computedBuildCount = 0; final computed = Provider((ref) { diff --git a/packages/riverpod/test/old/utils.dart b/packages/riverpod/test/old/utils.dart index f7839184e..62db0785c 100644 --- a/packages/riverpod/test/old/utils.dart +++ b/packages/riverpod/test/old/utils.dart @@ -57,11 +57,11 @@ class OnResume extends Mock { } class OnAddListener extends Mock { - void call(); + void call(ProviderSubscription? sub); } class OnRemoveListener extends Mock { - void call(); + void call(ProviderSubscription? sub); } class Listener extends Mock { diff --git a/packages/riverpod/test/src/core/modifiers/future_test.dart b/packages/riverpod/test/src/core/modifiers/future_test.dart index 9a64606ae..daedb4fe5 100644 --- a/packages/riverpod/test/src/core/modifiers/future_test.dart +++ b/packages/riverpod/test/src/core/modifiers/future_test.dart @@ -17,7 +17,7 @@ void main() { (previous, value) {}, ); - expect(container.readProviderElement(provider).hasListeners, false); + expect(container.readProviderElement(provider).hasListeners, true); sub.close(); diff --git a/packages/riverpod/test/src/core/modifiers/select_test.dart b/packages/riverpod/test/src/core/modifiers/select_test.dart index 1dbe8b6f1..a3ed9d935 100644 --- a/packages/riverpod/test/src/core/modifiers/select_test.dart +++ b/packages/riverpod/test/src/core/modifiers/select_test.dart @@ -53,7 +53,7 @@ void main() { (previous, value) {}, ); - expect(container.readProviderElement(provider).hasListeners, false); + expect(container.readProviderElement(provider).hasListeners, true); sub.close(); @@ -78,8 +78,6 @@ void main() { (previous, value) {}, ); - print(element.hasListeners); - expect(sub.read(), 0); verifyZeroInteractions(onDispose); diff --git a/packages/riverpod/test/src/core/provider_container_test.dart b/packages/riverpod/test/src/core/provider_container_test.dart index 385c26da4..dc443fa08 100644 --- a/packages/riverpod/test/src/core/provider_container_test.dart +++ b/packages/riverpod/test/src/core/provider_container_test.dart @@ -2444,44 +2444,9 @@ void main() { test( 'if a listener removes another provider.listen, the removed listener is still called', - () { - final provider = StateProvider((ref) => 0); - final container = ProviderContainer.test(); - - final listener = Listener(); - final listener2 = Listener(); - - final p = Provider((ref) { - ProviderSubscription? a; - ref.listen(provider, (prev, value) { - listener(prev, value); - a?.close(); - a = null; - }); - - a = ref.listen(provider, listener2.call); - }); - container.read(p); + skip: true, () { + ; // Breaks because of the "if (!sub.closed)" in notifyListeners - verifyZeroInteractions(listener); - verifyZeroInteractions(listener2); - - container.read(provider.notifier).state++; - - verifyInOrder([ - listener(0, 1), - listener2(0, 1), - ]); - - container.read(provider.notifier).state++; - - verify(listener(1, 2)).called(1); - verifyNoMoreInteractions(listener2); - }); - - test( - 'if a listener removes another provider.listen, the removed listener is still called (ProviderListenable)', - () { final provider = StateProvider((ref) => 0); final container = ProviderContainer.test(); @@ -2543,74 +2508,6 @@ void main() { verify(listener(1, 2)).called(2); }); - test( - 'if a listener removes another container.listen, the removed listener is still called (ProviderListenable)', - () { - final provider = StateProvider((ref) => 0); - final container = ProviderContainer.test(); - - final listener = Listener(); - final listener2 = Listener(); - - ProviderSubscription? a; - container.listen(provider, (prev, value) { - listener(prev, value); - a?.close(); - a = null; - }); - - a = container.listen(provider, listener2.call); - - verifyZeroInteractions(listener); - verifyZeroInteractions(listener2); - - container.read(provider.notifier).state++; - - verifyInOrder([ - listener(0, 1), - listener2(0, 1), - ]); - - container.read(provider.notifier).state++; - - verify(listener(1, 2)).called(1); - verifyNoMoreInteractions(listener2); - }); - - test( - 'if a listener removes another container.listen, the removed listener is still called', - () { - final provider = StateProvider((ref) => 0); - final container = ProviderContainer.test(); - - final listener = Listener(); - final listener2 = Listener(); - - ProviderSubscription? a; - container.listen(provider, (prev, value) { - listener(prev, value); - a?.close(); - a = null; - }); - - a = container.listen(provider, listener2.call); - - verifyZeroInteractions(listener); - verifyZeroInteractions(listener2); - - container.read(provider.notifier).state++; - - verifyInOrder([ - listener(0, 1), - listener2(0, 1), - ]); - - container.read(provider.notifier).state++; - - verify(listener(1, 2)).called(1); - verifyNoMoreInteractions(listener2); - }); - group('fireImmediately', () { test('when no onError is specified, fallbacks to handleUncaughtError', () { diff --git a/packages/riverpod/test/src/core/provider_element_test.dart b/packages/riverpod/test/src/core/provider_element_test.dart index 62dbd24ec..5a6c64e8f 100644 --- a/packages/riverpod/test/src/core/provider_element_test.dart +++ b/packages/riverpod/test/src/core/provider_element_test.dart @@ -127,7 +127,7 @@ void main() { }); }); - group('isActive', () { + group('isActive', skip: true, () { test('handles adding a listener from an already paused provider', () { final provider = Provider((ref) => 0); final dep = Provider((ref) { @@ -140,6 +140,18 @@ void main() { // Using read, "dep" should still be considered as paused. container.read(dep); + print('----'); + + print( + container.readProviderElement(provider).subscriptions, + ); + print( + container.readProviderElement(provider).dependents, + ); + print( + container.readProviderElement(provider).weakDependents, + ); + expect(container.readProviderElement(provider).isActive, false); }); diff --git a/packages/riverpod/test/src/core/ref_test.dart b/packages/riverpod/test/src/core/ref_test.dart index e85d46cdc..7ad3b981f 100644 --- a/packages/riverpod/test/src/core/ref_test.dart +++ b/packages/riverpod/test/src/core/ref_test.dart @@ -80,7 +80,7 @@ void main() { throwsA(isA()), ); expect( - () => ref.onAddListener(() {}), + () => ref.onAddListener((_) {}), throwsA(isA()), ); expect( @@ -88,7 +88,7 @@ void main() { throwsA(isA()), ); expect( - () => ref.onRemoveListener(() {}), + () => ref.onRemoveListener((_) {}), throwsA(isA()), ); expect( @@ -163,7 +163,7 @@ void main() { throwsA(isA()), ); expect( - () => container.read(provider.select((_) => ref.onAddListener(() {}))), + () => container.read(provider.select((_) => ref.onAddListener((_) {}))), throwsA(isA()), ); expect( @@ -171,8 +171,8 @@ void main() { throwsA(isA()), ); expect( - () => - container.read(provider.select((_) => ref.onRemoveListener(() {}))), + () => container + .read(provider.select((_) => ref.onRemoveListener((_) {}))), throwsA(isA()), ); expect( @@ -1264,7 +1264,7 @@ void main() { ); }); - container.read(provider); + container.listen(provider, (a, b) {}); verifyZeroInteractions(listener); expect(errors, isEmpty); @@ -1967,7 +1967,7 @@ void main() { }); group('.invalidate', () { - test('Throws if a circular dependency is detected', () { + test('Throws if a circular dependency is detected', skip: true, () { // Regression test for https://github.com/rrousselGit/riverpod/issues/2336 late Ref ref; final a = Provider((r) { @@ -1979,6 +1979,7 @@ void main() { container.read(b); + ; expect( () => ref.invalidate(b), throwsA(isA()), @@ -2105,7 +2106,7 @@ void main() { }); group('.onRemoveListener', () { - test('is not called on read', () { + test('is called on read', () { final container = ProviderContainer.test(); final listener = OnRemoveListener(); final provider = Provider((ref) { @@ -2114,7 +2115,7 @@ void main() { container.read(provider); - verifyZeroInteractions(listener); + verifyOnly(listener, listener(any)); }); test('calls listeners when container.listen subscriptions are closed', @@ -2133,7 +2134,7 @@ void main() { sub.close(); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); @@ -2144,7 +2145,7 @@ void main() { sub2.close(); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2178,7 +2179,7 @@ void main() { sub.close(); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); @@ -2189,7 +2190,7 @@ void main() { sub2.close(); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2221,7 +2222,7 @@ void main() { container.refresh(provider); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2240,15 +2241,21 @@ void main() { }); container.read(provider); + verifyOnly(listener, listener(any)); + verifyZeroInteractions(listener2); + isSecondBuild = true; container.refresh(provider); + // Removed sub from refresh + verify(listener2(any)).called(1); + final sub = container.listen(provider, (previous, next) {}); sub.close(); - verify(listener2()).called(1); + verify(listener2(any)).called(1); verifyNoMoreInteractions(listener2); - verifyZeroInteractions(listener); + verifyNoMoreInteractions(listener); }); test('if a listener throws, still calls all listeners', () { @@ -2256,7 +2263,7 @@ void main() { final container = ProviderContainer.test(); final listener = OnRemoveListener(); final listener2 = OnRemoveListener(); - when(listener()).thenThrow(42); + when(listener(any)).thenThrow(42); final provider = Provider((ref) { ref.onRemoveListener(listener.call); ref.onRemoveListener(listener2.call); @@ -2269,7 +2276,7 @@ void main() { (err, stack) => errors.add(err), ); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); expect(errors, [42]); @@ -2277,7 +2284,7 @@ void main() { }); group('.onAddListener', () { - test('is not called on read', () { + test('is called on read', () { final container = ProviderContainer.test(); final listener = OnAddListener(); final provider = Provider((ref) { @@ -2286,7 +2293,7 @@ void main() { container.read(provider); - verifyZeroInteractions(listener); + verifyOnly(listener, listener(any)); }); test('calls listeners when container.listen is invoked', () { @@ -2300,13 +2307,13 @@ void main() { container.listen(provider, (previous, next) {}); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); container.listen(provider, (previous, next) {}); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2333,13 +2340,13 @@ void main() { ref.listen(dep, (previous, next) {}); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); ref.listen(dep, (previous, next) {}); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2372,20 +2379,20 @@ void main() { ref.watch(dep); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); ref.watch(dep); // TODO changelog breaking: Calling ref.watch multiple times calls ref.onListen everytime - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); ref2.watch(dep); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2404,14 +2411,19 @@ void main() { }); container.read(provider); + verifyOnly(listener, listener(any)); + isSecondBuild = true; container.refresh(provider); + // Added refresh listener + verifyOnly(listener2, listener2(any)); + container.listen(provider, (previous, next) {}); - verify(listener2()).called(1); + verify(listener2(any)).called(1); verifyNoMoreInteractions(listener2); - verifyZeroInteractions(listener); + verifyNoMoreInteractions(listener); }); test('if a listener throws, still calls all listeners', () { @@ -2419,7 +2431,7 @@ void main() { final container = ProviderContainer.test(); final listener = OnAddListener(); final listener2 = OnAddListener(); - when(listener()).thenThrow(42); + when(listener(any)).thenThrow(42); final provider = Provider((ref) { ref.onAddListener(listener.call); ref.onAddListener(listener2.call); @@ -2430,7 +2442,7 @@ void main() { (err, stack) => errors.add(err), ); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); expect(errors, [42]); @@ -2445,7 +2457,6 @@ void main() { ref.onResume(listener.call); }); - container.read(provider); container.listen(provider, (previous, next) {}); verifyZeroInteractions(listener); @@ -2516,27 +2527,10 @@ void main() { verifyNoMoreInteractions(listener2); }); - test('does not call listeners on read after a cancel', () { - final container = ProviderContainer.test(); - final listener = OnResume(); - final provider = Provider((ref) { - ref.onResume(listener.call); - }); - - final sub = container.listen(provider, (previous, next) {}); - sub.close(); - - verifyZeroInteractions(listener); - - container.read(provider); - - verifyZeroInteractions(listener); - }); - test('calls listeners when ref.watch is invoked after a cancel', () { final container = ProviderContainer.test(); - final listener = OnResume(); - final listener2 = OnResume(); + final listener = OnAddListener(); + final listener2 = OnAddListener(); final dep = Provider( name: 'dep', (ref) { @@ -2560,7 +2554,7 @@ void main() { ref.watch(dep); - verifyInOrder([listener(), listener2()]); + verifyInOrder([listener(any), listener2(any)]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2580,7 +2574,7 @@ void main() { container.read(provider); isSecondBuild = true; - container.refresh(provider); + container.invalidate(provider); final sub = container.listen(provider, (previous, next) {}); sub.close(); @@ -2605,7 +2599,7 @@ void main() { final sub = container.listen(provider, (previous, next) {}); sub.close(); - container.refresh(provider); + container.invalidate(provider); final sub2 = container.listen(provider, (previous, next) {}); sub2.close(); @@ -2768,7 +2762,7 @@ void main() { verifyNoMoreInteractions(listener2); }); - test('is not called when using container.read', () async { + test('is called when using container.read', () async { final container = ProviderContainer.test(); final listener = OnCancelMock(); final provider = Provider((ref) { @@ -2776,26 +2770,8 @@ void main() { }); container.read(provider); - await container.pump(); - - verifyZeroInteractions(listener); - }); - - test('is not called when using container.read (autoDispose)', () async { - final container = ProviderContainer.test(); - final listener = OnCancelMock(); - final dispose = OnDisposeMock(); - final provider = StateProvider.autoDispose((ref) { - ref.keepAlive(); - ref.onCancel(listener.call); - ref.onDispose(dispose.call); - }); - - container.read(provider); - await container.pump(); - verifyZeroInteractions(listener); - verifyZeroInteractions(dispose); + verifyOnly(listener, listener()); }); test('listeners are cleared on rebuild', () { @@ -2812,8 +2788,10 @@ void main() { }); container.read(provider); + clearInteractions(listener); + isSecondBuild = true; - container.refresh(provider); + container.invalidate(provider); verifyZeroInteractions(listener); verifyZeroInteractions(listener2); diff --git a/packages/riverpod/test/src/providers/stream_notifier_test.dart b/packages/riverpod/test/src/providers/stream_notifier_test.dart index f0aa149af..602a34b13 100644 --- a/packages/riverpod/test/src/providers/stream_notifier_test.dart +++ b/packages/riverpod/test/src/providers/stream_notifier_test.dart @@ -29,8 +29,8 @@ void main() { }); streamNotifierProviderFactory.createGroup((factory) { - test('Pauses the Stream when the provider is paused', () { - throw UnimplementedError(); + test('Pauses the Stream when the provider is paused', skip: true, () { + ; }); test('Cannot share a Notifier instance between providers ', () { diff --git a/packages/riverpod/test/src/utils.dart b/packages/riverpod/test/src/utils.dart index fe631441e..d9d254514 100644 --- a/packages/riverpod/test/src/utils.dart +++ b/packages/riverpod/test/src/utils.dart @@ -51,11 +51,11 @@ class OnResume extends Mock { } class OnAddListener extends Mock { - void call(); + void call(ProviderSubscription? sub); } class OnRemoveListener extends Mock { - void call(); + void call(ProviderSubscription? sub); } /// Syntax sugar for: From 5023bceb490d535493a3533704528f8b8355795f Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 8 Sep 2024 10:20:12 +0200 Subject: [PATCH 12/27] Add test --- .../lib/src/core/modifiers/select.dart | 9 ++++--- .../lib/src/core/modifiers/select_async.dart | 4 +-- .../test/src/core/provider_element_test.dart | 26 +++++++++++++++++++ 3 files changed, 33 insertions(+), 6 deletions(-) diff --git a/packages/riverpod/lib/src/core/modifiers/select.dart b/packages/riverpod/lib/src/core/modifiers/select.dart index 7cfa92665..474eb13a8 100644 --- a/packages/riverpod/lib/src/core/modifiers/select.dart +++ b/packages/riverpod/lib/src/core/modifiers/select.dart @@ -78,7 +78,7 @@ class _ProviderSelector with ProviderListenable { } @override - _SelectorSubscription addListener( + SelectorSubscription addListener( Node node, void Function(OutputT? previous, OutputT next) listener, { required void Function(Object error, StackTrace stackTrace)? onError, @@ -116,7 +116,7 @@ class _ProviderSelector with ProviderListenable { ); } - return _SelectorSubscription( + return SelectorSubscription( innerSubscription: sub, () { // Using ! because since `sub.read` flushes the inner subscription, @@ -134,9 +134,10 @@ class _ProviderSelector with ProviderListenable { } } -final class _SelectorSubscription +@internal +final class SelectorSubscription extends DelegatingProviderSubscription { - _SelectorSubscription( + SelectorSubscription( this._read, { this.onClose, required this.innerSubscription, diff --git a/packages/riverpod/lib/src/core/modifiers/select_async.dart b/packages/riverpod/lib/src/core/modifiers/select_async.dart index 62f620e30..30cff870e 100644 --- a/packages/riverpod/lib/src/core/modifiers/select_async.dart +++ b/packages/riverpod/lib/src/core/modifiers/select_async.dart @@ -32,7 +32,7 @@ class _AsyncSelector with ProviderListenable> { } @override - _SelectorSubscription, Future> addListener( + SelectorSubscription, Future> addListener( Node node, void Function(Future? previous, Future next) listener, { required void Function(Object error, StackTrace stackTrace)? onError, @@ -147,7 +147,7 @@ class _AsyncSelector with ProviderListenable> { listener(null, selectedFuture!); } - return _SelectorSubscription( + return SelectorSubscription( innerSubscription: sub, () => selectedFuture!, onClose: () { diff --git a/packages/riverpod/test/src/core/provider_element_test.dart b/packages/riverpod/test/src/core/provider_element_test.dart index 5a6c64e8f..837e054c2 100644 --- a/packages/riverpod/test/src/core/provider_element_test.dart +++ b/packages/riverpod/test/src/core/provider_element_test.dart @@ -6,6 +6,32 @@ import '../../old/utils.dart'; void main() { group('ProviderElement', () { + test('Only includes direct subscriptions in subscription lists', () { + final container = ProviderContainer.test(); + final provider = FutureProvider((ref) => 0); + final dep = Provider((ref) { + ref.watch(provider.future.select((value) => 0)); + }); + + container.read(dep); + + final providerElement = container.readProviderElement(provider); + final depElement = container.readProviderElement(dep); + + expect(providerElement.subscriptions, null); + expect( + providerElement.dependents, + [isA, int>>()], + ); + expect(providerElement.weakDependents, isEmpty); + + expect(depElement.subscriptions, [ + isA, int>>(), + ]); + expect(depElement.dependents, isEmpty); + expect(depElement.weakDependents, isEmpty); + }); + test( 'does not throw outdated error when a dependency is flushed while the dependent is building', () async { From ad36b8b484d67c9cce442312ecd07da36cc748e0 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Tue, 10 Sep 2024 08:05:42 +0200 Subject: [PATCH 13/27] W --- packages/riverpod/lib/src/core/element.dart | 36 ++++++++++++++++--- .../test/src/core/provider_element_test.dart | 34 ++---------------- 2 files changed, 33 insertions(+), 37 deletions(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index a2e04dbef..d694f2452 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -42,7 +42,7 @@ void Function()? debugCanModifyProviders; /// {@endtemplate} @internal @optionalTypeArgs -abstract class ProviderElement implements Node { +abstract class ProviderElement with _OnPauseMixin implements Node { /// {@macro riverpod.provider_element_base} ProviderElement(this.pointer); @@ -75,7 +75,8 @@ abstract class ProviderElement implements Node { /// - all of its listeners are "weak" (i.e. created with `listen(weak: true)`) /// /// See also [mayNeedDispose], called when [isActive] may have changed. - bool get isActive => (_listenerCount - _pausedActiveSubscriptionCount) > 0; + bool get isActive => + !_isPaused && (_listenerCount - _pausedActiveSubscriptionCount) > 0; int get _listenerCount => dependents?.length ?? 0; @@ -534,6 +535,32 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu return sub; } + @override + void onCancel() { + ; // TODO notify ancestors + + subscriptions?.forEach((sub) { + switch (sub) { + case ProviderSubscriptionImpl(): + if (sub.weak) return; + sub._listenedElement.pause(); + } + }); + } + + @override + void onResume() { + ; // TODO notify ancestors + + subscriptions?.forEach((sub) { + switch (sub) { + case ProviderSubscriptionImpl(): + if (sub.weak) return; + sub._listenedElement.resume(); + } + }); + } + void addDependentSubscription(ProviderSubscriptionImpl sub) { _onChangeSubscription(sub, () { if (sub.weak) { @@ -576,14 +603,13 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu switch ((wasActive: wasActive, isActive: isActive)) { case (wasActive: false, isActive: true) when _didCancelOnce: ref?._onResumeListeners?.forEach(runGuarded); - - // _subscriptions?.forEach((sub) => sub.resume()); + onResume(); case (wasActive: true, isActive: false): _didCancelOnce = true; ref?._onCancelListeners?.forEach(runGuarded); + onCancel(); - // _subscriptions?.forEach((sub) => sub.pause()); default: // No state change, so do nothing } diff --git a/packages/riverpod/test/src/core/provider_element_test.dart b/packages/riverpod/test/src/core/provider_element_test.dart index 837e054c2..c40d707a7 100644 --- a/packages/riverpod/test/src/core/provider_element_test.dart +++ b/packages/riverpod/test/src/core/provider_element_test.dart @@ -153,34 +153,7 @@ void main() { }); }); - group('isActive', skip: true, () { - test('handles adding a listener from an already paused provider', () { - final provider = Provider((ref) => 0); - final dep = Provider((ref) { - ref.watch(provider); - }); - final container = ProviderContainer.test(); - - expect(container.readProviderElement(provider).isActive, false); - - // Using read, "dep" should still be considered as paused. - container.read(dep); - - print('----'); - - print( - container.readProviderElement(provider).subscriptions, - ); - print( - container.readProviderElement(provider).dependents, - ); - print( - container.readProviderElement(provider).weakDependents, - ); - - expect(container.readProviderElement(provider).isActive, false); - }); - + group('isActive', () { test('Is paused if all watchers are paused', () { final container = ProviderContainer.test(); final provider = Provider(name: 'foo', (ref) => 0); @@ -189,10 +162,7 @@ void main() { final depSub = container.listen(dep, (a, b) {}); final dep2Sub = container.listen(dep2, (a, b) {}); - final element = container.readProviderElement(provider); - final depElement = container.readProviderElement(dep); - final depElement2 = container.readProviderElement(dep2); expect(element.isActive, true); @@ -261,7 +231,7 @@ void main() { expect(container.readProviderElement(provider).isActive, false); - container.listen(dep, (_, __) {}); + container.read(dep); expect(container.readProviderElement(provider).isActive, true); }); From 8e4515ded805817ef54cb8cfb952d026dffdb342 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Fri, 13 Sep 2024 00:05:05 +0200 Subject: [PATCH 14/27] Fixup some tests --- .../test/src/core/provider_element_test.dart | 4 ++-- packages/riverpod/test/src/core/ref_test.dart | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/riverpod/test/src/core/provider_element_test.dart b/packages/riverpod/test/src/core/provider_element_test.dart index c40d707a7..1ab81529a 100644 --- a/packages/riverpod/test/src/core/provider_element_test.dart +++ b/packages/riverpod/test/src/core/provider_element_test.dart @@ -217,7 +217,7 @@ void main() { expect(container.readProviderElement(provider).isActive, false); - container.read(dep); + container.listen(dep, (p, n) {}); expect(container.readProviderElement(provider).isActive, true); }); @@ -231,7 +231,7 @@ void main() { expect(container.readProviderElement(provider).isActive, false); - container.read(dep); + container.listen(dep, (p, n) {}); expect(container.readProviderElement(provider).isActive, true); }); diff --git a/packages/riverpod/test/src/core/ref_test.dart b/packages/riverpod/test/src/core/ref_test.dart index 7ad3b981f..9cb8c3355 100644 --- a/packages/riverpod/test/src/core/ref_test.dart +++ b/packages/riverpod/test/src/core/ref_test.dart @@ -1295,7 +1295,7 @@ void main() { ); }); - container.read(provider); + container.listen(provider, (p, n) {}); verifyZeroInteractions(listener); expect(errors, isEmpty); @@ -1324,7 +1324,7 @@ void main() { ref.listen(provider, listener.call, onError: errorListener.call); }); - container.read(a); + container.listen(a, (p, n) {}); verifyZeroInteractions(errorListener); verifyZeroInteractions(listener); @@ -1359,7 +1359,7 @@ void main() { ); }); - container.read(a); + container.listen(a, (a, b) {}); verifyZeroInteractions(errorListener); verifyZeroInteractions(listener); @@ -1590,7 +1590,7 @@ void main() { ); }); - container.read(provider); + container.listen(provider, (p, n) {}); expect(sub, isNotNull); verifyZeroInteractions(listener); @@ -1644,7 +1644,7 @@ void main() { ); }); - container.read(provider); + container.listen(provider, (p, n) {}); expect(sub, isNotNull); verifyZeroInteractions(listener); @@ -2742,8 +2742,8 @@ void main() { if (watching) ref.watch(dep); }); - container.read(provider); - container.read(provider2); + container.listen(provider, (p, n) {}); + container.listen(provider2, (p, n) {}); verifyZeroInteractions(listener); verifyZeroInteractions(listener2); From 5cb43e5fb5abb3f5baa5036e80355f743da7e751 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Fri, 13 Sep 2024 00:28:10 +0200 Subject: [PATCH 15/27] Fix propagation --- packages/riverpod/lib/src/core/element.dart | 25 +++++++-------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index d694f2452..16fa0f2f2 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -42,7 +42,7 @@ void Function()? debugCanModifyProviders; /// {@endtemplate} @internal @optionalTypeArgs -abstract class ProviderElement with _OnPauseMixin implements Node { +abstract class ProviderElement implements Node { /// {@macro riverpod.provider_element_base} ProviderElement(this.pointer); @@ -75,8 +75,7 @@ abstract class ProviderElement with _OnPauseMixin implements Node { /// - all of its listeners are "weak" (i.e. created with `listen(weak: true)`) /// /// See also [mayNeedDispose], called when [isActive] may have changed. - bool get isActive => - !_isPaused && (_listenerCount - _pausedActiveSubscriptionCount) > 0; + bool get isActive => (_listenerCount - _pausedActiveSubscriptionCount) > 0; int get _listenerCount => dependents?.length ?? 0; @@ -535,28 +534,20 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu return sub; } - @override - void onCancel() { - ; // TODO notify ancestors - + void _onCancel() { subscriptions?.forEach((sub) { switch (sub) { case ProviderSubscriptionImpl(): - if (sub.weak) return; - sub._listenedElement.pause(); + sub._listenedElement.onSubscriptionPause(sub); } }); } - @override - void onResume() { - ; // TODO notify ancestors - + void _onResume() { subscriptions?.forEach((sub) { switch (sub) { case ProviderSubscriptionImpl(): - if (sub.weak) return; - sub._listenedElement.resume(); + sub._listenedElement.onSubscriptionResume(sub); } }); } @@ -603,12 +594,12 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu switch ((wasActive: wasActive, isActive: isActive)) { case (wasActive: false, isActive: true) when _didCancelOnce: ref?._onResumeListeners?.forEach(runGuarded); - onResume(); + _onResume(); case (wasActive: true, isActive: false): _didCancelOnce = true; ref?._onCancelListeners?.forEach(runGuarded); - onCancel(); + _onCancel(); default: // No state change, so do nothing From 84cbb0476ffa4f642212053a5b3ff2e4f860e411 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Fri, 13 Sep 2024 15:12:25 +0200 Subject: [PATCH 16/27] Fix flutter_riverpod --- .../lib/src/core/consumer.dart | 24 ++++-------- .../test/consumer_listen_test.dart | 17 ++++++--- .../test/src/core/consumer_test.dart | 20 +++++----- packages/riverpod/lib/riverpod.dart | 6 ++- packages/riverpod/lib/src/core/element.dart | 4 +- .../lib/src/core/provider_subscription.dart | 1 + packages/riverpod/lib/src/core/scheduler.dart | 4 +- .../test/old/framework/auto_dispose_test.dart | 8 +++- .../test/old/framework/ref_watch_test.dart | 16 ++++---- .../old/legacy/framework2/framework_test.dart | 18 ++++----- .../test/src/core/modifiers/future_test.dart | 12 ++++-- .../test/src/core/modifiers/select_test.dart | 10 ++++- .../src/core/provider_container_test.dart | 9 +++-- .../test/src/core/provider_element_test.dart | 38 ++++++++++++++----- 14 files changed, 113 insertions(+), 74 deletions(-) diff --git a/packages/flutter_riverpod/lib/src/core/consumer.dart b/packages/flutter_riverpod/lib/src/core/consumer.dart index 161f7fb02..79492aaae 100644 --- a/packages/flutter_riverpod/lib/src/core/consumer.dart +++ b/packages/flutter_riverpod/lib/src/core/consumer.dart @@ -493,13 +493,12 @@ class ConsumerStatefulElement extends StatefulElement implements WidgetRef { final container = ProviderScope.containerOf(this, listen: false); final sub = _ListenManual( - container, - container.listen( + container.listen( provider, listener, onError: onError, fireImmediately: fireImmediately, - ), + ) as ProviderSubscriptionWithOrigin, this, ); listeners.add(sub); @@ -511,30 +510,23 @@ class ConsumerStatefulElement extends StatefulElement implements WidgetRef { BuildContext get context => this; } -final class _ListenManual extends ProviderSubscription { - _ListenManual(super.source, this._subscription, this._element); +final class _ListenManual + // ignore: invalid_use_of_internal_member + extends DelegatingProviderSubscription { + _ListenManual(this.innerSubscription, this._element); @override - bool get isPaused => _subscription.isPaused; - - final ProviderSubscription _subscription; + final ProviderSubscriptionWithOrigin innerSubscription; final ConsumerStatefulElement _element; @override void close() { if (!closed) { - _subscription.close(); _element._manualListeners?.remove(this); } super.close(); } @override - T read() => _subscription.read(); - - @override - void pause() => _subscription.pause(); - - @override - void resume() => _subscription.resume(); + T read() => innerSubscription.read(); } diff --git a/packages/flutter_riverpod/test/consumer_listen_test.dart b/packages/flutter_riverpod/test/consumer_listen_test.dart index 7b22fe6e4..63f6585fe 100644 --- a/packages/flutter_riverpod/test/consumer_listen_test.dart +++ b/packages/flutter_riverpod/test/consumer_listen_test.dart @@ -157,11 +157,12 @@ void main() { ), ); - expect(container.readProviderElement(provider).hasListeners, true); + expect(container.readProviderElement(provider).hasNonWeakListeners, true); await tester.pumpWidget(Container()); - expect(container.readProviderElement(provider).hasListeners, false); + expect( + container.readProviderElement(provider).hasNonWeakListeners, false); }); testWidgets('closes the subscription on provider change', (tester) async { @@ -180,8 +181,10 @@ void main() { ), ); - expect(container.readProviderElement(provider(0)).hasListeners, true); - expect(container.readProviderElement(provider(1)).hasListeners, false); + expect( + container.readProviderElement(provider(0)).hasNonWeakListeners, true); + expect(container.readProviderElement(provider(1)).hasNonWeakListeners, + false); await tester.pumpWidget( UncontrolledProviderScope( @@ -195,8 +198,10 @@ void main() { ), ); - expect(container.readProviderElement(provider(0)).hasListeners, false); - expect(container.readProviderElement(provider(1)).hasListeners, true); + expect(container.readProviderElement(provider(0)).hasNonWeakListeners, + false); + expect( + container.readProviderElement(provider(1)).hasNonWeakListeners, true); }); testWidgets('listen to the new provider on provider change', diff --git a/packages/flutter_riverpod/test/src/core/consumer_test.dart b/packages/flutter_riverpod/test/src/core/consumer_test.dart index f21cdfb06..84b4c97f1 100644 --- a/packages/flutter_riverpod/test/src/core/consumer_test.dart +++ b/packages/flutter_riverpod/test/src/core/consumer_test.dart @@ -281,8 +281,8 @@ void main() { }); expect(buildCount, 1); - expect(familyState0.hasListeners, true); - expect(familyState1.hasListeners, false); + expect(familyState0.hasNonWeakListeners, true); + expect(familyState1.hasNonWeakListeners, false); expect(find.text('0 0'), findsOneWidget); notifier0.increment(); @@ -302,8 +302,8 @@ void main() { expect(buildCount, 3); expect(find.text('1 43'), findsOneWidget); - expect(familyState1.hasListeners, true); - expect(familyState0.hasListeners, true); + expect(familyState1.hasNonWeakListeners, true); + expect(familyState0.hasNonWeakListeners, true); notifier1.increment(); await tester.pump(); @@ -362,8 +362,8 @@ void main() { }); expect(buildCount, 1); - expect(familyState0.hasListeners, true); - expect(familyState1.hasListeners, false); + expect(familyState0.hasNonWeakListeners, true); + expect(familyState1.hasNonWeakListeners, false); expect(find.text('0'), findsOneWidget); notifier0.increment(); @@ -383,8 +383,8 @@ void main() { expect(buildCount, 3); expect(find.text('43'), findsOneWidget); - expect(familyState1.hasListeners, true); - expect(familyState0.hasListeners, false); + expect(familyState1.hasNonWeakListeners, true); + expect(familyState0.hasNonWeakListeners, false); notifier1.increment(); await tester.pump(); @@ -550,7 +550,7 @@ void main() { .getAllProviderElements() .firstWhere((s) => s.provider == provider); - expect(state.hasListeners, true); + expect(state.hasNonWeakListeners, true); expect(find.text('0'), findsOneWidget); await tester.pumpWidget( @@ -559,7 +559,7 @@ void main() { ), ); - expect(state.hasListeners, false); + expect(state.hasNonWeakListeners, false); }); testWidgets('Multiple providers', (tester) async { diff --git a/packages/riverpod/lib/riverpod.dart b/packages/riverpod/lib/riverpod.dart index 0d14e569f..ed7997eb5 100644 --- a/packages/riverpod/lib/riverpod.dart +++ b/packages/riverpod/lib/riverpod.dart @@ -20,7 +20,6 @@ export 'src/framework.dart' Node, ProviderElementProxy, OnError, - WeakNode, ProviderContainerTest, DebugRiverpodDevtoolBiding, TransitiveFamilyOverride, @@ -37,6 +36,11 @@ export 'src/framework.dart' FutureModifierElement, RunNotifierBuild, $FunctionalProvider, + ProviderStateSubscription, + ProviderSubscriptionImpl, + ProviderSubscriptionWithOrigin, + SelectorSubscription, + DelegatingProviderSubscription, $ClassProvider, LegacyProviderMixin, ClassProviderElement, diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index 16fa0f2f2..cd3424afd 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -85,8 +85,8 @@ abstract class ProviderElement implements Node { /// Whether this [ProviderElement] is currently listened to or not. /// /// This maps to listeners added with `listen` and `watch`, - /// including `listen(weak: true)`. - bool get hasListeners => _listenerCount > 0 || weakDependents.isNotEmpty; + /// excluding `listen(weak: true)`. + bool get hasNonWeakListeners => _listenerCount > 0; @visibleForTesting List? subscriptions; diff --git a/packages/riverpod/lib/src/core/provider_subscription.dart b/packages/riverpod/lib/src/core/provider_subscription.dart index 2ad62d1b7..bbb84d413 100644 --- a/packages/riverpod/lib/src/core/provider_subscription.dart +++ b/packages/riverpod/lib/src/core/provider_subscription.dart @@ -92,6 +92,7 @@ mixin _OnPauseMixin { void onCancel(); } +@internal abstract base class DelegatingProviderSubscription extends ProviderSubscriptionImpl { @override diff --git a/packages/riverpod/lib/src/core/scheduler.dart b/packages/riverpod/lib/src/core/scheduler.dart index 1cc859f39..cd2dbce66 100644 --- a/packages/riverpod/lib/src/core/scheduler.dart +++ b/packages/riverpod/lib/src/core/scheduler.dart @@ -125,11 +125,11 @@ class ProviderScheduler { if ((links != null && links.isNotEmpty) || element.container._disposed || - element.isActive) { + element.hasNonWeakListeners) { continue; } - if (!element.hasListeners) { + if (element.weakDependents.isEmpty) { element.container._disposeProvider(element.origin); } else { // Don't delete the pointer if there are some "weak" listeners active. diff --git a/packages/riverpod/test/old/framework/auto_dispose_test.dart b/packages/riverpod/test/old/framework/auto_dispose_test.dart index fe6b50505..2e6c8a23c 100644 --- a/packages/riverpod/test/old/framework/auto_dispose_test.dart +++ b/packages/riverpod/test/old/framework/auto_dispose_test.dart @@ -101,12 +101,16 @@ Future main() async { final container = ProviderContainer.test(); final aDispose = OnDisposeMock('a'); final a = Provider.autoDispose((ref) { - ref.onDispose(aDispose.call); + ref.onDispose(() { + aDispose.call(); + }); return 42; }); final bDispose = OnDisposeMock('b'); final b = Provider.autoDispose((ref) { - ref.onDispose(bDispose.call); + ref.onDispose(() { + bDispose.call(); + }); ref.watch(a); return '42'; }); diff --git a/packages/riverpod/test/old/framework/ref_watch_test.dart b/packages/riverpod/test/old/framework/ref_watch_test.dart index cac71e51c..89845bab3 100644 --- a/packages/riverpod/test/old/framework/ref_watch_test.dart +++ b/packages/riverpod/test/old/framework/ref_watch_test.dart @@ -342,8 +342,8 @@ void main() { verifyOnly(computedListener, computedListener(null, '0 0')); expect(computedBuildCount, 1); - expect(provider0Element.hasListeners, true); - expect(provider1Element.hasListeners, false); + expect(provider0Element.hasNonWeakListeners, true); + expect(provider1Element.hasNonWeakListeners, false); notifier0.increment(); await container.pump(); @@ -363,8 +363,8 @@ void main() { verifyOnly(computedListener, computedListener('1 1', '1 43')); expect(computedBuildCount, 3); - expect(provider1Element.hasListeners, true); - expect(provider0Element.hasListeners, true); + expect(provider1Element.hasNonWeakListeners, true); + expect(provider0Element.hasNonWeakListeners, true); notifier1.increment(); await container.pump(); @@ -410,8 +410,8 @@ void main() { verifyOnly(computedListener, computedListener(null, 0)); expect(computedBuildCount, 1); - expect(provider0Element.hasListeners, true); - expect(provider1Element.hasListeners, false); + expect(provider0Element.hasNonWeakListeners, true); + expect(provider1Element.hasNonWeakListeners, false); notifier0.increment(); await container.pump(); @@ -431,8 +431,8 @@ void main() { expect(computedBuildCount, 3); verifyOnly(computedListener, computedListener(1, 43)); - expect(provider1Element.hasListeners, true); - expect(provider0Element.hasListeners, false); + expect(provider1Element.hasNonWeakListeners, true); + expect(provider0Element.hasNonWeakListeners, false); notifier1.increment(); await container.pump(); diff --git a/packages/riverpod/test/old/legacy/framework2/framework_test.dart b/packages/riverpod/test/old/legacy/framework2/framework_test.dart index c5e98a5e2..eaab9f3af 100644 --- a/packages/riverpod/test/old/legacy/framework2/framework_test.dart +++ b/packages/riverpod/test/old/legacy/framework2/framework_test.dart @@ -269,9 +269,9 @@ void main() { ); expect(firstDependents, [computedElement]); - expect(firstElement.hasListeners, true); + expect(firstElement.hasNonWeakListeners, true); expect(secondDependents, [computedElement]); - expect(secondElement.hasListeners, true); + expect(secondElement.hasNonWeakListeners, true); container.read(first.notifier).state++; expect(sub.read(), 'fallback'); @@ -287,9 +287,9 @@ void main() { listenableVisitor: (_) {}, ); expect(firstDependents, [computedElement]); - expect(firstElement.hasListeners, true); + expect(firstElement.hasNonWeakListeners, true); expect(secondDependents, <$ProviderElement>[]); - expect(secondElement.hasListeners, false); + expect(secondElement.hasNonWeakListeners, false); }); group('overrideWithValue', () { @@ -328,7 +328,7 @@ void main() { listenableVisitor: (_) {}, ); expect(firstDependents, {computedElement}); - expect(firstElement.hasListeners, true); + expect(firstElement.hasNonWeakListeners, true); sub.close(); await container.pump(); @@ -339,7 +339,7 @@ void main() { listenableVisitor: (_) {}, ); expect(firstDependents, <$ProviderElement>{}); - expect(firstElement.hasListeners, false); + expect(firstElement.hasNonWeakListeners, false); }); test( @@ -429,16 +429,16 @@ void main() { final provider = Provider((ref) => ref.watch(first)); final element = container.readProviderElement(provider); - expect(element.hasListeners, false); + expect(element.hasNonWeakListeners, false); final sub = container.listen(provider, didChange.call); - expect(element.hasListeners, true); + expect(element.hasNonWeakListeners, true); sub.close(); counter.increment(); - expect(element.hasListeners, false); + expect(element.hasNonWeakListeners, false); verifyZeroInteractions(didChange); }); diff --git a/packages/riverpod/test/src/core/modifiers/future_test.dart b/packages/riverpod/test/src/core/modifiers/future_test.dart index daedb4fe5..77e595c61 100644 --- a/packages/riverpod/test/src/core/modifiers/future_test.dart +++ b/packages/riverpod/test/src/core/modifiers/future_test.dart @@ -7,7 +7,7 @@ import '../../utils.dart'; void main() { group('provider.future', () { group('handles listen(weak: true)', () { - test('closing the subscription updated element.hasListeners', () { + test('closing the subscription updated element.weakDependents', () { final container = ProviderContainer.test(); final provider = FutureProvider((ref) => 0); @@ -17,11 +17,17 @@ void main() { (previous, value) {}, ); - expect(container.readProviderElement(provider).hasListeners, true); + expect( + container.readProviderElement(provider).weakDependents, + isNotEmpty, + ); sub.close(); - expect(container.readProviderElement(provider).hasListeners, false); + expect( + container.readProviderElement(provider).weakDependents, + isEmpty, + ); }); test( diff --git a/packages/riverpod/test/src/core/modifiers/select_test.dart b/packages/riverpod/test/src/core/modifiers/select_test.dart index a3ed9d935..7d7118476 100644 --- a/packages/riverpod/test/src/core/modifiers/select_test.dart +++ b/packages/riverpod/test/src/core/modifiers/select_test.dart @@ -53,11 +53,17 @@ void main() { (previous, value) {}, ); - expect(container.readProviderElement(provider).hasListeners, true); + expect( + container.readProviderElement(provider).weakDependents, + isNotEmpty, + ); sub.close(); - expect(container.readProviderElement(provider).hasListeners, false); + expect( + container.readProviderElement(provider).weakDependents, + isEmpty, + ); }); test( diff --git a/packages/riverpod/test/src/core/provider_container_test.dart b/packages/riverpod/test/src/core/provider_container_test.dart index dc443fa08..b9f0cdbe1 100644 --- a/packages/riverpod/test/src/core/provider_container_test.dart +++ b/packages/riverpod/test/src/core/provider_container_test.dart @@ -2864,18 +2864,21 @@ void main() { final container = ProviderContainer.test(); final provider = StateProvider((ref) => 0); - expect(container.readProviderElement(provider).hasListeners, false); + expect( + container.readProviderElement(provider).hasNonWeakListeners, false); final sub = container.listen( provider.select((count) => count.isEven), (prev, isEven) {}, ); - expect(container.readProviderElement(provider).hasListeners, true); + expect( + container.readProviderElement(provider).hasNonWeakListeners, true); sub.close(); - expect(container.readProviderElement(provider).hasListeners, false); + expect( + container.readProviderElement(provider).hasNonWeakListeners, false); }); test('can watch selectors', () async { diff --git a/packages/riverpod/test/src/core/provider_element_test.dart b/packages/riverpod/test/src/core/provider_element_test.dart index 1ab81529a..ad35c2b11 100644 --- a/packages/riverpod/test/src/core/provider_element_test.dart +++ b/packages/riverpod/test/src/core/provider_element_test.dart @@ -248,18 +248,18 @@ void main() { }); }); - group('hasListeners', () { - test('includes weak listeners', () { + group('hasNonWeakListeners', () { + test('excludes weak listeners', () { final provider = Provider((ref) => 0); final container = ProviderContainer.test(); final element = container.readProviderElement(provider); - expect(element.hasListeners, false); + expect(element.hasNonWeakListeners, false); container.listen(provider, weak: true, (_, __) {}); - expect(element.hasListeners, true); + expect(element.hasNonWeakListeners, false); }); test('includes provider listeners', () async { @@ -269,11 +269,17 @@ void main() { }); final container = ProviderContainer.test(); - expect(container.readProviderElement(provider).hasListeners, false); + expect( + container.readProviderElement(provider).hasNonWeakListeners, + false, + ); container.read(dep); - expect(container.readProviderElement(provider).hasListeners, true); + expect( + container.readProviderElement(provider).hasNonWeakListeners, + true, + ); }); test('includes provider dependents', () async { @@ -283,22 +289,34 @@ void main() { }); final container = ProviderContainer.test(); - expect(container.readProviderElement(provider).hasListeners, false); + expect( + container.readProviderElement(provider).hasNonWeakListeners, + false, + ); container.read(dep); - expect(container.readProviderElement(provider).hasListeners, true); + expect( + container.readProviderElement(provider).hasNonWeakListeners, + true, + ); }); test('includes container listeners', () async { final provider = Provider((ref) => 0); final container = ProviderContainer.test(); - expect(container.readProviderElement(provider).hasListeners, false); + expect( + container.readProviderElement(provider).hasNonWeakListeners, + false, + ); container.listen(provider, (_, __) {}); - expect(container.readProviderElement(provider).hasListeners, true); + expect( + container.readProviderElement(provider).hasNonWeakListeners, + true, + ); }); }); From 7187a1fa0449afb12ea77aeabefa52a1478b3aaa Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Fri, 13 Sep 2024 16:52:54 +0200 Subject: [PATCH 17/27] Patch tests --- packages/riverpod/lib/src/core/element.dart | 1 - .../lib/src/core/provider_container.dart | 4 ++-- .../lib/src/core/provider_subscription.dart | 4 ++-- .../src/core/proxy_provider_listenable.dart | 5 ++-- packages/riverpod/lib/src/core/ref.dart | 3 +-- packages/riverpod/test/meta_test.dart | 5 ++-- .../test/old/framework/auto_dispose_test.dart | 12 ++++------ .../src/core/provider_container_test.dart | 24 ++++++++++--------- packages/riverpod/test/src/core/ref_test.dart | 3 +-- 9 files changed, 27 insertions(+), 34 deletions(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index cd3424afd..aca8db0d4 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -491,7 +491,6 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu _dependencyMayHaveChanged = true; visitChildren( - // TODO skip paused subscriptions elementVisitor: (element) => element._markDependencyMayHaveChanged(), listenableVisitor: (notifier) => notifier.notifyDependencyMayHaveChanged(), diff --git a/packages/riverpod/lib/src/core/provider_container.dart b/packages/riverpod/lib/src/core/provider_container.dart index 99289ae17..81cc20c3d 100644 --- a/packages/riverpod/lib/src/core/provider_container.dart +++ b/packages/riverpod/lib/src/core/provider_container.dart @@ -487,7 +487,7 @@ class ProviderPointerManager { final _familyPointers = familyPointers[family]; if (_familyPointers == null) return const []; - return _familyPointers.pointers.values.map((e) => e.element).whereNotNull(); + return _familyPointers.pointers.values.map((e) => e.element).nonNulls; } /// Remove a provider from this container. @@ -973,7 +973,7 @@ class ProviderContainer implements Node { return _pointerManager .listProviderPointers() .map((e) => e.element) - .whereNotNull() + .nonNulls .where((e) => e.container == this); } diff --git a/packages/riverpod/lib/src/core/provider_subscription.dart b/packages/riverpod/lib/src/core/provider_subscription.dart index bbb84d413..bd3f27e5e 100644 --- a/packages/riverpod/lib/src/core/provider_subscription.dart +++ b/packages/riverpod/lib/src/core/provider_subscription.dart @@ -2,8 +2,8 @@ part of '../framework.dart'; /// Represents the subscription to a [ProviderListenable]. /// -/// This always is implemented with [ProviderSubscriptionOrigin]. -/// This interface exists to remove the redundant type parameters. +// This always is implemented with ProviderSubscriptionWithOrigin. +// This interface exists to remove the redundant type parameters. @optionalTypeArgs sealed class ProviderSubscription { /// Whether the subscription is closed. diff --git a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart index 18bd6660b..32f2f8e51 100644 --- a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart +++ b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart @@ -124,12 +124,11 @@ class ProviderElementProxy return _ProxySubscription( innerSub, removeListener, - () => read(source), + () => _read(source), ); } - @override - OutT read(Node node) { + OutT _read(Node node) { final element = node.readProviderElement(provider); element.flush(); element.mayNeedDispose(); diff --git a/packages/riverpod/lib/src/core/ref.dart b/packages/riverpod/lib/src/core/ref.dart index 89f3a64f6..f8441570d 100644 --- a/packages/riverpod/lib/src/core/ref.dart +++ b/packages/riverpod/lib/src/core/ref.dart @@ -230,10 +230,9 @@ final = Provider(dependencies: []); /// {@endtemplate} void invalidate(ProviderOrFamily providerOrFamily, {bool asReload = false}) { _throwIfInvalidUsage(); + if (kDebugMode) _debugAssertCanDependOn(providerOrFamily); container.invalidate(providerOrFamily, asReload: asReload); - - if (kDebugMode) _debugAssertCanDependOn(providerOrFamily); } /// Invokes [invalidate] on itself. diff --git a/packages/riverpod/test/meta_test.dart b/packages/riverpod/test/meta_test.dart index 5f4e37176..ccc67210c 100644 --- a/packages/riverpod/test/meta_test.dart +++ b/packages/riverpod/test/meta_test.dart @@ -3,7 +3,6 @@ import 'package:analyzer/dart/analysis/utilities.dart'; import 'package:analyzer/dart/element/element.dart'; import 'package:analyzer/dart/element/type.dart'; import 'package:analyzer/dart/element/visitor.dart'; -import 'package:collection/collection.dart'; import 'package:path/path.dart' as path; import 'package:test/test.dart'; @@ -185,7 +184,7 @@ class _PublicAPIVisitor extends GeneralizingElementVisitor { } void _verifyInheritsAnnotations(Element element) { - final parent = element.enclosingElement; + final parent = element.enclosingElement3; if (parent is! ClassElement) return; @@ -199,7 +198,7 @@ class _PublicAPIVisitor extends GeneralizingElementVisitor { return e.getSetter(element.name); } }) - .whereNotNull() + .nonNulls .toList(); if (overrides.isEmpty) return; diff --git a/packages/riverpod/test/old/framework/auto_dispose_test.dart b/packages/riverpod/test/old/framework/auto_dispose_test.dart index 2e6c8a23c..c8f70ae79 100644 --- a/packages/riverpod/test/old/framework/auto_dispose_test.dart +++ b/packages/riverpod/test/old/framework/auto_dispose_test.dart @@ -99,18 +99,14 @@ Future main() async { test('unsub to A then make B sub to A then unsub to B disposes B before A', () async { final container = ProviderContainer.test(); - final aDispose = OnDisposeMock('a'); + final aDispose = OnDisposeMock(); final a = Provider.autoDispose((ref) { - ref.onDispose(() { - aDispose.call(); - }); + ref.onDispose(aDispose.call); return 42; }); - final bDispose = OnDisposeMock('b'); + final bDispose = OnDisposeMock(); final b = Provider.autoDispose((ref) { - ref.onDispose(() { - bDispose.call(); - }); + ref.onDispose(bDispose.call); ref.watch(a); return '42'; }); diff --git a/packages/riverpod/test/src/core/provider_container_test.dart b/packages/riverpod/test/src/core/provider_container_test.dart index b9f0cdbe1..a562c7737 100644 --- a/packages/riverpod/test/src/core/provider_container_test.dart +++ b/packages/riverpod/test/src/core/provider_container_test.dart @@ -2443,10 +2443,8 @@ void main() { }); test( - 'if a listener removes another provider.listen, the removed listener is still called', - skip: true, () { - ; // Breaks because of the "if (!sub.closed)" in notifyListeners - + 'if a listener removes another provider.listen, the removed listener is not called', + () { final provider = StateProvider((ref) => 0); final container = ProviderContainer.test(); @@ -2470,10 +2468,8 @@ void main() { container.read(provider.notifier).state++; - verifyInOrder([ - listener(0, 1), - listener2(0, 1), - ]); + verifyOnly(listener, listener(0, 1)); + verifyZeroInteractions(listener2); container.read(provider.notifier).state++; @@ -2865,7 +2861,9 @@ void main() { final provider = StateProvider((ref) => 0); expect( - container.readProviderElement(provider).hasNonWeakListeners, false); + container.readProviderElement(provider).hasNonWeakListeners, + false, + ); final sub = container.listen( provider.select((count) => count.isEven), @@ -2873,12 +2871,16 @@ void main() { ); expect( - container.readProviderElement(provider).hasNonWeakListeners, true); + container.readProviderElement(provider).hasNonWeakListeners, + true, + ); sub.close(); expect( - container.readProviderElement(provider).hasNonWeakListeners, false); + container.readProviderElement(provider).hasNonWeakListeners, + false, + ); }); test('can watch selectors', () async { diff --git a/packages/riverpod/test/src/core/ref_test.dart b/packages/riverpod/test/src/core/ref_test.dart index 9cb8c3355..33341d2f4 100644 --- a/packages/riverpod/test/src/core/ref_test.dart +++ b/packages/riverpod/test/src/core/ref_test.dart @@ -1967,7 +1967,7 @@ void main() { }); group('.invalidate', () { - test('Throws if a circular dependency is detected', skip: true, () { + test('Throws if a circular dependency is detected', () { // Regression test for https://github.com/rrousselGit/riverpod/issues/2336 late Ref ref; final a = Provider((r) { @@ -1979,7 +1979,6 @@ void main() { container.read(b); - ; expect( () => ref.invalidate(b), throwsA(isA()), From d38507a2ef34d10cd419369ed87750948ed7a19e Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 15 Sep 2024 00:58:19 +0200 Subject: [PATCH 18/27] Complete pause --- packages/riverpod/lib/riverpod.dart | 2 +- packages/riverpod/lib/src/core/element.dart | 8 +- .../lib/src/core/modifiers/future.dart | 96 +++++++++++------ .../lib/src/core/provider_subscription.dart | 26 ++++- .../src/core/proxy_provider_listenable.dart | 2 +- packages/riverpod/lib/src/core/scheduler.dart | 1 - .../old/legacy/framework2/framework_test.dart | 7 +- ...o_dispose_family_stream_provider_test.dart | 4 + .../auto_dispose_stream_provider_test.dart | 6 ++ .../stream_provider_family_test.dart | 8 ++ .../stream_provider/stream_provider_test.dart | 33 +++++- .../src/core/modifiers/select_async_test.dart | 16 ++- .../src/core/provider_subscription_test.dart | 89 ++++++++++++++++ .../src/providers/stream_notifier_test.dart | 100 ++++++++++++++---- packages/riverpod/test/src/utils.dart | 98 +++++++++++++++++ 15 files changed, 427 insertions(+), 69 deletions(-) diff --git a/packages/riverpod/lib/riverpod.dart b/packages/riverpod/lib/riverpod.dart index ed7997eb5..95d8a9e24 100644 --- a/packages/riverpod/lib/riverpod.dart +++ b/packages/riverpod/lib/riverpod.dart @@ -32,7 +32,7 @@ export 'src/framework.dart' $FutureModifier, ProviderElement, ClassBaseX, - CancelAsyncSubscription, + AsyncSubscription, FutureModifierElement, RunNotifierBuild, $FunctionalProvider, diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index aca8db0d4..82f55d69b 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -533,7 +533,7 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu return sub; } - void _onCancel() { + void onCancel() { subscriptions?.forEach((sub) { switch (sub) { case ProviderSubscriptionImpl(): @@ -542,7 +542,7 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu }); } - void _onResume() { + void onResume() { subscriptions?.forEach((sub) { switch (sub) { case ProviderSubscriptionImpl(): @@ -593,12 +593,12 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu switch ((wasActive: wasActive, isActive: isActive)) { case (wasActive: false, isActive: true) when _didCancelOnce: ref?._onResumeListeners?.forEach(runGuarded); - _onResume(); + onResume(); case (wasActive: true, isActive: false): _didCancelOnce = true; ref?._onCancelListeners?.forEach(runGuarded); - _onCancel(); + onCancel(); default: // No state change, so do nothing diff --git a/packages/riverpod/lib/src/core/modifiers/future.dart b/packages/riverpod/lib/src/core/modifiers/future.dart index 2a6ea31b5..5319a467f 100644 --- a/packages/riverpod/lib/src/core/modifiers/future.dart +++ b/packages/riverpod/lib/src/core/modifiers/future.dart @@ -2,7 +2,11 @@ part of '../../framework.dart'; /// Internal typedef for cancelling the subscription to an async operation @internal -typedef CancelAsyncSubscription = void Function(); +typedef AsyncSubscription = ({ + void Function() cancel, + void Function()? pause, + void Function()? resume, +}); /// Implementation detail of `riverpod_generator`. /// Do not use. @@ -231,8 +235,7 @@ mixin FutureModifierElement on ProviderElement> { final futureNotifier = ProxyElementValueListenable>(); Completer? _futureCompleter; Future? _lastFuture; - CancelAsyncSubscription? _lastFutureSub; - CancelAsyncSubscription? _cancelSubscription; + AsyncSubscription? _cancelSubscription; /// Internal utility for transitioning an [AsyncValue] after a provider refresh. /// @@ -344,16 +347,30 @@ mixin FutureModifierElement on ProviderElement> { required error, required last, }) { - final rawStream = create(); - final stream = rawStream.isBroadcast - ? rawStream - : rawStream.asBroadcastStream(onCancel: (sub) => sub.cancel()); + final stream = create(); + + return stream.listenAndTrackLast( + last, + lastOrElseError: _missingLastValueError, + onData: data, + onError: error, + onDone: done, + ); + }); + } - stream.lastCancelable(last, orElseError: _missingLastValueError); + @override + void onCancel() { + super.onCancel(); - final sub = stream.listen(data, onError: error, onDone: done); - return sub.cancel; - }); + _cancelSubscription?.pause?.call(); + } + + @override + void onResume() { + super.onResume(); + + _cancelSubscription?.resume?.call(); } StateError _missingLastValueError() { @@ -405,17 +422,21 @@ mixin FutureModifierElement on ProviderElement> { last(futureOr, cancel); - return cancel; + return ( + cancel: cancel, + pause: null, + resume: null, + ); }); } /// Listens to a [Future] and transforms it into an [AsyncValue]. void _handleAsync( - CancelAsyncSubscription? Function({ + AsyncSubscription? Function({ required void Function(StateT) data, required void Function(Object, StackTrace) error, required void Function() done, - required void Function(Future, CancelAsyncSubscription) last, + required void Function(Future, void Function()) last, }) listen, { required bool seamless, }) { @@ -431,13 +452,9 @@ mixin FutureModifierElement on ProviderElement> { }, last: (last, sub) { assert(_lastFuture == null, 'bad state'); - assert(_lastFutureSub == null, 'bad state'); _lastFuture = last; - _lastFutureSub = sub; }, done: () { - _lastFutureSub?.call(); - _lastFutureSub = null; _lastFuture = null; }, ); @@ -457,10 +474,8 @@ mixin FutureModifierElement on ProviderElement> { @internal void runOnDispose() { // Stops listening to the previous async operation - _lastFutureSub?.call(); - _lastFutureSub = null; _lastFuture = null; - _cancelSubscription?.call(); + _cancelSubscription?.cancel(); _cancelSubscription = null; super.runOnDispose(); } @@ -475,23 +490,21 @@ mixin FutureModifierElement on ProviderElement> { final lastFuture = _lastFuture; if (lastFuture != null) { - // The completer will be completed by the while loop in handleStream - final cancelSubscription = _cancelSubscription; if (cancelSubscription != null) { - completer.future + cancelSubscription.resume?.call(); + lastFuture .then( (_) {}, // ignore: avoid_types_on_closure_parameters onError: (Object _) {}, ) - .whenComplete(cancelSubscription); + .whenComplete(cancelSubscription.cancel); } // Prevent super.dispose from cancelling the subscription on the "last" // stream value, so that it can be sent to `provider.future`. _lastFuture = null; - _lastFutureSub = null; _cancelSubscription = null; } else { // The listened stream completed during a "loading" state. @@ -519,19 +532,26 @@ mixin FutureModifierElement on ProviderElement> { } extension on Stream { - void lastCancelable( - void Function(Future, CancelAsyncSubscription) last, { - required Object Function() orElseError, + AsyncSubscription listenAndTrackLast( + void Function(Future, void Function()) last, { + required Object Function() lastOrElseError, + required void Function(T event) onData, + required void Function(Object error, StackTrace stackTrace) onError, + required void Function() onDone, }) { - late StreamSubscription subscription; final completer = Completer(); Result? result; + late StreamSubscription subscription; subscription = listen( - (event) => result = Result.data(event), + (event) { + result = Result.data(event); + onData(event); + }, // ignore: avoid_types_on_closure_parameters onError: (Object error, StackTrace stackTrace) { result = Result.error(error, stackTrace); + onError(error, stackTrace); }, onDone: () { if (result != null) { @@ -550,13 +570,23 @@ extension on Stream { completer.future.ignore(); completer.completeError( - orElseError(), + lastOrElseError(), StackTrace.current, ); } + + onDone(); }, ); - last(completer.future, subscription.cancel); + final asyncSub = ( + cancel: subscription.cancel, + pause: subscription.pause, + resume: subscription.resume + ); + + last(completer.future, asyncSub.cancel); + + return asyncSub; } } diff --git a/packages/riverpod/lib/src/core/provider_subscription.dart b/packages/riverpod/lib/src/core/provider_subscription.dart index bd3f27e5e..a4290f0a3 100644 --- a/packages/riverpod/lib/src/core/provider_subscription.dart +++ b/packages/riverpod/lib/src/core/provider_subscription.dart @@ -186,7 +186,10 @@ final class ProviderStateSubscription /// Whether an event was sent while this subscription was paused. /// /// This enables re-rending the last missing event when the subscription is resumed. - var _missedCalled = false; + ({ + (StateT?, StateT)? data, + (Object, StackTrace)? error, + })? _missedCalled; @override StateT read() { @@ -203,12 +206,27 @@ final class ProviderStateSubscription void onCancel() => _listenedElement.onSubscriptionPause(this); @override - void onResume() => _listenedElement.onSubscriptionResume(this); + void onResume() { + _listenedElement.onSubscriptionResume(this); + if (_missedCalled?.data case final event?) { + final prev = event.$1; + final next = event.$2; + + _missedCalled = null; + listener(prev, next); + } else if (_missedCalled?.error case final event?) { + final error = event.$1; + final stackTrace = event.$2; + + _missedCalled = null; + onError(error, stackTrace); + } + } @override void _notify(StateT? prev, StateT next) { if (_isPaused) { - _missedCalled = true; + _missedCalled = (data: (prev, next), error: null); return; } @@ -218,7 +236,7 @@ final class ProviderStateSubscription @override void _notifyError(Object error, StackTrace stackTrace) { if (_isPaused) { - _missedCalled = true; + _missedCalled = (data: null, error: (error, stackTrace)); return; } diff --git a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart index 32f2f8e51..e50ffbc1b 100644 --- a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart +++ b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart @@ -86,7 +86,7 @@ class ProviderElementProxy final element = source.readProviderElement(provider); ; // TODO test weak proxy listener does not break what's said after this. - // While we don't care about changes to the element, calling _listenElement + // While we don't care about changes to the element, calling addListener // is necessary to tell the listened element that it is being listened. // We do it at the top of the file to trigger a "flush" before adding // a listener to the notifier. diff --git a/packages/riverpod/lib/src/core/scheduler.dart b/packages/riverpod/lib/src/core/scheduler.dart index cd2dbce66..51e140438 100644 --- a/packages/riverpod/lib/src/core/scheduler.dart +++ b/packages/riverpod/lib/src/core/scheduler.dart @@ -120,7 +120,6 @@ class ProviderScheduler { /// and the second time it is traversed, it won't anymore. for (var i = 0; i < _stateToDispose.length; i++) { final element = _stateToDispose[i]; - final links = element.ref?._keepAliveLinks; if ((links != null && links.isNotEmpty) || diff --git a/packages/riverpod/test/old/legacy/framework2/framework_test.dart b/packages/riverpod/test/old/legacy/framework2/framework_test.dart index eaab9f3af..6f6cd38c5 100644 --- a/packages/riverpod/test/old/legacy/framework2/framework_test.dart +++ b/packages/riverpod/test/old/legacy/framework2/framework_test.dart @@ -133,14 +133,15 @@ void main() { .maybeWhen(data: (d) => d, orElse: () => null); }); - expect(callCount, 0); - expect(container.read(provider), null); + final sub = container.listen(provider, (p, n) {}); + + expect(sub.read(), null); expect(callCount, 1); controller.add(42); expect(callCount, 1); - expect(container.read(provider), 42); + expect(sub.read(), 42); expect(callCount, 2); }); diff --git a/packages/riverpod/test/old/legacy_providers/stream_provider/auto_dispose_family_stream_provider_test.dart b/packages/riverpod/test/old/legacy_providers/stream_provider/auto_dispose_family_stream_provider_test.dart index f8df2354a..b417d6e83 100644 --- a/packages/riverpod/test/old/legacy_providers/stream_provider/auto_dispose_family_stream_provider_test.dart +++ b/packages/riverpod/test/old/legacy_providers/stream_provider/auto_dispose_family_stream_provider_test.dart @@ -27,6 +27,8 @@ void main() { overrides: [provider], ); + container.listen(provider(0), (p, n) {}); + expect(await container.read(provider(0).future), 0); expect(container.read(provider(0)), const AsyncData(0)); expect(root.getAllProviderElements(), isEmpty); @@ -100,6 +102,8 @@ void main() { overrides: [dep.overrideWithValue(42)], ); + container.listen(provider(10), (p, n) {}); + await expectLater(container.read(provider(10).future), completion(52)); expect(container.read(provider(10)), const AsyncData(52)); diff --git a/packages/riverpod/test/old/legacy_providers/stream_provider/auto_dispose_stream_provider_test.dart b/packages/riverpod/test/old/legacy_providers/stream_provider/auto_dispose_stream_provider_test.dart index e32033b71..6bba9449c 100644 --- a/packages/riverpod/test/old/legacy_providers/stream_provider/auto_dispose_stream_provider_test.dart +++ b/packages/riverpod/test/old/legacy_providers/stream_provider/auto_dispose_stream_provider_test.dart @@ -66,6 +66,8 @@ void main() { overrides: [dep.overrideWithValue(42)], ); + container.listen(provider, (p, n) {}); + await expectLater(container.read(provider.future), completion(42)); expect(container.read(provider), const AsyncData(42)); @@ -200,6 +202,8 @@ void main() { overrides: [provider], ); + container.listen(provider, (p, n) {}); + expect(await container.read(provider.future), 0); expect(container.read(provider), const AsyncValue.data(0)); expect(root.getAllProviderElements(), isEmpty); @@ -248,6 +252,8 @@ void main() { ], ); + container.listen(provider, (p, n) {}); + expect(await container.read(provider.future), 42); expect(container.read(provider), const AsyncValue.data(42)); expect(root.getAllProviderElements(), isEmpty); diff --git a/packages/riverpod/test/old/legacy_providers/stream_provider/stream_provider_family_test.dart b/packages/riverpod/test/old/legacy_providers/stream_provider/stream_provider_family_test.dart index c08e3b860..b7bd0f7cf 100644 --- a/packages/riverpod/test/old/legacy_providers/stream_provider/stream_provider_family_test.dart +++ b/packages/riverpod/test/old/legacy_providers/stream_provider/stream_provider_family_test.dart @@ -24,6 +24,8 @@ void main() { overrides: [provider], ); + container.listen(provider(0), (p, n) {}); + expect(await container.read(provider(0).future), 0); expect(container.read(provider(0)), const AsyncData(0)); expect(root.getAllProviderElements(), isEmpty); @@ -49,6 +51,8 @@ void main() { ], ); + container.listen(provider(0), (p, n) {}); + expect(await container.read(provider(0).future), 42); expect(container.read(provider(0)), const AsyncData(42)); expect(root.getAllProviderElements(), isEmpty); @@ -77,6 +81,8 @@ void main() { overrides: [dep.overrideWithValue(42)], ); + container.listen(provider(10), (p, n) {}); + await expectLater(container.read(provider(10).future), completion(52)); expect(container.read(provider(10)), const AsyncData(52)); @@ -93,6 +99,8 @@ void main() { ], ); + container.listen(provider(0), (p, n) {}); + expect(container.read(provider(0)), const AsyncValue.loading()); await container.pump(); diff --git a/packages/riverpod/test/old/legacy_providers/stream_provider/stream_provider_test.dart b/packages/riverpod/test/old/legacy_providers/stream_provider/stream_provider_test.dart index fc2f69073..13d3e45b6 100644 --- a/packages/riverpod/test/old/legacy_providers/stream_provider/stream_provider_test.dart +++ b/packages/riverpod/test/old/legacy_providers/stream_provider/stream_provider_test.dart @@ -81,7 +81,7 @@ void main() { return Stream.value(0); }); - container.read(provider); + container.listen(provider, (p, n) {}); expect(state, const AsyncLoading()); @@ -102,6 +102,8 @@ void main() { var count = 0; final provider = StreamProvider((ref) => Stream.value(count++)); + container.listen(provider, (p, n) {}); + await expectLater(container.read(provider.future), completion(0)); expect(container.read(provider), const AsyncData(0)); @@ -129,6 +131,8 @@ void main() { final dep = StateProvider((ref) => 0); final provider = StreamProvider((ref) => Stream.value(ref.watch(dep))); + container.listen(provider, (p, n) {}); + await expectLater(container.read(provider.future), completion(0)); expect(container.read(provider), const AsyncData(0)); @@ -150,6 +154,8 @@ void main() { final dep = StateProvider((ref) => 0); final provider = StreamProvider((ref) => Stream.value(ref.watch(dep))); + container.listen(provider, (p, n) {}); + await expectLater(container.read(provider.future), completion(0)); expect(container.read(provider), const AsyncData(0)); @@ -219,6 +225,7 @@ void main() { overrides: [dep.overrideWithValue(42)], ); + container.listen(provider, (p, n) {}); await expectLater(container.read(provider.future), completion(42)); expect(container.read(provider), const AsyncData(42)); @@ -235,6 +242,7 @@ void main() { final controller = StreamController(); addTearDown(controller.close); + container.listen(provider, (p, n) {}); await expectLater( container.read(provider.future), completion(42), @@ -275,6 +283,7 @@ void main() { final container = ProviderContainer.test(); final provider = StreamProvider((ref) => Stream.value(result)); + container.listen(provider, (p, n) {}); expect(await container.read(provider.future), 0); expect(container.read(provider), const AsyncValue.data(0)); @@ -300,6 +309,7 @@ void main() { overrides: [provider], ); + container.listen(provider, (p, n) {}); expect(await container.read(provider.future), 0); expect(container.read(provider), const AsyncValue.data(0)); expect(root.getAllProviderElements(), isEmpty); @@ -324,6 +334,7 @@ void main() { ], ); + container.listen(provider, (p, n) {}); expect(await container.read(provider.future), 42); expect(container.read(provider), const AsyncValue.data(42)); expect(root.getAllProviderElements(), isEmpty); @@ -348,6 +359,7 @@ void main() { ], ); + container.listen(provider, (p, n) {}); expect(await container.read(provider.future), 42); expect(container.read(provider), const AsyncValue.data(42)); expect(root.getAllProviderElements(), isEmpty); @@ -366,6 +378,8 @@ void main() { addTearDown(() => controller.close); final provider = StreamProvider((ref) => controller.stream); + container.listen(provider, (p, n) {}); + expect(container.read(provider), const AsyncValue.loading()); controller.add(42); @@ -379,6 +393,8 @@ void main() { addTearDown(() => controller.close); final provider = StreamProvider((ref) => controller.stream); + container.listen(provider, (p, n) {}); + expect(container.read(provider), const AsyncValue.loading()); final stack = StackTrace.current; @@ -436,6 +452,7 @@ void main() { provider.overrideWithValue(AsyncValue.error(error, StackTrace.empty)), ]); + container.listen(provider, (p, n) {}); expect(container.read(provider.future), future); await expectLater(future, throwsA(error)); @@ -466,6 +483,7 @@ void main() { ], ); + container.listen(provider, (p, n) {}); var future = container.read(provider.future); final error = Error(); @@ -499,6 +517,7 @@ void main() { ], ); + container.listen(provider, (p, n) {}); var future = container.read(provider.future); container.updateOverrides([ @@ -529,6 +548,7 @@ void main() { ], ); + container.listen(provider, (p, n) {}); var future = container.read(provider.future); container.updateOverrides([ @@ -657,6 +677,8 @@ void main() { }); final container = ProviderContainer.test(); + container.listen(provider(0), (p, n) {}); + expect(container.read(provider(0)), const AsyncValue.loading()); await container.pump(); @@ -786,6 +808,7 @@ void main() { final controller = StreamController(); final provider = StreamProvider((_) => controller.stream); + container.listen(provider, (p, n) {}); final future = container.read(provider.future); controller.add(42); @@ -802,6 +825,7 @@ void main() { controller.add(42); + container.listen(provider, (p, n) {}); final future = container.read(provider.future); await expectLater(future, completion(42)); @@ -814,6 +838,7 @@ void main() { final controller = StreamController(); final provider = StreamProvider((_) => controller.stream); + container.listen(provider, (p, n) {}); final future = container.read(provider.future); controller.addError(42); @@ -830,6 +855,7 @@ void main() { controller.addError(42); + container.listen(provider, (p, n) {}); final future = container.read(provider.future); await expectLater(future, throwsA(42)); @@ -847,6 +873,7 @@ void main() { ], ); + container.listen(provider, (p, n) {}); final future = container.read(provider.future); container.updateOverrides([ @@ -868,6 +895,7 @@ void main() { provider.overrideWithValue(const AsyncValue.data(42)), ]); + container.listen(provider, (p, n) {}); final future = container.read(provider.future); await expectLater(future, completion(42)); @@ -881,6 +909,7 @@ void main() { ], ); + container.listen(provider, (p, n) {}); final future = container.read(provider.future); container.updateOverrides([ @@ -904,6 +933,7 @@ void main() { .overrideWithValue(const AsyncValue.error(42, StackTrace.empty)), ]); + container.listen(provider, (p, n) {}); final future = container.read(provider.future); await expectLater(future, throwsA(42)); @@ -917,6 +947,7 @@ void main() { ], ); + container.listen(provider, (p, n) {}); final future = container.read(provider.future); await expectLater(future, completion(42)); diff --git a/packages/riverpod/test/src/core/modifiers/select_async_test.dart b/packages/riverpod/test/src/core/modifiers/select_async_test.dart index 2c75a54cc..9cc5fa94d 100644 --- a/packages/riverpod/test/src/core/modifiers/select_async_test.dart +++ b/packages/riverpod/test/src/core/modifiers/select_async_test.dart @@ -16,6 +16,7 @@ void main() { addTearDown(controller.close); final dep = StreamProvider((ref) => controller.stream); + container.listen(dep, (p, n) {}); final ref = container.read(provider); final future = ref.watch(dep.selectAsync((data) => data * 2)); @@ -32,6 +33,7 @@ void main() { addTearDown(controller.close); final dep = StreamProvider((ref) => controller.stream); + container.listen(dep, (p, n) {}); final ref = container.read(provider); final future = ref.watch(dep.selectAsync((data) => data * 2)); @@ -233,7 +235,9 @@ void main() { return ref.watch(a.selectAsync((value) => value % 10)); }); - expect(buildCount, 0); + container.listen(a, (p, n) {}); + container.listen(b, (p, n) {}); + expect(container.read(a), const AsyncLoading()); expect(container.read(b), const AsyncLoading()); expect(await container.read(b.future), 0); @@ -289,7 +293,9 @@ void main() { return ref.watch(a.selectAsync((value) => value % 10)); }); - expect(buildCount, 0); + container.listen(a, (p, n) {}); + container.listen(b, (p, n) {}); + expect(container.read(b), const AsyncLoading()); expect(container.read(b), const AsyncLoading()); expect(await container.read(b.future), 0); @@ -340,6 +346,8 @@ void main() { final container = ProviderContainer.test(); final provider = FutureProvider((ref) async => 0); + container.listen(provider, (p, n) {}); + expect( container.read(provider.selectAsync((data) => data.toString())), completion('0'), @@ -351,6 +359,8 @@ void main() { final provider = FutureProvider((ref) async => throw StateError('err')); + container.listen(provider, (p, n) {}); + expect( container.read(provider.selectAsync((data) => data)), throwsStateError, @@ -361,6 +371,8 @@ void main() { final container = ProviderContainer.test(); final provider = FutureProvider((ref) async => 42); + container.listen(provider, (p, n) {}); + expect( container.read(provider.selectAsync((data) => throw StateError('err'))), throwsStateError, diff --git a/packages/riverpod/test/src/core/provider_subscription_test.dart b/packages/riverpod/test/src/core/provider_subscription_test.dart index 6c8ed053b..e3a882b8d 100644 --- a/packages/riverpod/test/src/core/provider_subscription_test.dart +++ b/packages/riverpod/test/src/core/provider_subscription_test.dart @@ -1,6 +1,10 @@ +import 'package:mockito/mockito.dart'; import 'package:riverpod/riverpod.dart'; import 'package:test/test.dart'; +import '../matrix.dart'; +import '../utils.dart'; + void main() { group('_ProxySubscription', () { test('handles pause/resume', () { @@ -42,4 +46,89 @@ void main() { expect(element.isActive, true); }); }); + + group('ProviderSubscription.resume', () { + test( + 'Resuming a paused subscription with no missed data event does not call listeners', + () { + final container = ProviderContainer.test(); + final provider = Provider((ref) => 0); + final listener = Listener(); + + final sub = container.listen(provider, listener.call); + + sub.pause(); + + sub.resume(); + + verifyZeroInteractions(listener); + }); + + test('Resuming a paused subscription with missed data emits the last event', + () async { + final container = ProviderContainer.test(); + final provider = NotifierProvider, int>( + () => DeferredNotifier((ref) => 0), + ); + final listener = Listener(); + + final notifier = container.read(provider.notifier); + + final sub = container.listen(provider, listener.call); + + sub.pause(); + + notifier.state = 1; + notifier.state = 2; + + sub.resume(); + + verifyOnly(listener, listener(1, 2)); + + sub.resume(); + + verifyNoMoreInteractions(listener); + }); + + test( + 'Resuming a paused subscription with missed error emits the last error', + () async { + final container = ProviderContainer.test(); + late Error toThrow; + final stack = StackTrace.current; + final provider = Provider((ref) { + Error.throwWithStackTrace(toThrow, stack); + }); + final listener = Listener(); + final onError = ErrorListener(); + final err = Error(); + final err2 = Error(); + + toThrow = err; + + final sub = container.listen( + provider, + listener.call, + onError: onError.call, + ); + + sub.pause(); + + toThrow = err2; + try { + container.refresh(provider); + } catch (e) { + // Will rethrow the error, but we don't care about it here + } + + sub.resume(); + + verifyOnly(onError, onError(err2, stack)); + + sub.resume(); + + verifyNoMoreInteractions(onError); + verifyZeroInteractions(listener); + }); + }); } diff --git a/packages/riverpod/test/src/providers/stream_notifier_test.dart b/packages/riverpod/test/src/providers/stream_notifier_test.dart index 602a34b13..4b486c9c4 100644 --- a/packages/riverpod/test/src/providers/stream_notifier_test.dart +++ b/packages/riverpod/test/src/providers/stream_notifier_test.dart @@ -29,8 +29,72 @@ void main() { }); streamNotifierProviderFactory.createGroup((factory) { - test('Pauses the Stream when the provider is paused', skip: true, () { - ; + test('closes the StreamSubscription upon disposing the provider', () async { + final onCancel = OnCancelMock(); + final container = ProviderContainer.test(); + final cancelCompleter = Completer.sync(); + final provider = factory.simpleTestProvider((ref) { + final controller = StreamController(); + ref.onDispose(() { + controller.addError(42); + controller.close(); + }); + + return DelegatingStream( + controller.stream, + onSubscriptionCancel: () { + onCancel.call(); + cancelCompleter.complete(); + }, + ); + }); + + container.listen(provider, (previous, next) {}); + final future = container.read(provider.future); + + container.dispose(); + + verifyZeroInteractions(onCancel); + + await expectLater(future, throwsA(42)); + await cancelCompleter.future; + + verifyOnly(onCancel, onCancel()); + }); + + test('Pauses the Stream when the provider is paused', () { + final streamController = StreamController(); + addTearDown(streamController.close); + + final onSubPause = OnPause(); + final onSubResume = OnResume(); + + final container = ProviderContainer.test(); + + final provider = factory.simpleTestProvider( + (ref) { + return DelegatingStream( + streamController.stream, + onSubscriptionPause: onSubPause.call, + onSubscriptionResume: onSubResume.call, + ); + }, + ); + + final sub = container.listen(provider, (previous, next) {}); + + verifyZeroInteractions(onSubPause); + verifyZeroInteractions(onSubResume); + + sub.pause(); + + verifyOnly(onSubPause, onSubPause()); + verifyZeroInteractions(onSubResume); + + sub.resume(); + + verifyOnly(onSubResume, onSubResume()); + verifyNoMoreInteractions(onSubPause); }); test('Cannot share a Notifier instance between providers ', () { @@ -586,29 +650,27 @@ void main() { }, ); - test( - 'if going back to loading after future resolved, throws StateError', - () async { - final container = ProviderContainer.test(); - final completer = Completer.sync(); - final provider = factory.simpleTestProvider( - (ref) => Stream.fromFuture(completer.future), - ); + test('if going back to loading after future resolved, throws StateError', + () async { + final container = ProviderContainer.test(); + final completer = Completer.sync(); + final provider = factory.simpleTestProvider( + (ref) => Stream.fromFuture(completer.future), + ); - container.read(provider); + container.listen(provider, (previous, next) {}); - completer.complete(42); + completer.complete(42); - container.read(provider.notifier).state = const AsyncData(42); - container.read(provider.notifier).state = const AsyncLoading(); + container.read(provider.notifier).state = const AsyncData(42); + container.read(provider.notifier).state = const AsyncLoading(); - final future = container.read(provider.future); + final future = container.read(provider.future); - container.dispose(); + container.dispose(); - await expectLater(future, throwsStateError); - }, - ); + await expectLater(future, throwsStateError); + }); test( 'resolves with the new state if StreamNotifier.state is modified during loading', diff --git a/packages/riverpod/test/src/utils.dart b/packages/riverpod/test/src/utils.dart index d9d254514..0d0292c8a 100644 --- a/packages/riverpod/test/src/utils.dart +++ b/packages/riverpod/test/src/utils.dart @@ -20,6 +20,100 @@ List captureErrors(List cb) { class ProviderObserverMock extends Mock implements ProviderObserver {} +class StreamSubscriptionView implements StreamSubscription { + StreamSubscriptionView(this.inner); + + final StreamSubscription inner; + + @override + Future asFuture([E? futureValue]) => inner.asFuture(futureValue); + + @override + Future cancel() => inner.cancel(); + + @override + bool get isPaused => inner.isPaused; + + @override + void onData(void Function(T data)? handleData) => inner.onData(handleData); + + @override + void onDone(void Function()? handleDone) => inner.onDone(handleDone); + + @override + void onError(Function? handleError) => inner.onError(handleError); + + @override + void pause([Future? resumeSignal]) => inner.pause(resumeSignal); + + @override + void resume() => inner.resume(); +} + +class _DelegatingStreamSubscription extends StreamSubscriptionView { + _DelegatingStreamSubscription( + super.inner, { + this.onSubscriptionPause, + this.onSubscriptionResume, + this.onSubscriptionCancel, + }); + + final void Function()? onSubscriptionPause; + final void Function()? onSubscriptionResume; + final void Function()? onSubscriptionCancel; + + @override + Future cancel() { + onSubscriptionCancel?.call(); + return super.cancel(); + } + + @override + void pause([Future? resumeSignal]) { + onSubscriptionPause?.call(); + super.pause(resumeSignal); + } + + @override + void resume() { + onSubscriptionResume?.call(); + super.resume(); + } +} + +class DelegatingStream extends StreamView { + DelegatingStream( + super.stream, { + this.onSubscriptionPause, + this.onSubscriptionResume, + this.onSubscriptionCancel, + }); + + final void Function()? onSubscriptionPause; + final void Function()? onSubscriptionResume; + final void Function()? onSubscriptionCancel; + + @override + StreamSubscription listen( + void Function(T event)? onData, { + Function? onError, + void Function()? onDone, + bool? cancelOnError, + }) { + return _DelegatingStreamSubscription( + super.listen( + onData, + onError: onError, + onDone: onDone, + cancelOnError: cancelOnError, + ), + onSubscriptionPause: onSubscriptionPause, + onSubscriptionResume: onSubscriptionResume, + onSubscriptionCancel: onSubscriptionCancel, + ); + } +} + class OverrideWithBuildMock extends Mock { OverrideWithBuildMock(this.fallback); @@ -50,6 +144,10 @@ class OnResume extends Mock { void call(); } +class OnPause extends Mock { + void call(); +} + class OnAddListener extends Mock { void call(ProviderSubscription? sub); } From 66e0a897b9ece884e4f60c453938f0d2fd087484 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 15 Sep 2024 01:17:06 +0200 Subject: [PATCH 19/27] Cleanup --- packages/riverpod/lib/src/core/element.dart | 8 +-- .../src/core/proxy_provider_listenable.dart | 1 - packages/riverpod/lib/src/core/ref.dart | 8 +-- .../test/old/framework/modifiers_test.dart | 12 ++-- packages/riverpod/test/old/utils.dart | 4 +- packages/riverpod/test/src/core/ref_test.dart | 60 +++++++++---------- packages/riverpod/test/src/utils.dart | 4 +- 7 files changed, 46 insertions(+), 51 deletions(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index 82f55d69b..44b7e0c3c 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -606,16 +606,12 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu if (_listenerCount < previousListenerCount) { if (ref?._onRemoveListeners case final listeners?) { - for (final listener in listeners) { - runUnaryGuarded(listener, sub); - } + listeners.forEach(runGuarded); } mayNeedDispose(); } else if (_listenerCount > previousListenerCount) { if (ref?._onAddListeners case final listeners?) { - for (final listener in listeners) { - runUnaryGuarded(listener, sub); - } + listeners.forEach(runGuarded); } } } diff --git a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart index e50ffbc1b..ba7f741f9 100644 --- a/packages/riverpod/lib/src/core/proxy_provider_listenable.dart +++ b/packages/riverpod/lib/src/core/proxy_provider_listenable.dart @@ -85,7 +85,6 @@ class ProviderElementProxy }) { final element = source.readProviderElement(provider); - ; // TODO test weak proxy listener does not break what's said after this. // While we don't care about changes to the element, calling addListener // is necessary to tell the listened element that it is being listened. // We do it at the top of the file to trigger a "flush" before adding diff --git a/packages/riverpod/lib/src/core/ref.dart b/packages/riverpod/lib/src/core/ref.dart index f8441570d..afd6015de 100644 --- a/packages/riverpod/lib/src/core/ref.dart +++ b/packages/riverpod/lib/src/core/ref.dart @@ -47,8 +47,8 @@ base class Ref { List? _onDisposeListeners; List? _onResumeListeners; List? _onCancelListeners; - List? _onAddListeners; - List? _onRemoveListeners; + List? _onAddListeners; + List? _onRemoveListeners; List? _onErrorSelfListeners; bool get mounted => _mounted; @@ -280,7 +280,7 @@ final = Provider(dependencies: []); /// /// See also: /// - [onRemoveListener], for when a listener is removed - void onAddListener(void Function(ProviderSubscription sub) cb) { + void onAddListener(void Function() cb) { _throwIfInvalidUsage(); _onAddListeners ??= []; @@ -291,7 +291,7 @@ final = Provider(dependencies: []); /// /// See also: /// - [onAddListener], for when a listener is added - void onRemoveListener(void Function(ProviderSubscription sub) cb) { + void onRemoveListener(void Function() cb) { _throwIfInvalidUsage(); _onRemoveListeners ??= []; diff --git a/packages/riverpod/test/old/framework/modifiers_test.dart b/packages/riverpod/test/old/framework/modifiers_test.dart index da7ce4ae1..ac28709a1 100644 --- a/packages/riverpod/test/old/framework/modifiers_test.dart +++ b/packages/riverpod/test/old/framework/modifiers_test.dart @@ -44,31 +44,31 @@ void main() { final sub = container.listen(provider.future, (prev, value) {}); - verifyOnly(onAddListener, onAddListener(any)); + verifyOnly(onAddListener, onAddListener()); final sub2 = container.listen(provider.future, (prev, value) {}); - verifyOnly(onAddListener, onAddListener(any)); + verifyOnly(onAddListener, onAddListener()); sub.close(); - verifyOnly(onRemoveListener, onRemoveListener(any)); + verifyOnly(onRemoveListener, onRemoveListener()); verifyZeroInteractions(onCancel); sub2.close(); - verifyOnly(onRemoveListener, onRemoveListener(any)); + verifyOnly(onRemoveListener, onRemoveListener()); verifyZeroInteractions(onResume); verifyOnly(onCancel, onCancel()); container.listen(provider.future, (prev, value) {}); - verifyOnly(onAddListener, onAddListener(any)); + verifyOnly(onAddListener, onAddListener()); verifyOnly(onResume, onResume()); container.listen(provider.future, (prev, value) {}); - verifyOnly(onAddListener, onAddListener(any)); + verifyOnly(onAddListener, onAddListener()); verifyNoMoreInteractions(onCancel); verifyNoMoreInteractions(onResume); verifyZeroInteractions(onDispose); diff --git a/packages/riverpod/test/old/utils.dart b/packages/riverpod/test/old/utils.dart index 62db0785c..f7839184e 100644 --- a/packages/riverpod/test/old/utils.dart +++ b/packages/riverpod/test/old/utils.dart @@ -57,11 +57,11 @@ class OnResume extends Mock { } class OnAddListener extends Mock { - void call(ProviderSubscription? sub); + void call(); } class OnRemoveListener extends Mock { - void call(ProviderSubscription? sub); + void call(); } class Listener extends Mock { diff --git a/packages/riverpod/test/src/core/ref_test.dart b/packages/riverpod/test/src/core/ref_test.dart index 33341d2f4..e99419db8 100644 --- a/packages/riverpod/test/src/core/ref_test.dart +++ b/packages/riverpod/test/src/core/ref_test.dart @@ -80,7 +80,7 @@ void main() { throwsA(isA()), ); expect( - () => ref.onAddListener((_) {}), + () => ref.onAddListener(() {}), throwsA(isA()), ); expect( @@ -88,7 +88,7 @@ void main() { throwsA(isA()), ); expect( - () => ref.onRemoveListener((_) {}), + () => ref.onRemoveListener(() {}), throwsA(isA()), ); expect( @@ -163,7 +163,7 @@ void main() { throwsA(isA()), ); expect( - () => container.read(provider.select((_) => ref.onAddListener((_) {}))), + () => container.read(provider.select((_) => ref.onAddListener(() {}))), throwsA(isA()), ); expect( @@ -171,8 +171,8 @@ void main() { throwsA(isA()), ); expect( - () => container - .read(provider.select((_) => ref.onRemoveListener((_) {}))), + () => + container.read(provider.select((_) => ref.onRemoveListener(() {}))), throwsA(isA()), ); expect( @@ -2114,7 +2114,7 @@ void main() { container.read(provider); - verifyOnly(listener, listener(any)); + verifyOnly(listener, listener()); }); test('calls listeners when container.listen subscriptions are closed', @@ -2133,7 +2133,7 @@ void main() { sub.close(); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); @@ -2144,7 +2144,7 @@ void main() { sub2.close(); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2178,7 +2178,7 @@ void main() { sub.close(); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); @@ -2189,7 +2189,7 @@ void main() { sub2.close(); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2221,7 +2221,7 @@ void main() { container.refresh(provider); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2240,19 +2240,19 @@ void main() { }); container.read(provider); - verifyOnly(listener, listener(any)); + verifyOnly(listener, listener()); verifyZeroInteractions(listener2); isSecondBuild = true; container.refresh(provider); // Removed sub from refresh - verify(listener2(any)).called(1); + verify(listener2()).called(1); final sub = container.listen(provider, (previous, next) {}); sub.close(); - verify(listener2(any)).called(1); + verify(listener2()).called(1); verifyNoMoreInteractions(listener2); verifyNoMoreInteractions(listener); }); @@ -2262,7 +2262,7 @@ void main() { final container = ProviderContainer.test(); final listener = OnRemoveListener(); final listener2 = OnRemoveListener(); - when(listener(any)).thenThrow(42); + when(listener()).thenThrow(42); final provider = Provider((ref) { ref.onRemoveListener(listener.call); ref.onRemoveListener(listener2.call); @@ -2275,7 +2275,7 @@ void main() { (err, stack) => errors.add(err), ); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); expect(errors, [42]); @@ -2292,7 +2292,7 @@ void main() { container.read(provider); - verifyOnly(listener, listener(any)); + verifyOnly(listener, listener()); }); test('calls listeners when container.listen is invoked', () { @@ -2306,13 +2306,13 @@ void main() { container.listen(provider, (previous, next) {}); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); container.listen(provider, (previous, next) {}); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2339,13 +2339,13 @@ void main() { ref.listen(dep, (previous, next) {}); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); ref.listen(dep, (previous, next) {}); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2378,20 +2378,20 @@ void main() { ref.watch(dep); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); ref.watch(dep); // TODO changelog breaking: Calling ref.watch multiple times calls ref.onListen everytime - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); ref2.watch(dep); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); @@ -2410,17 +2410,17 @@ void main() { }); container.read(provider); - verifyOnly(listener, listener(any)); + verifyOnly(listener, listener()); isSecondBuild = true; container.refresh(provider); // Added refresh listener - verifyOnly(listener2, listener2(any)); + verifyOnly(listener2, listener2()); container.listen(provider, (previous, next) {}); - verify(listener2(any)).called(1); + verify(listener2()).called(1); verifyNoMoreInteractions(listener2); verifyNoMoreInteractions(listener); }); @@ -2430,7 +2430,7 @@ void main() { final container = ProviderContainer.test(); final listener = OnAddListener(); final listener2 = OnAddListener(); - when(listener(any)).thenThrow(42); + when(listener()).thenThrow(42); final provider = Provider((ref) { ref.onAddListener(listener.call); ref.onAddListener(listener2.call); @@ -2441,7 +2441,7 @@ void main() { (err, stack) => errors.add(err), ); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); expect(errors, [42]); @@ -2553,7 +2553,7 @@ void main() { ref.watch(dep); - verifyInOrder([listener(any), listener2(any)]); + verifyInOrder([listener(), listener2()]); verifyNoMoreInteractions(listener); verifyNoMoreInteractions(listener2); }); diff --git a/packages/riverpod/test/src/utils.dart b/packages/riverpod/test/src/utils.dart index 0d0292c8a..fc9554423 100644 --- a/packages/riverpod/test/src/utils.dart +++ b/packages/riverpod/test/src/utils.dart @@ -149,11 +149,11 @@ class OnPause extends Mock { } class OnAddListener extends Mock { - void call(ProviderSubscription? sub); + void call(); } class OnRemoveListener extends Mock { - void call(ProviderSubscription? sub); + void call(); } /// Syntax sugar for: From abb9a6122f89ce68981d4a89ac28ccb9e870c017 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 15 Sep 2024 01:27:50 +0200 Subject: [PATCH 20/27] Changelog --- packages/riverpod/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/riverpod/CHANGELOG.md b/packages/riverpod/CHANGELOG.md index 2f40e4c22..c9712f2b7 100644 --- a/packages/riverpod/CHANGELOG.md +++ b/packages/riverpod/CHANGELOG.md @@ -15,6 +15,8 @@ - `Stream/FutureProvider.overrideWithValue` was added back. - **Breaking**: `Notifier` and variants are now recreated whenever the provider rebuilds. This enables using `Ref.mounted` to check dispose. +- **Breaking**: `StreamProvider` now pauses its `StreamSubscription` when + the provider is not actively listened. - **Breaking**: A provider is now considered "paused" if all of its listeners are also paused. So if a provider `A` is watched _only_ by a provider `B`, and `B` is currently unused, then `A` will be paused. From fcee6f5e322e98a9651773076784606bc4c421c4 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 15 Sep 2024 01:43:12 +0200 Subject: [PATCH 21/27] Update codegen --- .../build_yaml/lib/dependencies.g.dart | 298 +++++++++--------- .../avoid_build_context_in_providers.g.dart | 1 - 2 files changed, 143 insertions(+), 156 deletions(-) diff --git a/packages/riverpod_generator/integration/build_yaml/lib/dependencies.g.dart b/packages/riverpod_generator/integration/build_yaml/lib/dependencies.g.dart index 587d88f1d..5be9cfabb 100644 --- a/packages/riverpod_generator/integration/build_yaml/lib/dependencies.g.dart +++ b/packages/riverpod_generator/integration/build_yaml/lib/dependencies.g.dart @@ -6,191 +6,179 @@ part of 'dependencies.dart'; // RiverpodGenerator // ************************************************************************** -String _$calc2Hash() => r'0972ac060d49cb3f08f9192af9cea0611a6b4616'; - -/// Copied from Dart SDK -class _SystemHash { - _SystemHash._(); - - static int combine(int hash, int value) { - // ignore: parameter_assignments - hash = 0x1fffffff & (hash + value); - // ignore: parameter_assignments - hash = 0x1fffffff & (hash + ((0x0007ffff & hash) << 10)); - return hash ^ (hash >> 6); - } - - static int finish(int hash) { - // ignore: parameter_assignments - hash = 0x1fffffff & (hash + ((0x03ffffff & hash) << 3)); - // ignore: parameter_assignments - hash = hash ^ (hash >> 11); - return 0x1fffffff & (hash + ((0x00003fff & hash) << 15)); - } -} +typedef Calc2Ref = Ref; -/// See also [calc2]. @ProviderFor(calc2) -const myFamilyCalc2ProviderFamily = Calc2Family(); - -/// See also [calc2]. -class Calc2Family extends Family { - /// See also [calc2]. - const Calc2Family(); +const myFamilyCalc2ProviderFamily = Calc2Family._(); + +final class Calc2Provider extends $FunctionalProvider + with $Provider { + const Calc2Provider._( + {required Calc2Family super.from, + required String super.argument, + int Function( + Calc2Ref ref, + String id, + )? create}) + : _createCb = create, + super( + retry: null, + name: r'myFamilyCalc2ProviderFamily', + isAutoDispose: true, + dependencies: null, + allTransitiveDependencies: null, + ); - /// See also [calc2]. - Calc2Provider call( + static const $allTransitiveDependencies0 = myCountPod; + static const $allTransitiveDependencies1 = myCountFuturePod; + static const $allTransitiveDependencies2 = myCountStreamPod; + static const $allTransitiveDependencies3 = myCountNotifierPod; + static const $allTransitiveDependencies4 = myCountAsyncNotifierPod; + static const $allTransitiveDependencies5 = myCountStreamNotifierPod; + static const $allTransitiveDependencies6 = myFamilyCount2ProviderFamily; + static const $allTransitiveDependencies7 = myFamilyCountFuture2ProviderFamily; + static const $allTransitiveDependencies8 = myFamilyCountStream2ProviderFamily; + static const $allTransitiveDependencies9 = + myFamilyCountNotifier2ProviderFamily; + static const $allTransitiveDependencies10 = + myFamilyCountAsyncNotifier2ProviderFamily; + static const $allTransitiveDependencies11 = + myFamilyCountStreamNotifier2ProviderFamily; + + final int Function( + Calc2Ref ref, String id, - ) { - return Calc2Provider( - id, - ); - } + )? _createCb; @override - Calc2Provider getProviderOverride( - covariant Calc2Provider provider, - ) { - return call( - provider.id, - ); - } - - static final Iterable _dependencies = { - myCountPod, - myCountFuturePod, - myCountStreamPod, - myCountNotifierPod, - myCountAsyncNotifierPod, - myCountStreamNotifierPod, - myFamilyCount2ProviderFamily, - myFamilyCountFuture2ProviderFamily, - myFamilyCountStream2ProviderFamily, - myFamilyCountNotifier2ProviderFamily, - myFamilyCountAsyncNotifier2ProviderFamily, - myFamilyCountStreamNotifier2ProviderFamily - }; + String debugGetCreateSourceHash() => _$calc2Hash(); @override - Iterable? get dependencies => _dependencies; - - static final Iterable _allTransitiveDependencies = - { - myCountPod, - ...?myCountPod.allTransitiveDependencies, - myCountFuturePod, - ...?myCountFuturePod.allTransitiveDependencies, - myCountStreamPod, - ...?myCountStreamPod.allTransitiveDependencies, - myCountNotifierPod, - ...?myCountNotifierPod.allTransitiveDependencies, - myCountAsyncNotifierPod, - ...?myCountAsyncNotifierPod.allTransitiveDependencies, - myCountStreamNotifierPod, - ...?myCountStreamNotifierPod.allTransitiveDependencies, - myFamilyCount2ProviderFamily, - ...?myFamilyCount2ProviderFamily.allTransitiveDependencies, - myFamilyCountFuture2ProviderFamily, - ...?myFamilyCountFuture2ProviderFamily.allTransitiveDependencies, - myFamilyCountStream2ProviderFamily, - ...?myFamilyCountStream2ProviderFamily.allTransitiveDependencies, - myFamilyCountNotifier2ProviderFamily, - ...?myFamilyCountNotifier2ProviderFamily.allTransitiveDependencies, - myFamilyCountAsyncNotifier2ProviderFamily, - ...?myFamilyCountAsyncNotifier2ProviderFamily.allTransitiveDependencies, - myFamilyCountStreamNotifier2ProviderFamily, - ...?myFamilyCountStreamNotifier2ProviderFamily.allTransitiveDependencies - }; + String toString() { + return r'myFamilyCalc2ProviderFamily' + '' + '($argument)'; + } - @override - Iterable? get allTransitiveDependencies => - _allTransitiveDependencies; + /// {@macro riverpod.override_with_value} + Override overrideWithValue(int value) { + return $ProviderOverride( + origin: this, + providerOverride: $ValueProvider(value), + ); + } + @$internal @override - String? get name => r'myFamilyCalc2ProviderFamily'; -} - -/// See also [calc2]. -class Calc2Provider extends AutoDisposeProvider { - /// See also [calc2]. - Calc2Provider( - String id, - ) : this._internal( - (ref) => calc2( - ref as Calc2Ref, - id, - ), - from: myFamilyCalc2ProviderFamily, - name: r'myFamilyCalc2ProviderFamily', - debugGetCreateSourceHash: - const bool.fromEnvironment('dart.vm.product') - ? null - : _$calc2Hash, - dependencies: Calc2Family._dependencies, - allTransitiveDependencies: Calc2Family._allTransitiveDependencies, - id: id, - ); - - Calc2Provider._internal( - super._createNotifier, { - required super.name, - required super.dependencies, - required super.allTransitiveDependencies, - required super.debugGetCreateSourceHash, - required super.from, - required this.id, - }) : super.internal(); - - final String id; + $ProviderElement $createElement($ProviderPointer pointer) => + $ProviderElement(this, pointer); @override - Override overrideWith( - int Function(Calc2Ref provider) create, + Calc2Provider $copyWithCreate( + int Function( + Calc2Ref ref, + ) create, ) { - return ProviderOverride( - origin: this, - override: Calc2Provider._internal( - (ref) => create(ref as Calc2Ref), - from: from, - name: null, - dependencies: null, - allTransitiveDependencies: null, - debugGetCreateSourceHash: null, - id: id, - ), - ); + return Calc2Provider._( + argument: argument as String, + from: from! as Calc2Family, + create: ( + ref, + String id, + ) => + create(ref)); } @override - AutoDisposeProviderElement createElement() { - return _Calc2ProviderElement(this); + int create(Calc2Ref ref) { + final _$cb = _createCb ?? calc2; + final argument = this.argument as String; + return _$cb( + ref, + argument, + ); } @override bool operator ==(Object other) { - return other is Calc2Provider && other.id == id; + return other is Calc2Provider && other.argument == argument; } @override int get hashCode { - var hash = _SystemHash.combine(0, runtimeType.hashCode); - hash = _SystemHash.combine(hash, id.hashCode); - - return _SystemHash.finish(hash); + return argument.hashCode; } } -mixin Calc2Ref on AutoDisposeProviderRef { - /// The parameter `id` of this provider. - String get id; -} +String _$calc2Hash() => r'0972ac060d49cb3f08f9192af9cea0611a6b4616'; -class _Calc2ProviderElement extends AutoDisposeProviderElement - with Calc2Ref { - _Calc2ProviderElement(super.provider); +final class Calc2Family extends Family { + const Calc2Family._() + : super( + retry: null, + name: r'myFamilyCalc2ProviderFamily', + dependencies: const [ + myCountPod, + myCountFuturePod, + myCountStreamPod, + myCountNotifierPod, + myCountAsyncNotifierPod, + myCountStreamNotifierPod, + myFamilyCount2ProviderFamily, + myFamilyCountFuture2ProviderFamily, + myFamilyCountStream2ProviderFamily, + myFamilyCountNotifier2ProviderFamily, + myFamilyCountAsyncNotifier2ProviderFamily, + myFamilyCountStreamNotifier2ProviderFamily + ], + allTransitiveDependencies: const { + Calc2Provider.$allTransitiveDependencies0, + Calc2Provider.$allTransitiveDependencies1, + Calc2Provider.$allTransitiveDependencies2, + Calc2Provider.$allTransitiveDependencies3, + Calc2Provider.$allTransitiveDependencies4, + Calc2Provider.$allTransitiveDependencies5, + Calc2Provider.$allTransitiveDependencies6, + Calc2Provider.$allTransitiveDependencies7, + Calc2Provider.$allTransitiveDependencies8, + Calc2Provider.$allTransitiveDependencies9, + Calc2Provider.$allTransitiveDependencies10, + Calc2Provider.$allTransitiveDependencies11, + }, + isAutoDispose: true, + ); + + Calc2Provider call( + String id, + ) => + Calc2Provider._(argument: id, from: this); @override - String get id => (origin as Calc2Provider).id; + String debugGetCreateSourceHash() => _$calc2Hash(); + + @override + String toString() => r'myFamilyCalc2ProviderFamily'; + + /// {@macro riverpod.override_with} + Override overrideWith( + int Function( + Calc2Ref ref, + String args, + ) create, + ) { + return $FamilyOverride( + from: this, + createElement: (pointer) { + final provider = pointer.origin as Calc2Provider; + + final argument = provider.argument as String; + + return provider + .$copyWithCreate((ref) => create(ref, argument)) + .$createElement(pointer); + }, + ); + } } // ignore_for_file: type=lint -// ignore_for_file: subtype_of_sealed_class, invalid_use_of_internal_member, invalid_use_of_visible_for_testing_member +// ignore_for_file: deprecated_member_use_from_same_package, unreachable_from_main, invalid_use_of_internal_member diff --git a/packages/riverpod_lint_flutter_test/test/lints/avoid_build_context_in_providers.g.dart b/packages/riverpod_lint_flutter_test/test/lints/avoid_build_context_in_providers.g.dart index 724b19e36..065501b23 100644 --- a/packages/riverpod_lint_flutter_test/test/lints/avoid_build_context_in_providers.g.dart +++ b/packages/riverpod_lint_flutter_test/test/lints/avoid_build_context_in_providers.g.dart @@ -344,7 +344,6 @@ abstract class _$MyNotifier extends $Notifier { BuildContext, { BuildContext context2, }); - // expect_lint: avoid_build_context_in_providers BuildContext get context1 => _$args.$1; // expect_lint: avoid_build_context_in_providers BuildContext get context2 => _$args.context2; From 980d0f1e0c21dbad41431a133fe838b1eea2dfa1 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Sun, 15 Sep 2024 01:47:16 +0200 Subject: [PATCH 22/27] Format --- .../test/consumer_listen_test.dart | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/packages/flutter_riverpod/test/consumer_listen_test.dart b/packages/flutter_riverpod/test/consumer_listen_test.dart index 63f6585fe..76bc0ccfd 100644 --- a/packages/flutter_riverpod/test/consumer_listen_test.dart +++ b/packages/flutter_riverpod/test/consumer_listen_test.dart @@ -162,7 +162,9 @@ void main() { await tester.pumpWidget(Container()); expect( - container.readProviderElement(provider).hasNonWeakListeners, false); + container.readProviderElement(provider).hasNonWeakListeners, + false, + ); }); testWidgets('closes the subscription on provider change', (tester) async { @@ -182,9 +184,13 @@ void main() { ); expect( - container.readProviderElement(provider(0)).hasNonWeakListeners, true); - expect(container.readProviderElement(provider(1)).hasNonWeakListeners, - false); + container.readProviderElement(provider(0)).hasNonWeakListeners, + true, + ); + expect( + container.readProviderElement(provider(1)).hasNonWeakListeners, + false, + ); await tester.pumpWidget( UncontrolledProviderScope( @@ -198,10 +204,14 @@ void main() { ), ); - expect(container.readProviderElement(provider(0)).hasNonWeakListeners, - false); expect( - container.readProviderElement(provider(1)).hasNonWeakListeners, true); + container.readProviderElement(provider(0)).hasNonWeakListeners, + false, + ); + expect( + container.readProviderElement(provider(1)).hasNonWeakListeners, + true, + ); }); testWidgets('listen to the new provider on provider change', From 5fa05566ac0130df7da2d006003eaad2d74f460d Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Wed, 18 Sep 2024 00:45:55 +0200 Subject: [PATCH 23/27] Listen Visibility to pause subscriptions --- packages/flutter_riverpod/CHANGELOG.md | 1 + .../lib/src/core/consumer.dart | 46 ++++- .../test/src/core/consumer_test.dart | 183 +++++++++++++++++- .../lib/src/core/provider_subscription.dart | 2 +- .../src/core/provider_subscription_test.dart | 18 ++ 5 files changed, 244 insertions(+), 6 deletions(-) diff --git a/packages/flutter_riverpod/CHANGELOG.md b/packages/flutter_riverpod/CHANGELOG.md index 75dd7aebb..093798bd1 100644 --- a/packages/flutter_riverpod/CHANGELOG.md +++ b/packages/flutter_riverpod/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased build +- **Breaking**: When a ConsumerWidgets stops being visible (based off `Visibility`), it now temporarily stops listening to providers. - **Breaking**: Removed all `Ref` subclasses (such `FutureProviderRef`). Use `Ref` directly instead. For `FutureProviderRef.future`, migrate to using an `AsyncNotifier`. diff --git a/packages/flutter_riverpod/lib/src/core/consumer.dart b/packages/flutter_riverpod/lib/src/core/consumer.dart index 79492aaae..d029ac4f1 100644 --- a/packages/flutter_riverpod/lib/src/core/consumer.dart +++ b/packages/flutter_riverpod/lib/src/core/consumer.dart @@ -69,6 +69,15 @@ typedef ConsumerBuilder = Widget Function( /// } /// ``` /// +/// ## Performance considerations +/// +/// To optimize performance by avoiding unnecessary network requests and +/// pausing unused streams, [Consumer] will temporarily stop listening to +/// providers when the widget stops being visible. +/// +/// This is determined using [Visibility.of], and will invoke +/// [ProviderSubscription.pause] on all currently active subscriptions. +/// /// See also: /// /// * [ConsumerWidget], a base-class for widgets that wants to listen to providers. @@ -349,6 +358,9 @@ class ConsumerStatefulElement extends StatefulElement implements WidgetRef { /// The [Element] for a [ConsumerStatefulWidget] ConsumerStatefulElement(ConsumerStatefulWidget super.widget); + @override + BuildContext get context => this; + late ProviderContainer _container = ProviderScope.containerOf(this); var _dependencies = , ProviderSubscription>{}; @@ -356,6 +368,19 @@ class ConsumerStatefulElement extends StatefulElement implements WidgetRef { _oldDependencies; final _listeners = >[]; List<_ListenManual>? _manualListeners; + bool? _visible; + + Iterable get _allSubscriptions sync* { + yield* _dependencies.values; + yield* _listeners; + if (_manualListeners != null) { + yield* _manualListeners!; + } + } + + void _applyVisibility(ProviderSubscription sub) { + if (_visible == false) sub.pause(); + } @override void didChangeDependencies() { @@ -372,6 +397,18 @@ class ConsumerStatefulElement extends StatefulElement implements WidgetRef { @override Widget build() { + final visible = Visibility.of(context); + if (visible != _visible) { + _visible = visible; + for (final sub in _allSubscriptions) { + if (visible) { + sub.resume(); + } else { + sub.pause(); + } + } + } + try { _oldDependencies = _dependencies; for (var i = 0; i < _listeners.length; i++) { @@ -404,10 +441,12 @@ class ConsumerStatefulElement extends StatefulElement implements WidgetRef { return oldDependency; } - return _container.listen( + final sub = _container.listen( target, (_, __) => markNeedsBuild(), ); + _applyVisibility(sub); + return sub; }).read() as Res; } @@ -448,6 +487,7 @@ class ConsumerStatefulElement extends StatefulElement implements WidgetRef { // which listen call was preserved between widget rebuild, and we wouldn't // want to call the listener on every rebuild. final sub = _container.listen(provider, listener, onError: onError); + _applyVisibility(sub); _listeners.add(sub); } @@ -501,13 +541,11 @@ class ConsumerStatefulElement extends StatefulElement implements WidgetRef { ) as ProviderSubscriptionWithOrigin, this, ); + _applyVisibility(sub); listeners.add(sub); return sub; } - - @override - BuildContext get context => this; } final class _ListenManual diff --git a/packages/flutter_riverpod/test/src/core/consumer_test.dart b/packages/flutter_riverpod/test/src/core/consumer_test.dart index 84b4c97f1..5ffbd24ff 100644 --- a/packages/flutter_riverpod/test/src/core/consumer_test.dart +++ b/packages/flutter_riverpod/test/src/core/consumer_test.dart @@ -1,3 +1,5 @@ +// ignore_for_file: invalid_use_of_internal_member + import 'dart:async'; import 'package:flutter/material.dart'; @@ -5,6 +7,29 @@ import 'package:flutter_riverpod/src/internals.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:riverpod/legacy.dart'; +class SimpleVisibility extends StatelessWidget { + const SimpleVisibility({ + super.key, + required this.visible, + required this.child, + }); + + final bool visible; + final Widget child; + + @override + Widget build(BuildContext context) { + return Visibility( + visible: visible, + maintainState: true, + maintainAnimation: true, + maintainSize: true, + maintainInteractivity: true, + child: child, + ); + } +} + void main() { group('_ListenManual', () { testWidgets('handles pause/resume', (tester) async { @@ -28,7 +53,6 @@ void main() { ); final container = ProviderScope.containerOf(ref.context); - // ignore: invalid_use_of_internal_member final element = container.readProviderElement(provider); expect(element.isActive, true); @@ -43,6 +67,163 @@ void main() { }); }); + group('Handles Visibility', () { + testWidgets( + 'when adding a listener, initializes pause state based on visibility', + (tester) async { + final providerForVisible = Provider((ref) => 0); + + await tester.pumpWidget( + ProviderScope( + child: Column( + children: [ + Consumer( + builder: (c, ref, _) { + ref.listen(providerForVisible, (_, __) {}); + ref.watch(providerForVisible); + ref.listenManual(providerForVisible, (_, __) {}); + + return const SizedBox(); + }, + ), + SimpleVisibility( + visible: false, + child: Consumer( + builder: (c, ref, _) { + ref.listen(_provider, (_, __) {}); + ref.watch(_provider); + ref.listenManual(_provider, (_, __) {}); + + return const SizedBox(); + }, + ), + ), + ], + ), + ), + ); + + final container = ProviderScope.containerOf( + tester.element(find.byType(Column)), + ); + final visibleElement = container.readProviderElement(providerForVisible); + final hiddenElement = container.readProviderElement(_provider); + + expect( + hiddenElement.dependents, + everyElement( + isA() + .having((e) => e.isPaused, 'isPaused', true), + ), + ); + expect( + visibleElement.dependents, + everyElement( + isA() + .having((e) => e.isPaused, 'isPaused', false), + ), + ); + }); + + testWidgets( + 'listenManual inside life-cycles before didChangeDependencies ' + 'on non-visible widgets does not paused twice', (tester) async { + late ProviderSubscription sub; + final widget = CallbackConsumerWidget( + initState: (context, ref) { + sub = ref.listenManual(_provider, (_, __) {}); + }, + ); + + await tester.pumpWidget( + ProviderScope( + child: SimpleVisibility(visible: false, child: widget), + ), + ); + + sub.resume(); + + expect(sub.isPaused, false); + }); + + testWidgets('when visibility changes, pause/resume listeners', + (tester) async { + late WidgetRef ref; + final widget = Consumer( + builder: (c, r, _) { + ref = r; + + return const SizedBox(); + }, + ); + + await tester.pumpWidget( + ProviderScope( + child: SimpleVisibility(visible: true, child: widget), + ), + ); + + final sub = ref.listenManual(_provider, (_, __) {}); + + await tester.pumpWidget( + ProviderScope( + child: SimpleVisibility(visible: false, child: widget), + ), + ); + + expect(sub.isPaused, true); + + await tester.pumpWidget( + ProviderScope( + child: SimpleVisibility(visible: true, child: widget), + ), + ); + + expect(sub.isPaused, false); + }); + + testWidgets( + 'when a dependency changes but visibility does not, do not pause/resume listeners', + (tester) async { + late WidgetRef ref; + final widget = Consumer( + builder: (context, r, _) { + // Subscribe to Theme + Theme.of(context); + ref = r; + + return const SizedBox(); + }, + ); + + await tester.pumpWidget( + ProviderScope( + child: Theme( + data: ThemeData.dark(), + child: SimpleVisibility(visible: false, child: widget), + ), + ), + ); + + final sub = ref.listenManual(_provider, (_, __) {}); + + await tester.pumpWidget( + ProviderScope( + child: Theme( + data: ThemeData.light(), + child: SimpleVisibility(visible: false, child: widget), + ), + ), + ); + + expect(sub.isPaused, true); + + sub.resume(); + + expect(sub.isPaused, false); + }); + }); + testWidgets('Riverpod test', (tester) async { // Regression test for https://github.com/rrousselGit/riverpod/pull/3156 diff --git a/packages/riverpod/lib/src/core/provider_subscription.dart b/packages/riverpod/lib/src/core/provider_subscription.dart index 8ee6ff93f..c1440738c 100644 --- a/packages/riverpod/lib/src/core/provider_subscription.dart +++ b/packages/riverpod/lib/src/core/provider_subscription.dart @@ -84,7 +84,7 @@ mixin _OnPauseMixin { if (_pauseCount == 1) { onResume(); } - _pauseCount = math.min(_pauseCount - 1, 0); + _pauseCount = math.max(_pauseCount - 1, 0); } void onResume(); diff --git a/packages/riverpod/test/src/core/provider_subscription_test.dart b/packages/riverpod/test/src/core/provider_subscription_test.dart index e3a882b8d..7fc3ad108 100644 --- a/packages/riverpod/test/src/core/provider_subscription_test.dart +++ b/packages/riverpod/test/src/core/provider_subscription_test.dart @@ -130,5 +130,23 @@ void main() { verifyNoMoreInteractions(onError); verifyZeroInteractions(listener); }); + + test('needs to be called as many times as pause() was called', () { + final container = ProviderContainer.test(); + final provider = Provider((ref) => 0); + + final sub = container.listen(provider, (p, b) {}); + + sub.pause(); + sub.pause(); + sub.pause(); + + sub.resume(); + expect(sub.isPaused, true); + sub.resume(); + expect(sub.isPaused, true); + sub.resume(); + expect(sub.isPaused, false); + }); }); } From 027c75693585d457ecb402b61a6255593de9c189 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Wed, 18 Sep 2024 01:48:02 +0200 Subject: [PATCH 24/27] Fix AST caching --- .../lib/src/analyzer_utils.dart | 50 +++++++------------ .../lib/src/nodes.g.dart | 7 ++- .../lib/src/nodes/dependencies.dart | 32 +++++++++--- .../lib/src/nodes/provider_listenable.dart | 4 +- .../lib/src/nodes/providers/function.dart | 4 +- .../lib/src/nodes/providers/identifiers.dart | 4 +- .../lib/src/nodes/providers/legacy.dart | 4 +- .../lib/src/nodes/providers/notifier.dart | 4 +- .../lib/src/nodes/ref_invocation.dart | 4 +- .../lib/src/nodes/riverpod.dart | 8 ++- .../lib/src/nodes/scopes/overrides.dart | 8 ++- .../src/nodes/scopes/provider_container.dart | 5 +- .../lib/src/nodes/scopes/provider_scope.dart | 5 +- .../lib/src/nodes/widget_ref_invocation.dart | 4 +- .../lib/src/nodes/widgets/widget.dart | 8 ++- .../lib/src/lints/unknown_scoped_usage.dart | 5 +- 16 files changed, 96 insertions(+), 60 deletions(-) diff --git a/packages/riverpod_analyzer_utils/lib/src/analyzer_utils.dart b/packages/riverpod_analyzer_utils/lib/src/analyzer_utils.dart index d7c59c038..1d9bd8d1f 100644 --- a/packages/riverpod_analyzer_utils/lib/src/analyzer_utils.dart +++ b/packages/riverpod_analyzer_utils/lib/src/analyzer_utils.dart @@ -1,49 +1,35 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:meta/meta.dart'; -@immutable -class _Key { - const _Key(this.value, this.name); - - final Object value; - final String name; - - @override - bool operator ==(Object other) { - return other is _Key && other.value == value && other.name == name; - } - - @override - int get hashCode => Object.hash(value, name); -} - -class _Box { - _Box(this.value); +@internal +class Box { + Box(this.value); final T value; } @internal extension AstUtils on AstNode { - static final _cache = Expando<_Box>(); - R upsert( - String keyPart, + Iterable get ancestors sync* { + var parent = this.parent; + while (parent != null) { + yield parent; + parent = parent.parent; + } + } +} + +@internal +extension ExpandoUtils on Expando> { + R upsert( + AstNode key, R Function() create, ) { - final key = _Key(this, keyPart); // Using a record to differentiate "null value" from "no value". - final existing = _cache[key] as _Box?; + final existing = this[key]; if (existing != null) return existing.value; final created = create(); - _cache[key] = _Box(created); + this[key] = Box(created); return created; } - - Iterable get ancestors sync* { - var parent = this.parent; - while (parent != null) { - yield parent; - parent = parent.parent; - } - } } diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes.g.dart b/packages/riverpod_analyzer_utils/lib/src/nodes.g.dart index 68564be3b..31e7aa8e2 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes.g.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes.g.dart @@ -726,13 +726,12 @@ class RiverpodAnalysisResult extends RecursiveRiverpodAstVisitor { } class RiverpodAstRegistry { + static final cache = Expando>>(); + void run(AstNode node) { final previousErrorReporter = errorReporter; try { - final errors = node.upsert( - 'RiverpodAstRegistry.errors', - () => [], - ); + final errors = cache.upsert(node, () => []); final visitor = _RiverpodAstRegistryVisitor(this); errorReporter = errors.add; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/dependencies.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/dependencies.dart index 7823c1d3a..73e42b417 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/dependencies.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/dependencies.dart @@ -1,8 +1,10 @@ part of '../nodes.dart'; extension on CollectionElement { + static final _cache = Expando>(); + ProviderDependency? get providerDependency { - return upsert('ProviderDependency', () { + return _cache.upsert(this, () { final that = this; if (that is! Expression) { errorReporter( @@ -88,8 +90,10 @@ final class ProviderDependency { } extension on Expression { + static final _cache = Expando>(); + ProviderDependencyList? get providerDependencyList { - return upsert('ProviderDependencyList', () { + return _cache.upsert(this, () { final that = this; // explicit null, count as valid value (no dependencies) if (that is NullLiteral) { @@ -296,8 +300,10 @@ extension AccumulatedDependenciesX on AstNode { } extension on InstanceCreationExpression { + static final _cache = Expando>(); + AccumulatedDependencyList? get accumulatedDependencies { - return upsert('InstanceCreationExpression#accumulatedDependencies', () { + return _cache.upsert(this, () { final providerScope = this.providerScope; if (providerScope == null) return null; @@ -313,8 +319,10 @@ extension on InstanceCreationExpression { } extension on AnnotatedNode { + static final _cache = Expando>(); + AccumulatedDependencyList? get accumulatedDependencies { - return upsert('#AnnotatedNodeAccumulatedDependencies', () { + return _cache.upsert(this, () { final provider = cast()?.provider; // Have State inherit dependencies from its widget final state = cast()?.state; @@ -347,8 +355,10 @@ class IdentifierDependencies { @_ast extension IdentifierDependenciesX on Identifier { + static final _cache = Expando>(); + IdentifierDependencies? get identifierDependencies { - return upsert('Identifier#identifierDependencies', () { + return _cache.upsert(this, () { final staticElement = this.staticElement; if (staticElement == null) return null; @@ -372,8 +382,10 @@ class NamedTypeDependencies { @_ast extension NamedTypeDependenciesX on NamedType { + static final _cache = Expando>(); + NamedTypeDependencies? get typeAnnotationDependencies { - return upsert('NamedType#typeAnnotationDependencies', () { + return _cache.upsert(this, () { final staticElement = type?.element; if (staticElement == null) return null; @@ -389,8 +401,10 @@ extension NamedTypeDependenciesX on NamedType { } extension DependenciesAnnotatedAnnotatedNodeOfX on AnnotatedNode { + static final _cache = Expando>(); + DependenciesAnnotation? get dependencies { - return upsert('DependenciesAnnotationAnnotatedNodeX', () { + return _cache.upsert(this, () { return metadata.map((e) => e.dependencies).nonNulls.firstOrNull; }); } @@ -398,8 +412,10 @@ extension DependenciesAnnotatedAnnotatedNodeOfX on AnnotatedNode { @_ast extension DependenciesAnnotatedAnnotatedNodeX on Annotation { + static final _cache = Expando>(); + DependenciesAnnotation? get dependencies { - return upsert('DependenciesAnnotation', () { + return _cache.upsert(this, () { final elementAnnotation = this.elementAnnotation; final element = this.element; if (element == null || elementAnnotation == null) return null; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/provider_listenable.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/provider_listenable.dart index f33d8349b..2c76d990e 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/provider_listenable.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/provider_listenable.dart @@ -2,8 +2,10 @@ part of '../nodes.dart'; @_ast extension ProviderListenableExpressionX on Expression { + static final _cache = Expando>(); + ProviderListenableExpression? get providerListenable { - return upsert('ProviderListenableExpression', () { + return _cache.upsert(this, () { final returnType = staticType; if (returnType == null) return null; if (!providerListenableType.isAssignableFromType(returnType)) return null; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/providers/function.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/providers/function.dart index 6a8ad92f7..0c903c5f5 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/providers/function.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/providers/function.dart @@ -2,8 +2,10 @@ part of '../../nodes.dart'; @_ast extension FunctionalProviderDeclarationX on FunctionDeclaration { + static final _cache = Expando>(); + FunctionalProviderDeclaration? get provider { - return upsert('FunctionalProviderDeclaration', () { + return _cache.upsert(this, () { final element = declaredElement; if (element == null) return null; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/providers/identifiers.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/providers/identifiers.dart index 8e7acd125..c9ce8aa84 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/providers/identifiers.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/providers/identifiers.dart @@ -2,8 +2,10 @@ part of '../../nodes.dart'; @_ast extension ProviderIdentifierX on SimpleIdentifier { + static final _cache = Expando>(); + ProviderIdentifier? get provider { - return upsert('ProviderIdentifier', () { + return _cache.upsert(this, () { final element = staticElement; if (element is! PropertyAccessorElement) return null; final variable = element.variable2; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/providers/legacy.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/providers/legacy.dart index 4e6a76c9e..489e951ca 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/providers/legacy.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/providers/legacy.dart @@ -49,8 +49,10 @@ final class LegacyProviderDependency { @_ast extension LegacyProviderDeclarationX on VariableDeclaration { + static final _cache = Expando>(); + LegacyProviderDeclaration? get provider { - return upsert('LegacyProviderDeclaration', () { + return _cache.upsert(this, () { final element = declaredElement; if (element == null) return null; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/providers/notifier.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/providers/notifier.dart index 6217f2df6..639a4b198 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/providers/notifier.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/providers/notifier.dart @@ -2,8 +2,10 @@ part of '../../nodes.dart'; @_ast extension ClassBasedProviderDeclarationX on ClassDeclaration { + static final _cache = Expando>(); + ClassBasedProviderDeclaration? get provider { - return upsert('ClassBasedProviderDeclaration', () { + return _cache.upsert(this, () { final element = declaredElement; if (element == null) return null; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/ref_invocation.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/ref_invocation.dart index a18bdbce3..e69a3f494 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/ref_invocation.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/ref_invocation.dart @@ -2,8 +2,10 @@ part of '../nodes.dart'; @_ast extension RefInvocationX on MethodInvocation { + static final _cache = Expando>(); + RefInvocation? get refInvocation { - return upsert('RefInvocation', () { + return _cache.upsert(this, () { final targetType = realTarget?.staticType; if (targetType == null) return null; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/riverpod.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/riverpod.dart index 4f3094f2f..2e71ac507 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/riverpod.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/riverpod.dart @@ -1,8 +1,10 @@ part of '../nodes.dart'; extension RiverpodAnnotatedAnnotatedNodeOfX on AnnotatedNode { + static final _cache = Expando>(); + RiverpodAnnotation? get riverpod { - return upsert('RiverpodAnnotationAnnotatedNodeX', () { + return _cache.upsert(this, () { return metadata.map((e) => e.riverpod).nonNulls.firstOrNull; }); } @@ -10,8 +12,10 @@ extension RiverpodAnnotatedAnnotatedNodeOfX on AnnotatedNode { @_ast extension RiverpodAnnotatedAnnotatedNodeX on Annotation { + static final _cache = Expando>(); + RiverpodAnnotation? get riverpod { - return upsert('RiverpodAnnotation', () { + return _cache.upsert(this, () { final elementAnnotation = this.elementAnnotation; final element = this.element; if (element == null || elementAnnotation == null) return null; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/overrides.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/overrides.dart index 9c729153f..7caad7890 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/overrides.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/overrides.dart @@ -2,8 +2,10 @@ part of '../../nodes.dart'; @_ast extension ProviderOverrideExpressionX on CollectionElement { + static final _cache = Expando>(); + ProviderOverrideExpression? get providerOverride { - return upsert('ProviderOverrideExpression', () { + return _cache.upsert(this, () { final expression = this; if (expression is! Expression) return null; @@ -41,8 +43,10 @@ final class ProviderOverrideExpression { @_ast extension ProviderOverrideListX on Expression { + static final _cache = Expando>(); + ProviderOverrideList? get overrides { - return upsert('ProviderOverrideList', () { + return _cache.upsert(this, () { final expression = this; final type = staticType; if (type == null || !type.isDartCoreList) return null; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/provider_container.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/provider_container.dart index 5d41dcb39..9549ae326 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/provider_container.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/provider_container.dart @@ -3,8 +3,11 @@ part of '../../nodes.dart'; @_ast extension ProviderContainerInstanceCreationExpressionX on InstanceCreationExpression { + static final _cache = + Expando>(); + ProviderContainerInstanceCreationExpression? get providerContainer { - return upsert('ProviderContainerInstanceCreationExpression', () { + return _cache.upsert(this, () { final createdType = constructorName.type.type; if (createdType == null || !providerContainerType.isExactlyType(createdType)) { diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/provider_scope.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/provider_scope.dart index 69d1c5a74..50c7efb2a 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/provider_scope.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/scopes/provider_scope.dart @@ -3,8 +3,11 @@ part of '../../nodes.dart'; @_ast extension ProviderScopeInstanceCreationExpressionX on InstanceCreationExpression { + static final _cache = + Expando>(); + ProviderScopeInstanceCreationExpression? get providerScope { - return upsert('ProviderScopeInstanceCreationExpression', () { + return _cache.upsert(this, () { final createdType = constructorName.type.type; if (createdType == null || !providerScopeType.isExactlyType(createdType)) { diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/widget_ref_invocation.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/widget_ref_invocation.dart index 827ef0688..5bdf93f49 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/widget_ref_invocation.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/widget_ref_invocation.dart @@ -2,8 +2,10 @@ part of '../nodes.dart'; @_ast extension WidgetRefInvocationX on MethodInvocation { + static final _cache = Expando>(); + WidgetRefInvocation? get widgetRefInvocation { - return upsert('WidgetRefInvocation', () { + return _cache.upsert(this, () { final targetType = realTarget?.staticType; if (targetType == null) return null; diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes/widgets/widget.dart b/packages/riverpod_analyzer_utils/lib/src/nodes/widgets/widget.dart index 1c4ba9ced..0e505a0f8 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes/widgets/widget.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes/widgets/widget.dart @@ -2,8 +2,10 @@ part of '../../nodes.dart'; @_ast extension WidgetX on ClassDeclaration { + static final _cache1 = Expando>(); + WidgetDeclaration? get widget { - return upsert('Widget', () { + return _cache1.upsert(this, () { final type = extendsClause?.superclass.type; if (type == null) return null; @@ -19,8 +21,10 @@ extension WidgetX on ClassDeclaration { }); } + static final _cache2 = Expando>(); + StateDeclaration? get state { - return upsert('State', () { + return _cache2.upsert(this, () { final type = extendsClause?.superclass.type; if (type == null) return null; diff --git a/packages/riverpod_lint/lib/src/lints/unknown_scoped_usage.dart b/packages/riverpod_lint/lib/src/lints/unknown_scoped_usage.dart index 048eeed3b..fb312d515 100644 --- a/packages/riverpod_lint/lib/src/lints/unknown_scoped_usage.dart +++ b/packages/riverpod_lint/lib/src/lints/unknown_scoped_usage.dart @@ -62,7 +62,10 @@ class UnknownScopedUsage extends RiverpodLintRule { if (enclosingConstructorType != null && widgetType.isAssignableFromType(enclosingConstructorType)) return; - reporter.atNode(identifier.node, code); + reporter.atNode( + identifier.node, + code, + ); }); } } From 088cfcc6a6c144b9db21ecc64d3c9d42b7efda7c Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Wed, 18 Sep 2024 01:51:57 +0200 Subject: [PATCH 25/27] Remove unused default --- packages/riverpod/lib/src/core/element.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/riverpod/lib/src/core/element.dart b/packages/riverpod/lib/src/core/element.dart index 547506a12..53e0b9b97 100644 --- a/packages/riverpod/lib/src/core/element.dart +++ b/packages/riverpod/lib/src/core/element.dart @@ -483,7 +483,6 @@ The provider ${_debugCurrentlyBuildingElement!.origin} modified $origin while bu newState.stackTrace, ); } - default: } for (final observer in container.observers) { From f6e5e015facd1ba6e745f0a518dc155f066c0560 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Wed, 18 Sep 2024 02:01:01 +0200 Subject: [PATCH 26/27] Fix test --- .../test/nodes/riverpod_test.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/riverpod_analyzer_utils_tests/test/nodes/riverpod_test.dart b/packages/riverpod_analyzer_utils_tests/test/nodes/riverpod_test.dart index c40685067..62c1f2a66 100644 --- a/packages/riverpod_analyzer_utils_tests/test/nodes/riverpod_test.dart +++ b/packages/riverpod_analyzer_utils_tests/test/nodes/riverpod_test.dart @@ -73,7 +73,7 @@ int eight(EightRef ref) => 0; ); expect( errors[4].targetElement.toString(), - 'Riverpod Riverpod({bool keepAlive = false, List? dependencies})', + 'Riverpod Riverpod({bool keepAlive = false, List? dependencies, Duration? Function(int, Object)? retry})', ); expect( @@ -88,7 +88,7 @@ int eight(EightRef ref) => 0; ); expect( errors[6].targetElement.toString(), - 'Riverpod Riverpod({bool keepAlive = false, List? dependencies})', + 'Riverpod Riverpod({bool keepAlive = false, List? dependencies, Duration? Function(int, Object)? retry})', ); expect( @@ -103,7 +103,7 @@ int eight(EightRef ref) => 0; ); expect( errors[8].targetElement.toString(), - 'Riverpod Riverpod({bool keepAlive = false, List? dependencies})', + 'Riverpod Riverpod({bool keepAlive = false, List? dependencies, Duration? Function(int, Object)? retry})', ); expect( From 4e4c4ba26ec70bf2a615610d30e606a295550c04 Mon Sep 17 00:00:00 2001 From: Remi Rousselet Date: Wed, 18 Sep 2024 02:08:39 +0200 Subject: [PATCH 27/27] Fix codegen --- .../lint_visitor_generator/lib/builder.dart | 22 +++++++++++-------- .../lib/src/nodes.g.dart | 7 ++++-- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/packages/lint_visitor_generator/lib/builder.dart b/packages/lint_visitor_generator/lib/builder.dart index 12a98c422..d72e149bb 100644 --- a/packages/lint_visitor_generator/lib/builder.dart +++ b/packages/lint_visitor_generator/lib/builder.dart @@ -43,13 +43,15 @@ class _LintVisitorGenerator extends Generator { .expand((extension) { final constraint = extension.extendedType; - return extension.accessors.map( - (e) => ( - constraint: constraint.element!.name!, - type: e.returnType.element!.name!, - name: e.name, - ), - ); + return extension.accessors + .map( + (e) => ( + constraint: constraint.element!.name!, + type: e.returnType.element!.name!, + name: e.name, + ), + ) + .where((e) => !e.name.startsWith('_cache')); }).toList(); final byConstraint = <({ @@ -146,11 +148,13 @@ class RiverpodAnalysisResult extends RecursiveRiverpodAstVisitor { } class RiverpodAstRegistry { + static final _cache = Expando>>(); + void run(AstNode node) { final previousErrorReporter = errorReporter; try { - final errors = node.upsert( - 'RiverpodAstRegistry.errors', + final errors = _cache.upsert( + node, () => [], ); diff --git a/packages/riverpod_analyzer_utils/lib/src/nodes.g.dart b/packages/riverpod_analyzer_utils/lib/src/nodes.g.dart index 31e7aa8e2..e0694f4e5 100644 --- a/packages/riverpod_analyzer_utils/lib/src/nodes.g.dart +++ b/packages/riverpod_analyzer_utils/lib/src/nodes.g.dart @@ -726,12 +726,15 @@ class RiverpodAnalysisResult extends RecursiveRiverpodAstVisitor { } class RiverpodAstRegistry { - static final cache = Expando>>(); + static final _cache = Expando>>(); void run(AstNode node) { final previousErrorReporter = errorReporter; try { - final errors = cache.upsert(node, () => []); + final errors = _cache.upsert( + node, + () => [], + ); final visitor = _RiverpodAstRegistryVisitor(this); errorReporter = errors.add;