Avoid scoping errors in our Dagger setup #1597
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aitorvs
force-pushed
the
feature/aitor/singlein
branch
from
1438942 to
75d13ff
Compare
aitorvs
force-pushed
the
feature/aitor/singlein
branch
from
084dc0f to
2120ac1
Compare
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite and will continue to be automatically updated while this PR remains open. |
aitorvs
force-pushed
the
feature/aitor/singlein
branch
5 times, most recently
from
85bea57 to
de9c77e
Compare
|
Sorry @aitorvs, I've been struggling to get time to review this. Can you either find another reviewer or tolerate waiting until next week for a review? |
aitorvs
force-pushed
the
feature/aitor/singlein
branch
from
de9c77e to
bb87d74
Compare
marcosholgado
approved these changes
Dec 10, 2021
Rename the ObjeGraph suffix to Scope
aitorvs
force-pushed
the
feature/aitor/singlein
branch
from
bb87d74 to
2a4520a
Compare
aitorvs
added a commit
that referenced
this pull request
Dec 13, 2021
…#1604) Task/Issue URL: https://app.asana.com/0/414730916066338/1201432423603230/f ### Description In PR #1597 we're harmonising the way we name dagger components and how we contribute dependencies to them. As a recap: * Use @SingletonIn(AppScope::class) for app scope * Use @SingletonIn(ActivityScope::class) for activity scope * Use @SingletonIn(FragmentScope::class) for fragment scope This a follow up PR to enable nesting those scopes: * AppScope -> ActivityScope -> FragmentScope Nesting means: * child scopes can get access to dependencies in their parent scopes (but not the other way around), ie. * `FragmentScope` would get access to deps in Fragment, Activity and App scopes * `ActivityScope` would get access to deps in Activity and App scopes * `AppScope` would get access to deps in App scope The PR changes the fragments that were not in the `FragmentScope` to use that one instead. Functionality should not be affected at all ### Steps to test this PR Smoke tests for app and AppTP
CDRussell
pushed a commit
that referenced
this pull request
Dec 15, 2021
Task/Issue URL: https://app.asana.com/0/414730916066338/1201415414578549/f ### Description This PR harmonises the way we will scope dependencies in Dagger so that we avoid past mistakes mixing scope annotations resulting in unpexpected behavior. We have also ditched the `ObjectGraph` naming and use `Scope` all over. Recommended to see the [asana task](https://app.asana.com/0/414730916066338/1201415414578549/f) for more information about this. With this PR we will have the following dagger scopes * `AppScope` -> bound to the life of the application * `ActivityScope` -> should be used for Activities * `FragmentScope` -> should be used for Fragments * `QuickSettingsScope` -> only use the quick tile setting for AppTP * `ReceiverScope` -> should be used for broadcast receivers * `VpnScope` -> only used for Vpn service _Exceptions_ We currently do NOT support nesting dagger scopes, this means that the `AppScope` is the parent of ALL remaining scopes and, ALL remaining scopes are siblings, ie. * children scopes have access to the dependencies in the parent scope * sibling scopes do not have access to the dependencies in other siblins Because we don't yet support nesting, some vpn-related activities that SHOULD use the `ActivityScope` are actually using the `VpnScope`, all because they need some dependencies that are contributed to the `VpnScope`. To track those for future fixes, we have added an annotation dubbed `@WrongScope` that provide information as to what should be the appropriate scope. In the future, when nesting is supported, we just need to find usages of that annotation and fix the scope. Incidentally, and because we are heavy users of multi-bindings and unfortunatelly those require the `JvmSuppressWildcards` annotations, I have added a sugar over `Set` and `Map` multibindings to avoid mistakes. ### Steps to test this PR This PR should not change any behavior, just fixes the usage of dagger annotations. If anything breaks, should now break at build time (this is why the `WrongScope` annotation exists now) * Just perform regular smoke tests for the app and AppTP
CDRussell
pushed a commit
that referenced
this pull request
Dec 15, 2021
…#1604) Task/Issue URL: https://app.asana.com/0/414730916066338/1201432423603230/f ### Description In PR #1597 we're harmonising the way we name dagger components and how we contribute dependencies to them. As a recap: * Use @SingletonIn(AppScope::class) for app scope * Use @SingletonIn(ActivityScope::class) for activity scope * Use @SingletonIn(FragmentScope::class) for fragment scope This a follow up PR to enable nesting those scopes: * AppScope -> ActivityScope -> FragmentScope Nesting means: * child scopes can get access to dependencies in their parent scopes (but not the other way around), ie. * `FragmentScope` would get access to deps in Fragment, Activity and App scopes * `ActivityScope` would get access to deps in Activity and App scopes * `AppScope` would get access to deps in App scope The PR changes the fragments that were not in the `FragmentScope` to use that one instead. Functionality should not be affected at all ### Steps to test this PR Smoke tests for app and AppTP
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task/Issue URL: https://app.asana.com/0/414730916066338/1201415414578549/f
Description
This PR harmonises the way we will scope dependencies in Dagger so that we avoid past mistakes mixing scope annotations resulting in unpexpected behavior.
We have also ditched the
ObjectGraphnaming and useScopeall over.Recommended to see the asana task for more information about this.
With this PR we will have the following dagger scopes
AppScope-> bound to the life of the applicationActivityScope-> should be used for ActivitiesFragmentScope-> should be used for FragmentsQuickSettingsScope-> only use the quick tile setting for AppTPReceiverScope-> should be used for broadcast receiversVpnScope-> only used for Vpn serviceExceptions
We currently do NOT support nesting dagger scopes, this means that the
AppScopeis the parent of ALL remaining scopes and, ALL remaining scopes are siblings, ie.Because we don't yet support nesting, some vpn-related activities that SHOULD use the
ActivityScopeare actually using theVpnScope, all because they need some dependencies that are contributed to theVpnScope.To track those for future fixes, we have added an annotation dubbed
@WrongScopethat provide information as to what should be the appropriate scope.In the future, when nesting is supported, we just need to find usages of that annotation and fix the scope.
Incidentally, and because we are heavy users of multi-bindings and unfortunatelly those require the
JvmSuppressWildcardsannotations, I have added a sugar overSetandMapmultibindings to avoid mistakes.Steps to test this PR
This PR should not change any behavior, just fixes the usage of dagger annotations. If anything breaks, should now break at build time (this is why the
WrongScopeannotation exists now)