-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Decouple the DaggerWorkerFactory using plugins #1075
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
Conversation
b33533d to
8ade7ef
Compare
8ade7ef to
f9ec26c
Compare
f9ec26c to
6177d9a
Compare
marcosholgado
left a comment
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.
Nice job @aitorvs! I noticed the clear data notification does not show up anymore, left a couple of comments about that.
| notificationDao: NotificationDao, | ||
| notificationFactory: NotificationFactory, | ||
| pixel: Pixel, | ||
| privacyProtectionNotification: PrivacyProtectionNotification |
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.
Should this be ClearDataNotification?
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.
good catch! done
| private val notificationDao: NotificationDao, | ||
| private val notificationFactory: NotificationFactory, | ||
| private val pixel: Pixel, | ||
| private val privacyProtectionNotification: PrivacyProtectionNotification |
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.
Should this be ClearDataNotification?
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.
good catch! done
Thanks @marcosholgado, good catch! it should be fixed now! |
marcosholgado
left a comment
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.
Nice one! 👍
Task/Issue URL: https://app.asana.com/0/0/1199916263225059/f
Tech Design URL:
CC:
Description:
Use dagger multibindings to decouple the
DaggerWorkerFactory.The
DaggerWorkerFactoryis a big object that creates the workers AND inject its dependencies. For that reason it needs to know and depend on ALL dependencies all the app workers require. Which makes it a god object.In this PR I use dagger multibindings to setup a kind of plugin system where the
DaggerWorkerFactorycan delegate the injection of dependencies to the worker plugins.Steps to test this PR:
Internal references:
Software Engineering Expectations
Technical Design Template