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

Refactor controllers to use "getState" action, "stateChange" event types defined in base-controller #2029

Merged
merged 10 commits into from
Nov 16, 2023

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Nov 13, 2023

Motivation

Explanation

  • This PR refactors all controllers in core so that their respective "getState" action and "stateChange" event types are defined using the ControllerGetStateAction and ControllerStateChangeEvent types from base-controller.
  • The redefined type definitions are exactly equivalent to the previous typings with no breaking changes introduced.

Impact

  • This accomplishes code de-duplication.
  • It makes it possible to propagate any future updates to these types from a single source.
  • All controllers now expect a "getState" action and a "stateChange" event, and can accept them without requiring type casting in base-controller.
  • immer can now be removed as a dependency from 7 packages.

References

Changelog

@metamask/base-controller

  • ADDED: Exports ControllerGetStateAction, ControllerStateChangeEvent types

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@MajorLift MajorLift self-assigned this Nov 13, 2023
@MajorLift MajorLift force-pushed the 231113-refactor-controllers-global-action-event branch from b180817 to 13940f1 Compare November 13, 2023 18:57
@MajorLift MajorLift force-pushed the 231113-refactor-controllers-global-action-event branch from 13940f1 to 591c234 Compare November 13, 2023 19:39
@MajorLift MajorLift marked this pull request as ready for review November 13, 2023 19:40
@MajorLift MajorLift requested review from a team as code owners November 13, 2023 19:40
@MajorLift MajorLift changed the title Refactor controllers to use "getState" action, "stateChange" event defined in base controller [monorepo] Refactor controllers to use "getState" action, "stateChange" event types defined in base-controller Nov 13, 2023
@MajorLift MajorLift changed the title [monorepo] Refactor controllers to use "getState" action, "stateChange" event types defined in base-controller Refactor controllers to use "getState" action, "stateChange" event types defined in base-controller Nov 14, 2023
Base automatically changed from 231023-queued-allowed-actions to main November 15, 2023 16:59
@MajorLift MajorLift force-pushed the 231113-refactor-controllers-global-action-event branch from 582ea07 to 0cb0dda Compare November 15, 2023 18:13
mcmire
mcmire previously approved these changes Nov 15, 2023
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@MajorLift
Copy link
Contributor Author

MajorLift commented Nov 15, 2023

@mcmire I removed some changes in network-controller that were irrelevant to the scope of this PR (replacing string literal controller names with template literals).

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Gudahtt
Copy link
Member

Gudahtt commented Nov 15, 2023

Maybe better to wait until after v91 is merged before merging this, to avoid complicating that release

@MajorLift
Copy link
Contributor Author

@Gudahtt Good call! Holding off for now.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MajorLift MajorLift merged commit 8923aa3 into main Nov 16, 2023
253 checks passed
@MajorLift MajorLift deleted the 231113-refactor-controllers-global-action-event branch November 16, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants