Add dispose to FutureProvider #912
Conversation
WalkthroughThe Changes
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/provider/lib/src/async_provider.dart (1)
184-194: Add doc comment for the newdisposeparameter
disposeis now part of the public API, but the constructor header comment above (lines 181-184) hasn’t been updated to explain its purpose or usage contract (e.g. when it is called, whether it can be async, etc.). This can confuse consumers scanning the source or relying on generated docs./// Creates a [Future] from `create` and subscribes to it. /// -/// `create` must not be `null`. +/// `create` must not be `null`. +/// +/// The optional [dispose] callback is invoked with the last exposed value +/// when the provider is removed from the widget tree, allowing cleanup of +/// resources (closing streams, cancelling timers, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/provider/lib/src/async_provider.dart(2 hunks)
🔇 Additional comments (1)
packages/provider/lib/src/async_provider.dart (1)
195-200: Forwardingdisposeto super looks correctPassing the parameter straight through to
DeferredInheritedProviderensures the disposal logic is wired without altering existing behaviour—good change.
RossComputerGuy
force-pushed
the
feat/future-dispose
branch
from
6e7f04b to
ea68b95
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/provider/lib/src/deferred_inherited_provider.dart (1)
160-169: 🛠️ Refactor suggestionDelegate stores the new
disposetype – same compatibility concern
final Dispose<R>? dispose;propagates the issue mentioned above to the delegate.
If you keep the current design, update the documentation of all providers (and their tests) to reflect thatdisposenow receives the value instead of the controller.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/provider/lib/src/async_provider.dart(2 hunks)packages/provider/lib/src/deferred_inherited_provider.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/provider/lib/src/async_provider.dart
🔇 Additional comments (1)
packages/provider/lib/src/deferred_inherited_provider.dart (1)
35-44: SwitchingdisposefromDispose<T>→Dispose<R>is a breaking change – double-check all call-sites
DeferredInheritedProvideris reused byStreamProvider,ChangeNotifierProxyProvider, … etc.
Those providers currently pass a callback that expects the controller (T) rather than the exposed value (R).
Changing the type here silently breaks those call-sites and will surface as anInvalid castor a compile-time error once NNBD-strict is enabled.Please audit every constructor that forwards
dispose:to this class and make sure their generic argument now matches the new contract, or consider keeping the original signature and introducing a separate parameter (e.g.disposeValue) for the future-specific path.
| super.dispose(); | ||
| if (_didBuild) { | ||
| delegate.dispose?.call(element!, _controller as T); | ||
| if (T is Future<R>) { | ||
| (_controller as Future<R>).then((value) => delegate.dispose?.call(element!, value)); | ||
| } else if (isLoaded) { | ||
| delegate.dispose?.call(element!, value); | ||
| } else { | ||
| delegate.dispose?.call(element!, _controller as R); | ||
| } |
There was a problem hiding this comment.
Compile-time error & wrong cast inside _CreateDeferredInheritedProviderElement.dispose
if (T is Future<R>)is invalid Dart – the left-hand side ofismust be an expression, not a type parameter.delegate.dispose?.call(element!, _controller as R);will throw at runtime whenT≠R(e.g.StreamProvider<Stream<int>, int>), because_controlleris aStream<int>whileRisint.
Suggested fix:
- if (T is Future<R>) {
- (_controller as Future<R>).then((value) => delegate.dispose?.call(element!, value));
+ if (_controller is Future<R>) {
+ // FutureProvider – dispose after the Future completes.
+ (_controller as Future<R>).then(
+ (resolved) => delegate.dispose?.call(element!, resolved),
+ onError: (_) {}, // ignore errors, we cannot dispose a failed value
+ );
} else if (isLoaded) {
delegate.dispose?.call(element!, value);
} else {
- delegate.dispose?.call(element!, _controller as R);
+ // No value produced; fall back to disposing the controller itself.
+ // Cast to `dynamic` to avoid a bad runtime cast when T ≠ R.
+ delegate.dispose?.call(element!, _controller as dynamic);
}This eliminates the compile error and prevents a potential TypeError at runtime.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| super.dispose(); | |
| if (_didBuild) { | |
| delegate.dispose?.call(element!, _controller as T); | |
| if (T is Future<R>) { | |
| (_controller as Future<R>).then((value) => delegate.dispose?.call(element!, value)); | |
| } else if (isLoaded) { | |
| delegate.dispose?.call(element!, value); | |
| } else { | |
| delegate.dispose?.call(element!, _controller as R); | |
| } | |
| super.dispose(); | |
| if (_didBuild) { | |
| if (_controller is Future<R>) { | |
| // FutureProvider – dispose after the Future completes. | |
| (_controller as Future<R>).then( | |
| (resolved) => delegate.dispose?.call(element!, resolved), | |
| onError: (_) {}, // ignore errors, we cannot dispose a failed value | |
| ); | |
| } else if (isLoaded) { | |
| delegate.dispose?.call(element!, value); | |
| } else { | |
| // No value produced; fall back to disposing the controller itself. | |
| // Cast to `dynamic` to avoid a bad runtime cast when T ≠ R. | |
| delegate.dispose?.call(element!, _controller as dynamic); | |
| } |
🤖 Prompt for AI Agents
In packages/provider/lib/src/deferred_inherited_provider.dart around lines 221
to 229, the code incorrectly uses a type check with a type parameter in `if (T
is Future<R>)`, which is invalid in Dart, and performs an unsafe cast
`_controller as R` that can cause runtime errors when T and R differ. To fix
this, replace the invalid type check with a runtime type check on the actual
instance held by _controller, such as using `_controller is Future<R>`, and
avoid casting _controller directly to R; instead, ensure the correct value is
passed to delegate.dispose by handling each case based on the runtime type of
_controller or by restructuring the logic to safely extract the value without
unsafe casts.
| Key? key, | ||
| required Create<Future<T>?> create, | ||
| required T initialData, | ||
| Dispose<T>? dispose, |
There was a problem hiding this comment.
This is impractical because more often than not, it's not T that needs disposing when using futures/streams. It's something else, like a Completer/StreamController/...
There was a problem hiding this comment.
Oh, the problem I was running into was that if it were a Future then I didn't have access to the data since it was already completed. How would I get around this to work for Completer or a StreamController?
There was a problem hiding this comment.
There's no easy solution with the API that Provider offers. We'd have to make a breaking change and do:
StreamProvider(create: (context, ref) {
final controller = StreamController()
ref.onDispose(() => controller.close());
return controller.stream;
})That's the API Riverpod uses.
2 participants
This is useful for disposing data which was created in a future provider.
Summary by CodeRabbit