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

feat(preferences-controller): Convert to BaseControllerV2 #3713

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jan 4, 2024

Explanation

The PreferencesController has been migrated to BaseControllerV2. As part of this migration, the unused name state property has also been removed.

References

Closes #3708

Changelog

@metamask/preferences-controller

Changed

  • BREAKING: Convert to BaseControllerV2
    • The constructor parameters have changed; rather than accepting an empty "config" parameter and a "state" parameter, there is now just a single object for all constructor arguments. This object has a mandatory messenger and an optional state property.
    • Additional type exports have been added for the controller messenger and associated 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

@Gudahtt

This comment was marked as resolved.

@Gudahtt Gudahtt force-pushed the convert-preferences-controller-to-base-controller-v2 branch 2 times, most recently from a2d6420 to ab2cb6c Compare January 4, 2024 16:42
@Gudahtt Gudahtt force-pushed the clean-up-preferences-controller-types branch from 320415c to 0ffc8af Compare January 4, 2024 19:45
Base automatically changed from clean-up-preferences-controller-types to main January 4, 2024 20:34
@Gudahtt Gudahtt force-pushed the convert-preferences-controller-to-base-controller-v2 branch 4 times, most recently from 69003d7 to 941bc4e Compare January 8, 2024 21:41
@Gudahtt Gudahtt marked this pull request as ready for review January 8, 2024 21:42
@Gudahtt Gudahtt requested a review from a team as a code owner January 8, 2024 21:42
@Gudahtt Gudahtt force-pushed the convert-preferences-controller-to-base-controller-v2 branch from 941bc4e to 24e158f Compare January 9, 2024 14:21
The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

Closes #3708
@Gudahtt Gudahtt force-pushed the convert-preferences-controller-to-base-controller-v2 branch from b92b379 to e74bd82 Compare January 9, 2024 21:05
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.

One question, but looks good regardless.

* @returns A PreferencesController instance.
*/
function setupPreferencesController(
options: Partial<ConstructorParameters<typeof PreferencesController>[0]> = {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: For some other controllers we have an *Options type that holds the options. Think it's worth it to extract that? I guess this is also fine too, just a bit verbose.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I actually find that pattern makes the code it bit harder to read. I prefer declaring options inline rather than in types declared somewhere else.

@Gudahtt Gudahtt merged commit 02b4cca into main Jan 9, 2024
136 checks passed
@Gudahtt Gudahtt deleted the convert-preferences-controller-to-base-controller-v2 branch January 9, 2024 21:21
@Gudahtt Gudahtt mentioned this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade PreferencesController to BaseControllerV2
2 participants