-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
feat: Upgrade alert controller to base controller v2 #28054
base: develop
Are you sure you want to change the base?
feat: Upgrade alert controller to base controller v2 #28054
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. |
Builds ready [3c04312]
Page Load Metrics (2279 ± 109 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
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.
Looks good! only minor comments.
}; | ||
|
||
const defaultState: AlertControllerState = { | ||
export const getDefaultAlertControllerState = (): AlertControllerState => ({ |
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.
nit: can we add some jsdoc
for this exported var?
|
||
const alertMessenger = controllerMessenger.getRestricted({ | ||
const setupController = ({ |
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.
nit: we could use the withController
pattern we have been using for other controllers
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.
Sure, it make sense to have a common coding patterns.
7c3949e
Builds ready [7c3949e]
Page Load Metrics (1957 ± 74 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
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.
Overall looks pretty good, just had some suggestions on simplifying certain sections.
web3ShimUsageOrigins[origin] = value; | ||
this.store.updateState({ web3ShimUsageOrigins }); | ||
this.update((state) => { | ||
state.web3ShimUsageOrigins = state.web3ShimUsageOrigins || {}; |
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.
Do we need this line? It wasn't there in the original code. We also already check for the presence of this property on line 220.
state.web3ShimUsageOrigins = state.web3ShimUsageOrigins || {}; |
await withController({ state: initialState }, ({ controller }) => { | ||
expect(controller.state).toStrictEqual({ | ||
alertEnabledness: { | ||
unconnectedAccount: true, | ||
web3ShimUsage: true, | ||
}, | ||
unconnectedAccountAlertShownOrigins: { | ||
testUnconnectedOrigin: false, | ||
}, | ||
web3ShimUsageOrigins: { | ||
testWeb3ShimUsageOrigin: 0, | ||
}, | ||
}); |
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.
Hmm, this test is funny. "Default state" implies that no initial state is passed and yet this controller was instantiated with initial state in the test setup. And now we are explicitly passing state. So I feel like one or both of these things isn't needed.
Perhaps we can simplify it to the following?
await withController({ state: initialState }, ({ controller }) => { | |
expect(controller.state).toStrictEqual({ | |
alertEnabledness: { | |
unconnectedAccount: true, | |
web3ShimUsage: true, | |
}, | |
unconnectedAccountAlertShownOrigins: { | |
testUnconnectedOrigin: false, | |
}, | |
web3ShimUsageOrigins: { | |
testWeb3ShimUsageOrigin: 0, | |
}, | |
}); | |
const defaultState = getDefaultAlertControllerState(); | |
await withController({ state: defaultState }, ({ controller }) => { | |
expect(controller.state).toStrictEqual(defaultState); |
alertController.store.getState().alertEnabledness.unconnectedAccount, | ||
).toStrictEqual(true); | ||
it('should default unconnectedAccount of alertEnabledness to true', async () => { | ||
await withController({ state: initialState }, ({ controller }) => { |
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.
Do we need to pass initial state? The controller is defined such the state
option is optional, in which case it just uses the default state. So it seems like we could say:
await withController({ state: initialState }, ({ controller }) => { | |
await withController(({ controller }) => { |
await withController( | ||
{ state: initialState }, | ||
({ controller, messenger }) => { |
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.
Similar point as above: can we simplify this?
await withController( | |
{ state: initialState }, | |
({ controller, messenger }) => { | |
await withController(({ controller, messenger }) => { |
I won't make suggestions on the remaining tests, but the same applies.
messenger.call('AlertController:getState').web3ShimUsageOrigins, | ||
).toStrictEqual(defaultWeb3ShimUsageOrigins); | ||
}, | ||
); | ||
}); | ||
}); | ||
|
||
describe('AlertController:stateChange', () => { |
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.
Hmm, is this test needed at all? We don't often test that a controller's stateChange
event gets published when its state changes, because that functionality is provided by BaseController, and we already test it there. Perhaps we can remove it entirely?
Description
Following the Wallet Framework team's OKRs for Q3 2024, we want to bring AlertController up to date with our latest controller patterns.
Related issues
Fixes: #25915
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist