riverpod
riverpod copied to clipboard
rrousselGit
Should the type difference between autoDispose and non-autoDispose providers be removed?
Currently, if a provider is marked as "autoDispose", it has some voluntary impacts on the type system. Such that:
- a non-autoDispose provider cannot watch an autoDispose provider
- only autoDispose providers have access to ref.keepAlive()
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 Notifier vs AutoDisposeNotifier for the sole purpose of changing Notifier.ref from Ref to AutoDisposeRef.
The thing is, at the time of implementation, riverpod_lint did 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:
- Removing all of the "AutoDisposeX" types, such as AutoDisposeRef vs Ref but also AutoDisposeNotifier, AutoDisposeProvider, ...
- Removing "AlwaysAliveX" types, such as AlwaysAliveProviderBase
- Make ref.keepAlive() accessible on Ref itself instead of AutoDisposeRef
- Implement a warning inside riverpod_lint when an autoDispose provider is listened to in a non-autoDispsoe provider
Is this a change that would be desirable to you?
Please vote using :+1: / :-1:
As an aside, this change would enable having autoDispose be a named parameter on providers instead of the current unusual syntax:
// before:
final provider = Provider.autoDispose((ref) => 0);
// after:
final provider = Provider((ref) => 0, autoDispose: true);
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.
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 mixin MyMixin<T> on Notifier<T> doesn't quite work, because that mixin is not compatible with AutoDisposeNotifier<T>.
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:
-
I never considered the size of Riverpod tombe a blocker. Smaller is nice though.
-
Whilst the lint is useful, is it 100% used by all developers?
-
Are we exchanging build time errors for runtime errors?
-
Would a smaller code base at this time make future releases faster to deliver or is Riverpod approaching a state of "done" feature wise?
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 🤔
Would a smaller code base at this time make future releases faster to deliver or is Riverpod approaching a state of "done" feature wise?
That's the main appeal, yes, along with some simplification of the API surface and the mixin usage.
Currently, it requires quite a bit of effort to have an AutoDispose variant for pretty much everything.
Are we exchanging build time errors for runtime errors?
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?
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 ~runtime error~ silent bugs" is an impossible scenario. That is exactly why there was a compilation error in the first place: to avoid a runtime ~error~ problem. Riverpod was indeed advertised to be compile-time safe on certain topics.
Initially, when reading this, I was very much against this change. Why would one change a great compile-time error into a risky ~run-time error~ silent bug.
Nonetheless, if the following conditions are met...
- riverpod_lint and custom_lint must actually work on Windows and Linux without crashing
- such lints must be advocated as mandatory when installing riverpod
- such lints must work (no false negatives!!)
- such lints produce errors or it should be documented how to move the autodispose lints up to the error level, and not the info level (note related to this: this should happen for scoping lints, too)
- documentation must be updated accordingly
... here's some great reasons to make this happen
- cutting in half the size of the Riverpod codebase means that remi and other contributors can move way faster
- lints will be promoted even more (as mandatory), increasing the DX for riverpod devs
- less complicated API for beginners or for peeps that don't use code gen
- mixins: as mentioned above, it would be possible to write mixins once in a wider and more maintainable way
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.
riverpod_lint and custom_lint must actually work on Windows and Linux without crashing
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 custom_lint in the last 1/2 months?
"no runtime error" is an impossible scenario. That is exactly why there was a compilation error in the first place: to avoid a runtime error.
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 https://github.com/rrousselGit/riverpod/issues/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. If you stopped maintaining it for some reason, I'm quite certain it would survive.
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). Well, integrating Riverpod and custom_lint more tightly would probably help that custom_lint get popular enough to reach the critical mass to ensure its survival.
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 any case, lints are core to my long-term vision of the project.
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 internal class & ignore some lints to make it work without duplication, which is a bad experience.
but, I don't understand why you guys prefer implementing lints only over both lints and runtime error. If you're using lints and it's not broken, you'll not get a runtime error anyway. I find implementing both is fine and more secure.
There's no runtime error needed here.
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.
I'd have to manually do something to add a runtime error here.
What about another poll about this? 😛
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.
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).
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 these risks have to weighted against the benefits that it brings.
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 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.
As of now. you're already missing out, e.g. riverpo_lints excludes a runtime error that would be a huge DX pain. Are you sure the ""risk"" of having an unmaintained package is worth it, since you're using Riverpod already?
It's also by no means a deal-breaker for us either way, I appreciate that you ask for feedback.
Uh, this is not my business, but given what I've wrote above, you should have broken that deal by now.
riverpod is an opinionated tool (...). There are valid reasons for these changes, but they do make it more heavyweight.
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.
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?
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.
As of now. you're already missing out, e.g. riverpo_lints excludes a runtime error that would be a huge DX pain. Are you sure the ""risk"" of having an unmaintained package is worth it, since you're using Riverpod already?
Yes, for our team right now, the benefits of having these links are not worth the additional dependencies, in my opinion.
Uh, this is not my business, but given what I've wrote above, you should have broken that deal by now.
No. I am happy with riverpod and I will continue using it, even if I don't 100% agree with all the new decisions. In the end, this is a gripe over a relatively minor issue and it would be silly to switch to another solution over that. Remi asked for feedback, I gave my thoughts.
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? 👀
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.
@TimWhiting yes, the dependency_overrides is one thing that I'd like to avoid. I have been burned by analyzer_plugin before, because google did not always update it to the latest analyzer dependency in the past and this lead to issues like this and this.
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 AlwaysAlive providers at all given that we have ref.keepAlive() now.
I mean, any always alive provider can just first-line a ref.keepAlive() and the final result would be essentially the same.
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) You'd be surprised how many bugs tests can catch. I think that a provider living longer than intended should be in this space.
