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

[base-controller] stateChange subscribers with selectors always fire on the first state change #3701

Closed
Gudahtt opened this issue Dec 22, 2023 · 0 comments · Fixed by #3702
Closed
Assignees
Labels
bug Something isn't working team-wallet-framework

Comments

@Gudahtt
Copy link
Member

Gudahtt commented Dec 22, 2023

Subscribers to the stateChange event for each controller will always fire today on the first state change, even if the change doesn't affect the selected value. This is because selectors don't yet consider the initial value when determining if there has been a change. The previous value also isn't correctly returned, it's set to undefined for the initial call instead.

We should ensure that event subscriptions that use a selector always compare against the correct previous value, and return the correct previous value when they do fire.

The issue #3649 is about addressing this problem in general, for the "subscriber selector" feature of the ControllerMessenger. This issue is meant to track this problem specifically in how it impacts the ${name}:stateChange controller event that is part of BaseControllerV2.

@Gudahtt Gudahtt added the bug Something isn't working label Dec 22, 2023
Gudahtt added a commit that referenced this issue Dec 22, 2023
Subscribers to the `stateChange` event of any `BaseControllerV2`-based
controllers will now correctly handle the initial state change event.

Previously the initial state change would always result in this event
firing, even for subscriptions with selectors where the selected value
has not changed. Additionally, the `previousValue` returned was always
set to `undefined` the first time.

`BaseControllerV2` has been updated to correctly compare with the
previous value even for the first state change. The returned
`previousValue` is also now guaranteed to be correct even for the
initial state change.

Fixes #3701
@Gudahtt Gudahtt self-assigned this Jan 3, 2024
Gudahtt added a commit that referenced this issue Jan 3, 2024
Subscribers to the `stateChange` event of any `BaseControllerV2`-based
controllers will now correctly handle the initial state change event.

Previously the initial state change would always result in this event
firing, even for subscriptions with selectors where the selected value
has not changed. Additionally, the `previousValue` returned was always
set to `undefined` the first time.

`BaseControllerV2` has been updated to correctly compare with the
previous value even for the first state change. The returned
`previousValue` is also now guaranteed to be correct even for the
initial state change.

Fixes #3701
Gudahtt added a commit that referenced this issue Jan 3, 2024
Subscribers to the `stateChange` event of any `BaseControllerV2`-based
controllers will now correctly handle the initial state change event.

Previously the initial state change would always result in this event
firing, even for subscriptions with selectors where the selected value
has not changed. Additionally, the `previousValue` returned was always
set to `undefined` the first time.

`BaseControllerV2` has been updated to correctly compare with the
previous value even for the first state change. The returned
`previousValue` is also now guaranteed to be correct even for the
initial state change.

Fixes #3701
Gudahtt added a commit that referenced this issue Jan 3, 2024
Subscribers to the `stateChange` event of any `BaseControllerV2`-based
controllers will now correctly handle the initial state change event.

Previously the initial state change would always result in this event
firing, even for subscriptions with selectors where the selected value
has not changed. Additionally, the `previousValue` returned was always
set to `undefined` the first time.

`BaseControllerV2` has been updated to correctly compare with the
previous value even for the first state change. The returned
`previousValue` is also now guaranteed to be correct even for the
initial state change.

Fixes #3701
Gudahtt added a commit that referenced this issue Jan 9, 2024
Subscribers to the `stateChange` event of any `BaseControllerV2`-based
controllers will now correctly handle the initial state change event.

Previously the initial state change would always result in this event
firing, even for subscriptions with selectors where the selected value
has not changed. Additionally, the `previousValue` returned was always
set to `undefined` the first time.

`BaseControllerV2` has been updated to correctly compare with the
previous value even for the first state change. The returned
`previousValue` is also now guaranteed to be correct even for the
initial state change.

Fixes #3701
Gudahtt added a commit that referenced this issue Jan 10, 2024
…3702)

## Explanation

Subscribers to the `stateChange` event of any `BaseControllerV2`-based
controllers will now correctly handle the initial state change event.

Previously the initial state change would always result in this event
firing, even for subscriptions with selectors where the selected value
has not changed. Additionally, the `previousValue` returned was always
set to `undefined` the first time.

`BaseControllerV2` has been updated to correctly compare with the
previous value even for the first state change. The returned
`previousValue` is also now guaranteed to be correct even for the
initial state change.

## References

Fixes #3701

## Changelog

### `@metamask/base-controller`

- Fixed: Subscribers to the `stateChange` event of any
`BaseControllerV2`-based controllers will now correctly handle the
initial state change event
- Previously the initial state change would always result in this event
firing, even for subscriptions with selectors where the selected value
has not changed. Additionally, the `previousValue` returned was always
set to `undefined` the first time.
- `BaseControllerV2` has been updated to correctly compare with the
previous value even for the first state change. The returned
`previousValue` is also now guaranteed to be correct even for the
initial state change.

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working team-wallet-framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant