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

Restore ComposableController compatibility between different BaseControllers #458

Merged
merged 2 commits into from
Apr 30, 2021

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Apr 30, 2021

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 Gudahtt requested a review from a team as a code owner April 30, 2021 14:30
@Gudahtt Gudahtt force-pushed the replace-instanceof-with-duck-typing branch from c1364b4 to b6e67d1 Compare April 30, 2021 14:30
…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 Gudahtt force-pushed the replace-instanceof-with-duck-typing branch from b6e67d1 to ca35de0 Compare April 30, 2021 14:32
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.
Copy link
Member

@wachunei wachunei 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 Gudahtt merged commit 12d85f9 into develop Apr 30, 2021
@Gudahtt Gudahtt deleted the replace-instanceof-with-duck-typing branch April 30, 2021 19:30
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
…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.
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.

2 participants