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}
| 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.)
…leOptions
The getNoRestrictedSyntaxEntries function was redundant with the
pre-existing collectExistingRuleOptions helper. It was also less
robust: it used '?? []' instead of an Array.isArray guard, meaning it
would throw a TypeError if an upstream config ever set the rule to a
non-array value (e.g. 'off' or 0).
Replace the call site with collectExistingRuleOptions('no-restricted-syntax',
[typescript]) and remove the now-unused helper.
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
4 participants
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 maintain backward compatibility, we add a new event,:stateChanged, rather than replacing the existing event. We also 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 state publication path by emitting an additional event and expanding event typing, which could impact any controller/messenger wiring and listener assumptions. Backward compatibility is maintained, but consumers may see extra publishes and new lint failures when using:stateChange.Overview
Adds a new state update event name across controllers.
BaseControllernow emits and registers initial payloads for both${Controller}:stateChangeand the new${Controller}:stateChanged, and introduces/exportControllerStateChangedEventwhile markingControllerStateChangeEventas deprecated.Guides and nudges consumers to migrate. Controller guidelines/docs are updated to prefer
:stateChanged, tests are updated to validate both events (and adjust expectations for additional publishes), and ESLint adds a customno-restricted-syntaxrule to flag subscribing to or delegating:stateChange(with new suppressions where needed).Written by Cursor Bugbot for commit 5d5e0df. This will update automatically on new commits. Configure here.