chore: integrate di needle - WPB-15593 #2447
Conversation
…and NotificationSession impl
echoes-hq
bot
added
echoes: technical-roadmap
There was a problem hiding this comment.
First review, some things are maybe out of scope of the injection, but I guess were needed to test the whole thing.
It would be good to have a graph to show the components hierarchy (to add a .docc for example) to have an overview of this new NSE and its dependencies
| backendEnvironmentProvider: BackendEnvironmentProvider | ||
| ) { | ||
| self.userID = userID | ||
| self.clientID = clientID |
There was a problem hiding this comment.
so needle replace the assemblies ?
There was a problem hiding this comment.
My expectation would be that we no longer need the assembly, but not that Needle is part of WireDomain. Rather, needle is part of a feature module, and is responsible for initializing components from WireDomain.
| } | ||
|
|
||
| coreData.syncContext.performAndWait { | ||
| if DeveloperFlag.proteusViaCoreCrypto.isOn { // do we still need this ? |
There was a problem hiding this comment.
no in new code, you can assume its on by default, so the check is useless
|
|
||
| coreData.syncContext.performAndWait { | ||
| if DeveloperFlag.proteusViaCoreCrypto.isOn { // do we still need this ? | ||
| coreData.syncContext.proteusService = proteusService |
There was a problem hiding this comment.
I would assume we don't continue this hack in WireDomain, or is it needed for a bridge with legacy code. I mean the legacy code relies on the NSManagedObjectContext to provide the proteusService for example, but within WireDomain it would make sense that we inject proteusService where it's needed.
| let mlsFeature = featureRepository.fetchMLS() | ||
|
|
||
| if mlsFeature.isEnabled { | ||
| coreData.syncContext.mlsDecryptionService = mlsDecryptionService |
| } | ||
|
|
||
| /// Provides an authentication service with injected components. | ||
| final class AuthenticationComponent: Component<EmptyDependency>, AuthenticationServiceProvider { |
There was a problem hiding this comment.
AuthenticatedComponent vs AuthenticationComponent difficult to say who does what from the name
| try authenticatedSession.setup() | ||
|
|
||
| let decodedEventsStream = try await authenticatedSession.startSync( | ||
| newEventID: eventID | ||
| ) | ||
|
|
||
| for await decodedEvents in decodedEventsStream { | ||
| generateNotificationContent(for: decodedEvents) | ||
| } |
| attributes: [.eventEnvelopeID: envelope.id] | ||
| ) | ||
|
|
||
| let encoder = JSONEncoder() |
There was a problem hiding this comment.
I guess we can reuse the encoder if we put it outside the forloop
| return AsyncStream { | ||
| $0.yield(events) | ||
| $0.finish() | ||
| } |
There was a problem hiding this comment.
quesstion: what's $0 in our case? I am not sure to understand how this is built
| @@ -71,6 +63,8 @@ final class MessageLocalStoreTests: XCTestCase { | |||
| in: context | |||
| ) | |||
|
|
|||
| conversation.isForcedReadOnly = false | |||
| "branch": "master", | ||
| "revision": "4f43ec612307012b34664b9e95ac680c75622906", | ||
| "version": null |
There was a problem hiding this comment.
should we use a released version instead of head of master?
There was a problem hiding this comment.
question: is Needle part of WireDomain? I would expect it to be part of an NSE target.
| backendEnvironmentProvider: BackendEnvironmentProvider | ||
| ) { | ||
| self.userID = userID | ||
| self.clientID = clientID |
There was a problem hiding this comment.
My expectation would be that we no longer need the assembly, but not that Needle is part of WireDomain. Rather, needle is part of a feature module, and is responsible for initializing components from WireDomain.
| @@ -39,7 +39,6 @@ protocol MLSMessageDecryptorProtocol { | |||
| struct MLSMessageDecryptor: MLSMessageDecryptorProtocol { | |||
|
|
|||
| let mlsDecryptionService: any MLSDecryptionServiceInterface | |||
| let mlsService: any MLSServiceInterface | |||
There was a problem hiding this comment.
question: what this not being used?
| import WireLogging | ||
|
|
||
| // sourcery: AutoMockable | ||
| protocol AuthenticatedSessionProtocol { |
There was a problem hiding this comment.
issue: I'm concerned about the naming here, since this is to be used for the NSE only but it's not clear in the name. I'm wondering if it would make more sense to have a separate target just for the NSE implementation. What do you think?
echoes-hq
bot
removed
the
echoes: technical-roadmap
WPB-15593
Key points
This PR is about integrating and implementing the Needle third-party library, which is a dependency injection mechanism, into the new NSE mechanism.
More details here: https://github.com/uber/needle
Testing
Checklist
[WPB-XXX].