Skip to content
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

refactor: create WireAuthentication feature module #2255

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

johnxnguyen
Copy link
Collaborator

Issue

Initial ideas for a WireAuthentication feature module that would contain the whole login and registration flow.


Checklist

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

UI accessibility checklist

If your PR includes UI changes, please utilize this checklist:

  • Make sure you use the API for UI elements that support large fonts.
  • All colors are taken from WireDesign.ColorTheme or constructed using WireDesign.BaseColorPalette.
  • New UI elements have Accessibility strings for VoiceOver.

Copy link
Contributor

github-actions bot commented Dec 6, 2024

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit d47cbb5.

Comment on lines +23 to +24
@MainActor
func invoke(emailOrSSOCode: String) async -> AuthenticationMethod
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be a good practice to always mark the invoke method as @MainActor because:

  1. It creates a cleaner call side from the view model which should also be main actor isolated (i.e just create a Task and run the use case.
  2. It makes it explicit to the implementation on which thread the use case is being called on and therefore the responsibility of the implantation to manage efficient concurrent execution.

/// To load the contents of the module, create an instance of
/// the assembly, which you can then use to make required views.
@MainActor
public struct WireAuthenticationAssembly {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will there be more code than the assembly in the entry point target?
If not, isn't it a bit too much of an overhead to have a separate target?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there would be anything other than instantiating components, wiring them together and returning them. Where would be another place to put it if we don't have a separate target? It couldn't exist inside either the UI or Core targets because the assembly needs to combine components from both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants