Should the type difference between autoDispose and non-autoDispose providers be removed? #2576
Comments
|
For reference, this change alone would probably cut in half the size of the Riverpod codebase. The difference between Ref/AutoDisposeRef & co involves lots of type duplications to get to work. |
|
This would also help with folks wanting to make a mixin on Notifiers. Currently writing But if there's no difference between those two, the problem should disappear |
|
In case this is still unclear, it's about removing the following compilation error: https://riverpod.dev/docs/concepts/modifiers/auto_dispose#the-argument-type-autodisposeprovider-cant-be-assigned-to-the-parameter-type-alwaysaliveproviderbase Instead, it would be implemented as a warning in riverpod_lint (with a much better error message, and possibly a quick-fix) |
|
Things that might help/or not:
|
|
Sorry additional thought... Does having to support two types for mixins a real blocker? Does it make sense to have a modified version for each type Vs presumably checking if it is autodispose? Would love to know what use cases there are here as never thought to make one. Sounds interesting 🤔 |
That's the main appeal, yes, along with some simplification of the API surface and the mixin usage.
We could either have no error and let users do whatever they want, or a runtime error yes. Riverpod used to not have any error for this a while ago. An error was requested because the mistake was fairly common. About mixins, folks regularly want to refactor logic they have which is reused in multiple Notifiers. The mixin case I described is a fairly regular question. I see it pop-up once a week approximately. |
|
@rrousselGit Sounds good, with this change the following mixin could already be used in all variations of Async/Notifier? import 'package:riverpod_annotation/riverpod_annotation.dart';
part 'generated_providers.g.dart';
mixin IncrementMixin on Notifier<int> {
void increment() {
ref; // ref valid for all notifiers
state = state + 1;
}
}
/// Valid
@Riverpod(keepAlive: true)
class CounterAlive extends _$CounterAlive with IncrementMixin {
@override
int build() {
return 0;
}
}
/// 'IncrementMixin' can't be mixed onto 'AutoDisposeNotifier<int>'
/// because 'AutoDisposeNotifier<int>' doesn't implement 'Notifier<int>'.
@riverpod
class Counter extends _$Counter with IncrementMixin {
@override
int build() {
return 0;
}
}
/// 'IncrementMixin' can't be mixed onto '_$CounterFamily'
/// because '_$CounterFamily' doesn't implement 'Notifier<int>'.
@riverpod
class CounterFamily extends _$CounterFamily with IncrementMixin {
@override
int build({
required int initialCount,
}) {
return initialCount;
}
}
/// 'IncrementMixin' can't be mixed onto '_$CounterFamilyAlive'
/// because '_$CounterFamilyAlive' doesn't implement 'Notifier<int>'.
@Riverpod(keepAlive: true)
class CounterFamilyAlive extends _$CounterFamilyAlive with IncrementMixin {
@override
int build({
required int initialCount,
}) {
return initialCount;
}
} |
|
@MiniSuperDev I'd likely expose a "NotifierBase" which would be shared between Notifier and FamilyNotifier & the code-generated ones. So you could do: mixin IncrementMixin on NotifierBase<int> {
void increment() {
ref; // ref valid for all notifiers
state = state + 1;
}
}We currently can't quite do this because of the Ref vs AutoDisposeRef. That's the main limiting factor. As an aside, folks could also make a mixin for providers with specific parameters. Like: mixin MyMixin on NotifierBase<int> {
// The notifier has an "id" parameter.
String get id;
void doSomething() {
print('id($state)');
}
}
...
@riverpod
class Example extends _$Example with MyMixin {
int build({required String id}) => 0;
}
// Works too:
@Riverpod(keepAlive: true)
class Example extends _$Example with MyMixin {
int build({required String id}) => 0;
} |
|
If there's no runtime error, then I'm okay with this change. Changing a compile time error to a runtime error just isn't worth it. |
|
To clarify, for those who are against the runtime error, are you okay with using lints to ensure that you aren't keeping auto dispose providers alive by listening within a regular provider? |
I'm fine with a warning lint personally, as long as there's no runtime error. |
|
TLDR: yes on these changes, if some pre-conditions are met. "no Initially, when reading this, I was very much against this change. Why would one change a great compile-time error into a risky Nonetheless, if the following conditions are met...
... here's some great reasons to make this happen
All in all, while removing a compile time error sounds really bad, lints can answer this if you config them accordingly. I realize I wrote a lot of pre-conditions but I feel those are necessary to safely route users towards a safe, simpler API. |
Every windows/Linux machine I have and those from folks I know don't have an issue with custom_lint. At that point, if you have a crash, there's nothing I can do without getting more information. |
|
This is OT, but I'll dig into it and reach you back. I assume some updates have been made onto |
|
Nothing regarding the crash you're referring to. |
There's no runtime error needed here. I'd have to manually do something to add a runtime error here. Otherwise, nothing terrible would happen, besides maybe an autoDispose provider not getting disposed because it's still used by a different provider. But to begin with, a devtool is on its way to showcase what prevents an autoDispose provider from disposing. So debugging that would be much simpler. |
|
I see the appeal of vastly simplifying the codebase and reducing its size. However, as a user of Riverpod currently not using custom_lint, I am against that change. I do not have anything against custom_lint and I think is a great tool. But when developing an app that is supposed to live several years, adopting such a third-party tool is also a risk as it is not guaranteed to be maintained, and if it breaks in a future version of Flutter and nobody of the original maintainers were still there to fix it, this it would cause pain, and these risks have to weighted against the benefits that it brings. As for the app that I am currently working on, the official supported lints are good enough for now, we decided to not use custom_lint for now. A change like this would force us to re-evaluate this decision, as the API without different types for AutoDipose/Non-AutoDispose would be more error-prone without riverpod_lint enabled. Since you are also a maintainer of custom_lint, that's probably a good thing for you ;) It's also by no means a deal-breaker for us either way, I appreciate that you ask for feedback. If you go the route of removing the AutoDispose-Types, I'd suggest making it clear in the documentation that riverpod is an opinionated tool that strongly encourages the use of riverpod_lint in order to use it safely. |
|
I don't understand the maintenance issue, because Riverpod and custom_lint are both maintained by me. If I stop maintaining stuff, that'll impact both Riverpod and custom_lint equally. In any case, lints are core to my long-term vision of the project. One example is #2356, which is making scoping providers "compile safe" through lints. |
|
Riverpod is now one of the most popular state management packages and a critical part of many production apps. And I think an experienced developer could get to an understanding of where they could do at least light maintenance relatively quickly, I have at least a general idea of what riverpod does though debugging issues that I had. For custom_lint, I'm not so sure about both these aspects (to be fair: I don't know much about the architecture). My main gripe is that I committed to using riverpod when it was a relatively light-weight library, and it is starting to become more of an opinionated framework. There are valid reasons for these changes, but they do make it more heavyweight.
In this case, I'd say go ahead with the changes. |
|
I find the pros outweigh the cons here. I tried making mixins for both autoDispose and non-autoDispose providers and ended up using but, I don't understand why you guys prefer implementing |
Yeah, I'm sorry, I hadn't my coffee this morning 😛 This makes me believe that the scenario might be even worse: no compilation error, no runtime error, just a silent bug (as you've mentioned). See this.
What about another poll about this? 😛
That's just perfect. |
|
I want to further share my opinion (as if someone cares, but still) and ask some questions (I do really care).
You're experiencing that risk right now, by using this lib. Are you sure you want to use riverpod and not developing your own internal and supposedly safer solution?
As of now. you're already missing out, e.g.
Uh, this is not my business, but given what I've wrote above, you should have broken that deal by now.
I'm confused about this. What's opinionated about Riverpod, given that lints should be mandatory and that one should pick the ecosystem as a whole? What's "heavy" about Riverpod? 👀 |
|
Not sure if this is related, but could this also remove WidgetRef? |
No. WidgetRef is here to stay. It is voluntary that Ref and WidgetRef are dissociated. WidgetRef isn't meant to be used a lot. Using WidgetRef is equivalent to putting logic into your UI. Use Ref instead, which is correctly separated from the UI. |
|
Can the linter be combined into the Riverpod package? Since this essentially makes using a linter almost a necessity it makes sense to simplify the setup and not have to tell users to add a different package. It would just be nice to have. All in all, I think the type difference being removed is a good idea. |
Yes, but there's some nuance there. I'm not suggesting avoiding all dependencies at all costs, but to be wary of creating debt by using too many dependencies and weighing to benefits of the risks that each dependency brings. Many developers have seen projects that use way too many third-party packages for everything to the point where it becomes very hard to update anything. When I choose riverpod, I committed to having a dependency on riverpod, which is a pure Dart project with only dependencies on meta, stack_trace, and state_notifier. I did not commit to also having to pull in custom_lint, which has dependencies that are known to be annoying because of frequent breaking changes like analyzer and analyzer_plugin.
Yes, for our team right now, the benefits of having these links are not worth the additional dependencies, in my opinion.
No. I am happy with riverpod and I will continue using it, even if I don't 100% agree with all the new decisions.
riverpod with a strong recommendation to use riverpod_lint. which depends on custom_lint which depends on analyzer and analyzer_plugin is not as lightweight as just using riverpod. More heavyweight does not necessarily worse, if the added features are worth it. |
|
Note that custom_lint + analyzer + analyzer_plugin would be dev dependencies, and analyzer is already an implicit dev dependency in all dart projects (correct me if I'm wrong). I think the main problem in the past with analyzer as a dev dependency is that analyzer often makes breaking changes, and having it pinned in the pubspec can make the analyzer used by the dart sdk and the analyzer version required by the package mismatch. This can often be solved with a simple dependency override until one or the other gets updated, or staying on an old version of the dart / flutter sdk, and as Remi envisions the lints to be a major portion of riverpod, I assume any problems with upgrades to the dart/flutter sdk should be solved quickly. |
|
@knaeckeKami thank you for answering my doubts, it was very useful. |
|
@TimWhiting yes, the dependency_overrides is one thing that I'd like to avoid. That being said, analyzer_plugin did not make any troubles in the last months, so it seems to be better maintained now. And if the direction is to embrace custom lints anyway, then I'm also not against this change proposed here. |
|
It actually seems a bit weird to have I mean, any always alive provider can just first-line a I would say that if it makes riverpod that much easier to maintain, then do it. extremely rarely do i try to depend on an autodispose provider from an always alive one, and if you're doing that, you probably intended to keep the other one always alive to begin with. In most cases as well, keeping a provider alive unintentionally is not a problem at all. I would argue that your provider tests would catch that instantly. (and these tests are stupid easy to make) |
|
It would also make it much easier to contribute to Riverpod without the whole AlwaysAlive and AutoDispose and Family mess. And honestly, AlwaysAlive providers could just cheat with |
|
It would be great also to be able to change the keep alive status at any moment. |
|
@busslina that's already possible via the current |
|
One component of a code-base I work on relies on being able to type-check whether a provider is capable of auto-disposing. It's a wrapper widget that allows you to give it a list of providers you depend on, and the wrapper widget will make sure these providers have a value before calling the build function. It's basically AsyncValue.when for multiple providers at once (I know one could use a dedicated provider for that). This wrapper widget can't unsubscribe from autoDispose providers before calling the build function, because the value would be disposed of before the build function expecting the value to be present, is called. With non-autoDispose providers, it can unsubscribe before calling the build method because the value will be present. I'm sure there are 47 reasons to avoid the pattern above, like creating a wrapper provider instead, or straight up not allowing autoDispose providers to be passed as dependencies, or 9 better ways to do the same thing. I just wanted to throw one use-case in there where being able to differentiate between autoDispose and non-autoDispose providers is useful. If this functionality would be an exposed property, it would eliminate the need for type-checking. |
I'm not sure how you've implemented this, but, given that you don't want to migrate to a more idiomatic approach, you'd just need to add @riverpod
int yourKeepAlive(YourKeepAliveRef ref) {
ref.keepAlive();
return 0;
}Using |
|
@lucavenir We don't always have control over the providers we use, so the only way to make an existing autoDispose provider keep its state until it's needed, is to wrap it with another provider, or to staying subscribed until someone else (the child) is subscribing to it. Right now, we're type-checking against the providers to see if we have any autoDispose providers. If that's the case, we're rescheduling our unsubscribe to make sure the builder of our child widget is called and can access the provider with a guaranteed value before we unsubscribe. void _subscribeToDependencies() {
final hasAsyncProviders = _dependencies.any(...);
if (!hasAsyncProviders) {
return _transitionIntoReadyState();
}
if (_allProvidersHaveAValue()) {
return _transitionIntoReadyState();
}
// Some providers don't have a value yet, so we need to listen to them
for (final provider in _dependencies) {
final providerSubscription = ref.listenManual(provider, (oldValue, newValue) {
// ...
if (_allProvidersHaveAValue()) {
_transitionIntoReadyState();
}
});
_subscriptions.add(providerSubscription);
}
}
void _transitionIntoReadyState() {
if (mounted) {
_unsubscribeFromDependencies();
// ...
}
}
void _unsubscribeFromDependencies() {
final hasAutoDisposeProviders = _dependencies.any(...);
unsubscribeAndRemove() {
for (final subscription in _subscriptions) {
subscription.close();
}
_subscriptions.clear();
}
if (!hasAutoDisposeProviders) {
// unsubscribe immediately
return unsubscribeAndRemove();
}
// With auto-dispose providers, we need to wait for the next frame
// so the child widget subscribe before we unsubscribe
WidgetsBinding.instance.addPostFrameCallback((_) {
unsubscribeAndRemove();
});
} |
|
imho it goes without saying that what you're trying to do is NOT how you use Riverpod This being said..
Not really @riverpod
int keptAlive(KeptAliveRef ref) {
ref.keepAlive();
return ref.watch(youDontOwnThisOrWhateverProvider);
}And then, you just use Furthermore, while not encouraged, there's ref.exists |
You're literally wrapping the provider with another provider, which is exactly what I said, isn't it? When wrapping the providers with another provider, I will have to watch them in the wrapper widget, even though I don't care about any changes. This means that the wrapper widget will also be rebuilt. While I can use listenManual on the wrapper providers instead of watching, I would still get notified about changes to the providers, and would have to disregard them. This is unnecessary. Staying subscribed to the providers until they have a value allows me to unsubscribe in the wrapper widget. I achieve what I want (eager-load the providers), while minimizing rebuilds/notifications by only having the child be notified on changes, not the wrapper widget.
Needless to say, this discussion here is not about how my widget should be designed, but whether or not the type differentiation between autoDispose and non-autoDispose is neccessary. I just wanted to throw out there that being able to differentiate between autoDispose and non-autoDispose providers is useful in some cases, and that currently, this is done by type-checking. While I have a weird, specific, unorthodox use-case, I might not be the only one wanting to differentiate between the providers. Like I said above, if there was a way to check whether a provider is autoDisposable without having to type-check, via a property for example, the need for type-checking goes away. |
|
This was implemented in the dev branch, and will be released at a later date |
Currently, if a provider is marked as "autoDispose", it has some voluntary impacts on the type system. Such that:
This is helpful to spot mistakes but complexifies the API quite a bit; especially now that we have
Async/Notifier.For example, it requires having a
NotifiervsAutoDisposeNotifierfor the sole purpose of changingNotifier.reffromReftoAutoDisposeRef.The thing is, at the time of implementation,
riverpod_lintdid not exist yet. So the current implementation was the only way to make this error be highlighted in the IDE.But nowadays with
riverpod_lint, we can define custom warnings/errors.This raises the following question:
Should the type difference caused by enabling/disabling autoDispose be removed? The current compilation errors would then be implemented as "lints" in riverpod_lint.
Effectively this would involve:
Is this a change that would be desirable to you?
Please vote using 👍 / 👎
As an aside, this change would enable having
autoDisposebe a named parameter on providers instead of the current unusual syntax:This is a different topic, and I'll discuss this in a different issue if this one is accepted. But it's worth keeping in mind.
The text was updated successfully, but these errors were encountered: