Deprecate BaseControllerV1 and use BaseControllerV2 as default#2078
Merged
Deprecate BaseControllerV1 and use BaseControllerV2 as default#2078
BaseControllerV1 and use BaseControllerV2 as default#2078Conversation
d58f9a1 to
bd3e200
Compare
bd3e200 to
253872a
Compare
mcmire
reviewed
Nov 21, 2023
Contributor
mcmire
left a comment
There was a problem hiding this comment.
Had a couple of comments.
| export type { BaseConfig, BaseState, Listener } from './BaseController'; | ||
| export { BaseController } from './BaseController'; | ||
| export type { BaseConfig, BaseState, Listener } from './BaseControllerV1'; | ||
| export { BaseController as BaseControllerV1 } from './BaseControllerV1'; |
Contributor
There was a problem hiding this comment.
What do you think about renaming BaseController to BaseControllerV1 in BaseControllerV1.ts? That way it matches the file name and we don't have to alias it here.
Contributor
Author
There was a problem hiding this comment.
We can definitely do that. Applied here: 42a87e7
Merged
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
10 tasks
Mrtenz
approved these changes
Nov 22, 2023
mcmire
approved these changes
Nov 22, 2023
mcmire
added a commit
that referenced
this pull request
Nov 22, 2023
…2078) ## Motivation The controller-messenger pattern implemented by `BaseControllerV2` is intended to be used by all of our controllers. Currently, the default `BaseController` name points to the older base controller implementation, which needs to be deprecated and only used for temporary backwards compatibility while all controllers are brought up-to-date. ## Explanation In this commit, we export `BaseControllerV2` as the default `BaseController`, and add a `@deprecated` tsdoc tag to the old `BaseController` class as well as export it under the new name `BaseControllerV1` to discourage future usage. This aligns with the `PollingController` naming scheme, where the up-to-date version is the default, and outdated mixins are named with qualifiers (`PollingControllerOnly`, `PollingControllerV1`). ## Impact Existing controller classes in core that extend from the old base controller are not affected — they now simply extend from `BaseControllerV1` instead of `BaseController`. External packages will need to update any instances of `extends BaseController` to `extends BaseControllerV1` and `extends BaseControllerV2` to `extends BaseController`, along with accompanying changes to import statements. --------- Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The controller-messenger pattern implemented by
BaseControllerV2is intended to be used by all of our controllers. Currently, the defaultBaseControllername points to the older base controller implementation, which needs to be deprecated and only used for temporary backwards compatibility while all controllers are brought up-to-date.Explanation
In this commit, we export
BaseControllerV2as the defaultBaseController, and add a@deprecatedtsdoc tag to the oldBaseControllerclass as well as export it under the new nameBaseControllerV1to discourage future usage.This aligns with the
PollingControllernaming scheme, where the up-to-date version is the default, and outdated mixins are named with qualifiers (PollingControllerOnly,PollingControllerV1).Impact
Existing controller classes in core that extend from the old base controller are not affected — they now simply extend from
BaseControllerV1instead ofBaseController.External packages will need to update any instances of
extends BaseControllertoextends BaseControllerV1andextends BaseControllerV2toextends BaseController, along with accompanying changes to import statements.References
BaseControllerreferences in controllers that are not located in core #2080Changelog
@metamask/base-controllerChanged
BaseControllerexport now points to the up-to-date implementation that was formerly namedBaseControllerV2.BaseControllerclass is marked as deprecated with the@deprecatedtsdoc tag and exported under the nameBaseControllerV1.Checklist