Skip to content

Changed PollingController to be a mixin so it can be used with both V1 and V2 controllers - #1736

Merged
adonesky1 merged 9 commits into
mainfrom
polling-controller-mixin
Sep 28, 2023
Merged

Changed PollingController to be a mixin so it can be used with both V1 and V2 controllers#1736
adonesky1 merged 9 commits into
mainfrom
polling-controller-mixin

Conversation

@shanejonas

@shanejonasshanejonas commented Sep 27, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Explanation

  • Currently the PollingController extends from BaseControllerV2 but we need to use it for both V1 and V2 controllers.
  • This PR uses typescript mixins 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.

@shanejonas
shanejonas requested a review from a team as a code ownerSeptember 27, 2023 15:59
@shanejonas
shanejonasforce-pushed the polling-controller-mixin branch from c162a8b to 82255b8CompareSeptember 27, 2023 16:13
shanejonas
shanejonas commented Sep 27, 2023
View reviewed changes
Comment threadpackages/polling-controller/src/PollingController.ts Outdated
shanejonas
shanejonas commented Sep 27, 2023
View reviewed changes
Comment threadpackages/polling-controller/src/PollingController.ts Outdated
@shanejonasshanejonas changed the title Fixed PollingController to be a mixin so it can be used with both V1 and V2 controllersChanged PollingController to be a mixin so it can be used with both V1 and V2 controllersSep 27, 2023
jiexi
jiexi reviewed Sep 27, 2023
View reviewed changes
Comment threadpackages/polling-controller/src/PollingController.ts
}
setIntervalLength(length: number) {
this.#intervalLength = length;
}

@jiexijiexiSep 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does intervalLength need to be private if we're just exposing it?

@GudahttGudahttSep 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@shanejonasshanejonasSep 28, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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)

image

@shanejonasshanejonasSep 28, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

see: microsoft/TypeScript#30355

adonesky1
adonesky1 reviewed Sep 27, 2023
View reviewed changes
Comment threadpackages/polling-controller/src/PollingController.ts
} catch (error) {
console.error(error);

onPollingComplete(

@adonesky1adonesky1Sep 27, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit]

Suggested change
onPollingComplete(
setOnPollingComplete(

@GudahttGudahttSep 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems a bit more conventional as-is IMO. Event listeners are usually on____. Or add___Listener

adonesky1
adonesky1 reviewed Sep 27, 2023
View reviewed changes
Comment threadpackages/polling-controller/src/PollingController.ts
messenger,
metadata,
});
#intervalLength = 1000;

@adonesky1adonesky1Sep 27, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] Wondering if we ought remove a default and throw if never set?

adonesky1
adonesky1 previously approved these changes Sep 27, 2023
View reviewed changes

@adonesky1adonesky1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Gudahtt
Gudahtt reviewed Sep 27, 2023
View reviewed changes
Comment threadpackages/polling-controller/src/PollingController.ts Outdated
Gudahtt
Gudahtt reviewed Sep 27, 2023
View reviewed changes
Comment threadpackages/polling-controller/src/PollingController.ts
Gudahtt
Gudahtt reviewed Sep 27, 2023
View reviewed changes
Comment threadpackages/polling-controller/src/PollingController.ts
Gudahtt
Gudahtt reviewed Sep 27, 2023
View reviewed changes

@GudahttGudahtt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great! I can re-review after some of these suggestions are resolved

shanejonasand others added 3 commits September 28, 2023 06:23
@adonesky1
adonesky1 merged commit 897ba05 into mainSep 28, 2023
@adonesky1
adonesky1 deleted the polling-controller-mixin branch September 28, 2023 14:52
@adonesky1adonesky1 mentioned this pull request Sep 28, 2023
Merged
adonesky1 added a commit that referenced this pull request Sep 29, 2023
@adonesky1
Integrate PollingController mixin with GasFeeController (#1673)
fd03dc6
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
@shanejonasshanejonas mentioned this pull request Oct 6, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GudahttGudahttGudahtt approved these changes
@adonesky1adonesky1adonesky1 approved these changes
@jiexijiexiAwaiting requested review from jiexi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shanejonas@jiexi@Gudahtt@adonesky1