Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork527
Add dispose to FutureProvider#912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Add dispose to FutureProvider#912
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderabbitaibot commentedJun 13, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 withCodeRabbit:
SupportNeed help? Create a ticket on oursupport 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 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
6e7f04b toea68b95CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 receivesthe value instead ofthe 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 thecontroller (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!, _controllerasT); | ||
| if (TisFuture<R>) { | ||
| (_controllerasFuture<R>).then((value)=> delegate.dispose?.call(element!, value)); | ||
| }elseif (isLoaded) { | ||
| delegate.dispose?.call(element!, value); | ||
| }else { | ||
| delegate.dispose?.call(element!, _controllerasR); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Compile-time error & wrong cast inside_CreateDeferredInheritedProviderElement.dispose
if (T is Future<R>)is invalid Dart – the left-hand side ofismust bean 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 potentialTypeError 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!, _controllerasT); | |
| if (TisFuture<R>) { | |
| (_controllerasFuture<R>).then((value)=> delegate.dispose?.call(element!, value)); | |
| }elseif (isLoaded) { | |
| delegate.dispose?.call(element!, value); | |
| }else { | |
| delegate.dispose?.call(element!, _controllerasR); | |
| } | |
| super.dispose(); | |
| if (_didBuild) { | |
| if (_controllerisFuture<R>) { | |
| // FutureProvider – dispose after the Future completes. | |
| (_controllerasFuture<R>).then( | |
| (resolved)=> delegate.dispose?.call(element!, resolved), | |
| onError: (_) {},// ignore errors, we cannot dispose a failed value | |
| ); | |
| }elseif (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!, _controllerasdynamic); | |
| } |
🤖 Prompt for AI Agents
In packages/provider/lib/src/deferred_inherited_provider.dart around lines 221to 229, the code incorrectly uses a type check with a type parameter in `if (Tis 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 fixthis, replace the invalid type check with a runtime type check on the actualinstance held by _controller, such as using `_controller is Future<R>`, andavoid casting _controller directly to R; instead, ensure the correct value ispassed to delegate.dispose by handling each case based on the runtime type of_controller or by restructuring the logic to safely extract the value withoutunsafe casts.| Key? key, | ||
| requiredCreate<Future<T>?> create, | ||
| requiredT initialData, | ||
| Dispose<T>? dispose, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is impractical because more often than not, it's notT that needs disposing when using futures/streams. It's something else, like a Completer/StreamController/...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, the problem I was running into was that if it were aFuture then I didn't have access to the data since it was already completed. How would I get around this to work forCompleter or aStreamController?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
This is useful for disposing data which was created in a future provider.
Summary by CodeRabbit