-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
chore: #12184 MVP split engine file #12366
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
…/12184-split-engine-file
Bitrise✅✅✅ Commit hash: 8443cc7 Note
|
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.
We are not exporting via index for AccountsController here since it exposes the risk of circular dependencies (caused by action/event dependencies)
Skipping sonar failure (Engine.ts) since this PR just removes code from the file |
Bitrise✅✅✅ Commit hash: f69b6d3 Note
|
Quality Gate failedFailed conditions |
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.
LGTM
Description
This PR establishes a pattern for splitting out controllers from the main
Engine.ts
file, which provides the following benefitsThe POC here applies to the
AccountsController
initialization logic, which has been mostly abstracted out intoAccountsController
directory. This pattern should be applicable to other controllers in the Engine file as well.This pattern also enables us to redirect users to vault recovery in the event of controller initialization failures and should reduce the chances of users being "bricked". These changes may be introduced in future interations
Related issues
Fixes: #12184
Manual testing steps
The changes should not affect app behavior
Screenshots/Recordings
Before
After
App still runs normally
app.mov
Pre-merge author checklist
Pre-merge reviewer checklist