feat: Invoke DetermineAuthMethodUseCase - WPB-15920

[samwyndham]

Issue

This PR invokes DetermineAuthMethodUseCase. When this is merged DetermineAuthMethodView should be largely complete, excluding the route navigation of loginViaSSO & onPremLogin.

Testing

Run unit tests

Checklist

• Title contains a reference JIRA issue number like [WPB-XXX].
• Description is filled and free of optional paragraphs.
• Adds/updates automated tests.

[samwyndham]

No longer needed

[samwyndham]

No longer needed

[samwyndham]

@KaterinaWire I haven't yet managed to fix this concurrency warning. One solution is to do:

            let task = Task.detached { [determineAuthMethod, emailOrSSOCode] in
                try await determineAuthMethod.invoke(emailOrSSOCode: emailOrSSOCode)
            }
            let method = try await task.value

However the typed error becomes untyped with this which is a little annoying.

[KaterinaWire]

@samwyndham I marked invoke(emailOrSSOCode:) as @mainactor, maybe as a temp solution. We still have same warnings in DetermineAuthMethodUseCase and need to find a proper solution

[johnxnguyen]

I think there are a few ways to solve this that we should explore, but if we mark the use case as main actor, then we must ensure appropriate concurrency inside the use case implementation.

@samwyndham if you don't want to lose the error type you could catch inside the task and return a typed error in the result. This adds a bit of boilerplate, so it could be factored out in a utility extension so the API looks something like:

try await Task.detached<Success, Failure> {
  ...
}

[caldrian]

Could we instead declare DetermineAuthMethodUseCaseProtocol as Sendable and everything else which is needed for the compiler to not produce Sendable related warnings?
Like AuthenticationAPI etc.

[github-actions]

Test Results

24 tests   24 ✅  7s ⏱️
 3 suites   0 💤
 1 files     0 ❌

Results for commit 50558b1.

♻️ This comment has been updated with latest results.

[datadog-wireapp]

Datadog Report

Branch report: feat/invoke-auth-method-use-case-WPB-15920
Commit report: 51f8d68
Test service: wire-ios-mono

✅ 0 Failed, 19 Passed, 0 Skipped, 7.58s Total Time

[johnxnguyen]

thought: I thought about this in the past whether use cases should be invoked on the main thread and internally they manage their own concurrency, or if we should instead defer that thought to the caller (i.e a view model which is marked as MainActor). Perhaps it's a topic for the architecture forum.

[caldrian]

In my opinion the @MainActor requirement here on the protocol should be removed.
Since invoke is declared async anyhow, it's up to the implementation to decide, whether it should run on the main actor or not.

[johnxnguyen]

praise: nice way to keep the view code a more readable.

[johnxnguyen]

I think there are a few ways to solve this that we should explore, but if we mark the use case as main actor, then we must ensure appropriate concurrency inside the use case implementation.

@samwyndham if you don't want to lose the error type you could catch inside the task and return a typed error in the result. This adds a bit of boilerplate, so it could be factored out in a utility extension so the API looks something like:

try await Task.detached<Success, Failure> {
  ...
}

[caldrian]

In my opinion the @MainActor requirement here on the protocol should be removed.
Since invoke is declared async anyhow, it's up to the implementation to decide, whether it should run on the main actor or not.

[caldrian]

Isn't Alert deprecated?
https://developer.apple.com/documentation/swiftui/alert

[caldrian]

Could we instead declare DetermineAuthMethodUseCaseProtocol as Sendable and everything else which is needed for the compiler to not produce Sendable related warnings?
Like AuthenticationAPI etc.