Add BaseControllerV2 support to ComposableController #447
Conversation
This file contains 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
|
|
Gudahtt
force-pushed
the
restricted-controller-messenger-optional-allowed
branch
from
c81ceaf
to
7c7eb66
Compare
Gudahtt
force-pushed
the
composed-controller-v2-compatibility
branch
from
91d13f2
to
c38021a
Compare
Base automatically changed from
restricted-controller-messenger-optional-allowed
to
develop
April 19, 2021 21:37
The ComposableController now supports controllers that are based upon BaseControllerV2. These newer controllers use the controller messaging system to communicate, so the messenger is now a required constructor parameter if any BaseControllerV2 controllers are used. The `name` property in `BaseControllerV2` had to be made public for the ComposableController to properly construct its state. This seems OK. It was only kept private initially until a use was found for it.
Gudahtt
force-pushed
the
composed-controller-v2-compatibility
branch
from
c38021a
to
6ee460f
Compare
Gudahtt
commented
Apr 20, 2021
rekmarks
previously approved these changes
Apr 22, 2021
These generic type parameters weren't really used in practice. They were passed to the `messagingSystem`, but we erased the type of the messaging system in the one place that we call it, making the more specific types pointless. You can tell that they weren't useful because they were never set in tests, and everything still worked.
|
I've pushed one more commit to simplify the ComposableController type by removing the generic parameters: b1772f2. This should be ready for review again now |
rekmarks
approved these changes
Apr 22, 2021
Gudahtt
added a commit
that referenced
this pull request
Apr 30, 2021
…rollers When adding support for BaseControllerV2 to the ComposableController in #447, we accidentally removed support for using two different versions of the BaseController in the same ComposableController. This is because we used the `instanceof` operator to check whether a controller was extended from BaseController. The `instanceof` check has been replaced by a check for the `subscribe` function. It seems unlikely that a BaseControllerV2 controller will ever have a `subscribe` function because the controller messenger will be used for all events.
Gudahtt
added a commit
that referenced
this pull request
Apr 30, 2021
…rollers When adding support for BaseControllerV2 to the ComposableController in #447, we accidentally removed support for using two different versions of the BaseController in the same ComposableController. This is because we used the `instanceof` operator to check whether a controller was extended from BaseController. The `instanceof` check has been replaced by a check for the `subscribe` function. It seems unlikely that a BaseControllerV2 controller will ever have a `subscribe` function because the controller messenger will be used for all events.
Gudahtt
added a commit
that referenced
this pull request
Apr 30, 2021
…rollers When adding support for BaseControllerV2 to the ComposableController in #447, we accidentally removed support for using two different versions of the BaseController in the same ComposableController. This is because we used the `instanceof` operator to check whether a controller was extended from BaseController. The `instanceof` check has been replaced by a check for the `subscribe` function. It seems unlikely that a BaseControllerV2 controller will ever have a `subscribe` function because the controller messenger will be used for all events.
Gudahtt
added a commit
that referenced
this pull request
Apr 30, 2021
…rollers (#458) When adding support for BaseControllerV2 to the ComposableController in #447, we accidentally removed support for using two different versions of the BaseController in the same ComposableController. This is because we used the `instanceof` operator to check whether a controller was extended from BaseController. The `instanceof` check has been replaced by a check for the `subscribe` function. It seems unlikely that a BaseControllerV2 controller will ever have a `subscribe` function because the controller messenger will be used for all events. The BaseControllerV2 class now prevents the use of the `subscribe` property, ensuring the duck typing strategy used by the ComposableController to tell old and new controllers apart continues to work.
estebanmino
added a commit
that referenced
this pull request
May 4, 2021
This reverts commit 5777094.
Merged
estebanmino
added a commit
that referenced
this pull request
May 20, 2021
* openseainterface * openseainterface * types * fixes MetaMask/metamask-mobile#2264 * collectiblesmetadata * collectiblecontract * compareCollectiblesMetadata * compareCollectiblesMetadatatest * ignoreassigns * lintfix * Revert "Add BaseControllerV2 support to ComposableController (#447)" This reverts commit 5777094. * ApiCollectibleLastSale * address * addmetadata * fixtests * Revert "Revert "Add BaseControllerV2 support to ComposableController (#447)"" This reverts commit c3ebf47. * ignore
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
The ComposableController now supports controllers that are based upon BaseControllerV2. These newer controllers use the controller messaging system to communicate, so the messenger is now a required constructor parameter if any BaseControllerV2 controllers are used. The `name` property in `BaseControllerV2` had to be made public for the ComposableController to properly construct its state. This seems OK. It was only kept private initially until a use was found for it.
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
…rollers (#458) When adding support for BaseControllerV2 to the ComposableController in #447, we accidentally removed support for using two different versions of the BaseController in the same ComposableController. This is because we used the `instanceof` operator to check whether a controller was extended from BaseController. The `instanceof` check has been replaced by a check for the `subscribe` function. It seems unlikely that a BaseControllerV2 controller will ever have a `subscribe` function because the controller messenger will be used for all events. The BaseControllerV2 class now prevents the use of the `subscribe` property, ensuring the duck typing strategy used by the ComposableController to tell old and new controllers apart continues to work.
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
* openseainterface * openseainterface * types * fixes MetaMask/metamask-mobile#2264 * collectiblesmetadata * collectiblecontract * compareCollectiblesMetadata * compareCollectiblesMetadatatest * ignoreassigns * lintfix * Revert "Add BaseControllerV2 support to ComposableController (#447)" This reverts commit 5777094. * ApiCollectibleLastSale * address * addmetadata * fixtests * Revert "Revert "Add BaseControllerV2 support to ComposableController (#447)"" This reverts commit c3ebf47. * ignore
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
The ComposableController now supports controllers that are based upon BaseControllerV2. These newer controllers use the controller messaging system to communicate, so the messenger is now a required constructor parameter if any BaseControllerV2 controllers are used. The `name` property in `BaseControllerV2` had to be made public for the ComposableController to properly construct its state. This seems OK. It was only kept private initially until a use was found for it.
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
…rollers (#458) When adding support for BaseControllerV2 to the ComposableController in #447, we accidentally removed support for using two different versions of the BaseController in the same ComposableController. This is because we used the `instanceof` operator to check whether a controller was extended from BaseController. The `instanceof` check has been replaced by a check for the `subscribe` function. It seems unlikely that a BaseControllerV2 controller will ever have a `subscribe` function because the controller messenger will be used for all events. The BaseControllerV2 class now prevents the use of the `subscribe` property, ensuring the duck typing strategy used by the ComposableController to tell old and new controllers apart continues to work.
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
* openseainterface * openseainterface * types * fixes MetaMask/metamask-mobile#2264 * collectiblesmetadata * collectiblecontract * compareCollectiblesMetadata * compareCollectiblesMetadatatest * ignoreassigns * lintfix * Revert "Add BaseControllerV2 support to ComposableController (#447)" This reverts commit 5777094. * ApiCollectibleLastSale * address * addmetadata * fixtests * Revert "Revert "Add BaseControllerV2 support to ComposableController (#447)"" This reverts commit c3ebf47. * ignore
Open
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The ComposableController now supports controllers that are based upon BaseControllerV2. These newer controllers use the controller messaging system to communicate, so the messenger is now a required constructor parameter if any BaseControllerV2 controllers are used.
The
nameproperty inBaseControllerV2had to be made public for the ComposableController to properly construct its state. This seems OK. It was only kept private initially until a use was found for it.