Deprecate <ControllerName>:stateChange in favor of :stateChanged#8187
Deprecate <ControllerName>:stateChange in favor of :stateChanged#8187
Conversation
mcmire
force-pushed
the
deprecate-state-change-event
branch
from
dbd3929 to
d8d71e5
Compare
mcmire
changed the title
Since the `:stateChange` event was added to `BaseController`, we have added a guideline asking engineers to use past tense for all event names. This commit updates BaseController to align with that guideline. To achieve backward compatibility, we add a new event, `:stateChanged`, rather than replacing the existing event. We deprecate `:stateChange` by adding some custom ESLint rules.
mcmire
force-pushed
the
deprecate-state-change-event
branch
from
d8d71e5 to
41daada
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // replaces the rule entirely rather than merging arrays. | ||
| 'no-restricted-syntax': [ | ||
| 'error', | ||
| ...getNoRestrictedSyntaxEntries(typescript), |
There was a problem hiding this comment.
Feels like there ought to be a better way to do this...
| expect(messengerSpy).toHaveBeenNthCalledWith( | ||
| // 1. AccountsController:stateChange | ||
| 2, | ||
| expect(messengerSpy).toHaveBeenCalledWith( |
There was a problem hiding this comment.
The name of this test indicates that all we care about is that the accountAdded event is published, so I not only fixed the test but changed it to reflect that.
| expect(messengerSpy).toHaveBeenNthCalledWith( | ||
| // 1. AccountsController:stateChange | ||
| 2, | ||
| expect(messengerSpy).toHaveBeenCalledWith( |
| accountsController.state.internalAccounts.selectedAccount, | ||
| ).toStrictEqual(mockNonEvmAccount.id); | ||
|
|
||
| expect(messengerSpy.mock.calls).toHaveLength(2); // state change and then selectedAccountChange |
There was a problem hiding this comment.
Similar story; based on the test name it seems all we care about is that the selectedEvmAccountChange event is not published.
|
|
||
| // check the 3rd call since the 2nd one is for the | ||
| // event stateChange | ||
| // Some of these calls are for state changes |
There was a problem hiding this comment.
Since there is a new state change event it changes the number of expected events.
| >['type'] extends MessengerEvents<ControllerMessenger>['type'] | ||
| ? ControllerMessenger | ||
| : never | ||
| : ControllerStateChangedEvent< |
There was a problem hiding this comment.
Controllers must at least support :stateChange or :stateChanged. (We don't want to require that controllers support both :stateChange and :stateChanged, so we can't use ControllerEvents here anymore.)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
1 participant
Explanation
Since the
:stateChangeevent was added to BaseController, we have added a guideline which suggests that engineers use past tense for all event names. This commit updatesBaseControllerto align with that guideline. To achieve backward compatibility, we add a new event,:stateChanged, rather than replacing the existing event. We deprecate subscribing to or delegating:stateChangeby adding some custom ESLint rules.References
We are about to add the
:cacheUpdatetoBaseDataServicehere, so I thought I'd deprecateBaseController:stateChangeso we can addBaseDataService:cacheUpdatedinstead.Checklist
Note
Medium Risk
Touches
BaseController’s core event surface: it now emits an additional state event and updates typings/linting, which could affect downstream consumers and event expectations despite backward compatibility.Overview
BaseControllernow publishes a new${ControllerName}:stateChangedevent (and registers its initial payload) alongside the existing${ControllerName}:stateChange, and exports the newControllerStateChangedEventtype while markingControllerStateChangeEventas deprecated.Repo-wide guidance is updated to prefer
:stateChanged, and ESLint now flags subscribing to or delegating:stateChangevia a customno-restricted-syntaxselector rule (with correspondingeslint-suppressions.jsonupdates). Tests and docs are adjusted to account for the extra publish calls and the new event name.Written by Cursor Bugbot for commit 635f6df. This will update automatically on new commits. Configure here.