Changed PollingController to be a mixin so it can be used with both V1 and V2 controllers #1736
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
…and V2 controllers
shanejonas
force-pushed
the
polling-controller-mixin
branch
from
c162a8b
to
82255b8
Compare
shanejonas
commented
Sep 27, 2023
shanejonas
commented
Sep 27, 2023
shanejonas
changed the title
jiexi
reviewed
Sep 27, 2023
| } | ||
| setIntervalLength(length: number) { | ||
| this.#intervalLength = length; | ||
| } |
There was a problem hiding this comment.
does intervalLength need to be private if we're just exposing it?
There was a problem hiding this comment.
We could make it protected so that the extending class could decide to expose it or not. I doubt we need this to be configurable at runtime very often, if ever.
There was a problem hiding this comment.
Can't make it protected with the mixin pattern, i get this error:
Property 'intervalLength' of exported class expression may not be private or protected.ts(4094)
There was a problem hiding this comment.
I think there is a way to do this (make it protected), just not if I have an abstract class where the class has to be named.
adonesky1
reviewed
Sep 27, 2023
| } catch (error) { | ||
| console.error(error); | ||
|
|
||
| onPollingComplete( |
There was a problem hiding this comment.
[nit]
Suggested change
| onPollingComplete( | |
| setOnPollingComplete( |
There was a problem hiding this comment.
Seems a bit more conventional as-is IMO. Event listeners are usually on____. Or add___Listener
adonesky1
reviewed
Sep 27, 2023
| messenger, | ||
| metadata, | ||
| }); | ||
| #intervalLength = 1000; |
There was a problem hiding this comment.
[nit] Wondering if we ought remove a default and throw if never set?
Gudahtt
reviewed
Sep 27, 2023
Gudahtt
reviewed
Sep 27, 2023
Gudahtt
reviewed
Sep 27, 2023
Gudahtt
reviewed
Sep 27, 2023
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Gudahtt
approved these changes
Sep 28, 2023
adonesky1
approved these changes
Sep 28, 2023
adonesky1
added a commit
that referenced
this pull request
Sep 29, 2023
Integrates recently introduced [`PollingController` mixin](#1736) with `GasFeeController`. Leaves old polling pattern in pace for now so as to not force a ton of breaking changes for mobile, but with intention to activate new pattern in both clients ASAP. Addresses: MetaMask/MetaMask-planning#1314
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
…1 and V2 controllers (#1736) ## Explanation - Currently the PollingController extends from BaseControllerV2 but we need to use it for both V1 and V2 controllers. - This PR uses typescript [mixins](https://www.typescriptlang.org/docs/handbook/mixins.html) to achieve a faux-multiple inheritance which TypeScript has support for. The pattern allows you to create a class which is a merge of many classes. I've added a V1 and a V2 version of the PollingController that can be used almost the same way as the original `PollingController`. --------- Co-authored-by: Mark Stacey <markjstacey@gmail.com>
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
Integrates recently introduced [`PollingController` mixin](#1736) with `GasFeeController`. Leaves old polling pattern in pace for now so as to not force a ton of breaking changes for mobile, but with intention to activate new pattern in both clients ASAP. Addresses: MetaMask/MetaMask-planning#1314
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
…1 and V2 controllers (#1736) ## Explanation - Currently the PollingController extends from BaseControllerV2 but we need to use it for both V1 and V2 controllers. - This PR uses typescript [mixins](https://www.typescriptlang.org/docs/handbook/mixins.html) to achieve a faux-multiple inheritance which TypeScript has support for. The pattern allows you to create a class which is a merge of many classes. I've added a V1 and a V2 version of the PollingController that can be used almost the same way as the original `PollingController`. --------- Co-authored-by: Mark Stacey <markjstacey@gmail.com>
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
Integrates recently introduced [`PollingController` mixin](#1736) with `GasFeeController`. Leaves old polling pattern in pace for now so as to not force a ton of breaking changes for mobile, but with intention to activate new pattern in both clients ASAP. Addresses: MetaMask/MetaMask-planning#1314
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.
Explanation
allows you to create a class which is a merge of many classes. I've added a V1 and a V2 version of the PollingController that can be used almost the same way as the original
PollingController.