-
Notifications
You must be signed in to change notification settings - Fork 0
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add ProviderScopes #18
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## main #18 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 215 215
=========================================
Hits 215 215 |
body: ProviderScopes( | ||
providers: [ | ||
[numberProvider], | ||
[doubleNumberProvider], | ||
], | ||
child: Builder( | ||
builder: (context) { | ||
final number = numberProvider.of(context); | ||
final doubleNumber = doubleNumberProvider.of(context); | ||
return Text('$number $doubleNumber'); | ||
}, | ||
), |
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.
If possible, it would be great to add a lint package that detects e.g. ProviderScopes
with only one scope and suggests to change it to ProviderScope
. And if there are two nested ProviderScope
s it suggests refactoring to a ProviderScopes
.
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 model has no sense, it's like using two nested ProviderScopes, which is more simple for the user.
never add a model (complexity) when not needed, we should keep things simple. Just one way of doing things is better
I thought the main problem in the other PR was that there was a lot of nesting involved, and this solution while being cumbersome is at least safer and allows to get rid of repetitive nesting. However, I agree, we should only keep the current implementation in |
Currently the other PR (#17) has some problems: circular dependencies and it is not tested with some provider reusage in different, coexisting subtrees (at the moment it clearly introduces bugs).
This PR does not present the above problems (and in generally it shouldn't introduce new ones, since
ProviderScopes
usesProviderScope
internally without changing the existing code).ProviderScopes
offers a similar API but allows multiple scopes at once. Here is how it looks like: