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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
Sep 28, 2023
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ddf7c70
Fixed PollingController to be a mixin so it can be used with both V1 …
shanejonas Sep 27, 2023
82255b8
Changed onPollingComplete to be by networkClientId
shanejonas Sep 27, 2023
0b79807
Fixed linting
shanejonas Sep 27, 2023
2a2e00a
Removed usused type in PollingController test
shanejonas Sep 27, 2023
7670038
Merge branch 'main' into polling-controller-mixin
shanejonas Sep 27, 2023
5696702
Merge branch 'main' into polling-controller-mixin
shanejonas Sep 28, 2023
ae7372d
Update packages/polling-controller/src/PollingController.ts
shanejonas Sep 28, 2023
3f88265
Update packages/polling-controller/src/PollingController.ts
shanejonas Sep 28, 2023
b28ebd3
Changed JSDoc for mixin return
shanejonas Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 7 additions & 21 deletions packages/polling-controller/src/PollingController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { ControllerMessenger } from '@metamask/base-controller';

import type { PollingCompleteType } from './PollingController';
import PollingController from './PollingController';
import { PollingController } from './PollingController';

const TICK_TIME = 1000;

Expand All @@ -27,7 +26,6 @@ describe('PollingController', () => {
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME,
});
controller.start('mainnet');
jest.advanceTimersByTime(TICK_TIME);
Expand All @@ -48,7 +46,6 @@ describe('PollingController', () => {
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME,
});
const pollingToken = controller.start('mainnet');
jest.advanceTimersByTime(TICK_TIME);
Expand All @@ -69,7 +66,6 @@ describe('PollingController', () => {
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME,
});
const pollingToken1 = controller.start('mainnet');
controller.start('mainnet');
Expand All @@ -93,7 +89,6 @@ describe('PollingController', () => {
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME,
});
controller.start('mainnet');
expect(() => {
Expand All @@ -113,7 +108,6 @@ describe('PollingController', () => {
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME,
});
controller.start('mainnet');
expect(() => {
Expand All @@ -136,7 +130,6 @@ describe('PollingController', () => {
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME,
});
controller.start('mainnet');
jest.advanceTimersByTime(TICK_TIME);
Expand All @@ -158,7 +151,6 @@ describe('PollingController', () => {
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME,
});
controller.start('mainnet');
controller.start('mainnet');
Expand All @@ -177,25 +169,20 @@ describe('PollingController', () => {
}
const name = 'PollingController';

const mockMessenger = new ControllerMessenger<
any,
PollingCompleteType<typeof name>
>();

mockMessenger.subscribe(`${name}:pollingComplete`, pollingComplete);
const mockMessenger = new ControllerMessenger<any, any>();

const controller = new MyGasFeeController({
messenger: mockMessenger,
metadata: {},
name,
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME,
});
controller.onPollingComplete('mainnet', pollingComplete);
const pollingToken = controller.start('mainnet');
controller.stop(pollingToken);
expect(pollingComplete).toHaveBeenCalledTimes(1);
});
it('should poll at the interval length passed via the constructor', async () => {
it('should poll at the interval length when set via setIntervalLength', async () => {
jest.useFakeTimers();

class MyGasFeeController extends PollingController<any, any, any> {
Expand All @@ -208,8 +195,8 @@ describe('PollingController', () => {
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME * 3,
});
controller.setIntervalLength(TICK_TIME * 3);
controller.start('mainnet');
jest.advanceTimersByTime(TICK_TIME);
await Promise.resolve();
Expand Down Expand Up @@ -238,7 +225,6 @@ describe('PollingController', () => {
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME,
});
controller.start('mainnet');
controller.start('rinkeby');
Expand All @@ -259,7 +245,7 @@ describe('PollingController', () => {
controller.stopAll();
});

it('should poll multiple networkClientIds at the interval length passed via the constructor', async () => {
it('should poll multiple networkClientIds when setting interval length', async () => {
jest.useFakeTimers();

class MyGasFeeController extends PollingController<any, any, any> {
Expand All @@ -272,8 +258,8 @@ describe('PollingController', () => {
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
pollingIntervalLength: TICK_TIME * 2,
});
controller.setIntervalLength(TICK_TIME * 2);
controller.start('mainnet');
jest.advanceTimersByTime(TICK_TIME);
await Promise.resolve();
Expand Down
240 changes: 117 additions & 123 deletions packages/polling-controller/src/PollingController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,150 +1,144 @@
import { BaseControllerV2 } from '@metamask/base-controller';
import type {
RestrictedControllerMessenger,
StateMetadata,
} from '@metamask/base-controller';
import { BaseController, BaseControllerV2 } from '@metamask/base-controller';
import type { NetworkClientId } from '@metamask/network-controller';
import type { Json } from '@metamask/utils';
import { v4 as random } from 'uuid';

export type PollingCompleteType<N extends string> = {
type: `${N}:pollingComplete`;
payload: [string];
};
// eslint-disable-next-line @typescript-eslint/ban-types
type Constructor = new (...args: any[]) => {};

shanejonas marked this conversation as resolved.
Show resolved Hide resolved
/**
* PollingController is an abstract class that implements the polling
* functionality for a controller. It is meant to be extended by a controller
* that needs to poll for data by networkClientId.
* PollingControllerMixin
*
* @param Base - The base class to mix onto.
* @returns The mixin.
*/
shanejonas marked this conversation as resolved.
Show resolved Hide resolved
shanejonas marked this conversation as resolved.
Show resolved Hide resolved
export default abstract class PollingController<
Name extends string,
State extends Record<string, Json>,
messenger extends RestrictedControllerMessenger<
Name,
any,
PollingCompleteType<Name> | any,
string,
string
>,
> extends BaseControllerV2<Name, State, messenger> {
readonly #intervalLength: number;
function PollingControllerMixin<TBase extends Constructor>(Base: TBase) {
/**
* PollingController is an abstract class that implements the polling
* functionality for a controller. It is meant to be extended by a controller
* that needs to poll for data by networkClientId.
*
*/
abstract class PollingControllerBase extends Base {
readonly #networkClientIdTokensMap: Map<NetworkClientId, Set<string>> =
new Map();

private readonly networkClientIdTokensMap: Map<NetworkClientId, Set<string>> =
new Map();
readonly #intervalIds: Record<NetworkClientId, NodeJS.Timeout> = {};

private readonly intervalIds: Record<NetworkClientId, NodeJS.Timeout> = {};
#callbacks: Map<
NetworkClientId,
Set<(networkClientId: NetworkClientId) => void>
> = new Map();

constructor({
name,
state,
messenger,
metadata,
pollingIntervalLength,
}: {
name: Name;
state: State;
metadata: StateMetadata<State>;
messenger: messenger;
pollingIntervalLength: number;
}) {
super({
name,
state,
messenger,
metadata,
});
#intervalLength = 1000;

Copy link
Contributor

@adonesky1 adonesky1 Sep 27, 2023 •
edited
Loading

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?

if (!pollingIntervalLength) {
throw new Error('pollingIntervalLength required for PollingController');
getIntervalLength() {
return this.#intervalLength;
}

this.#intervalLength = pollingIntervalLength;
}
setIntervalLength(length: number) {
this.#intervalLength = length;
}

Copy link
Contributor

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?

Copy link
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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@shanejonas shanejonas Sep 28, 2023 •
edited
Loading

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

/**
* Starts polling for a networkClientId
*
* @param networkClientId - The networkClientId to start polling for
* @returns void
*/
start(networkClientId: NetworkClientId) {
const innerPollToken = random();
if (this.networkClientIdTokensMap.has(networkClientId)) {
const set = this.networkClientIdTokensMap.get(networkClientId);
set?.add(innerPollToken);
} else {
const set = new Set<string>();
set.add(innerPollToken);
this.networkClientIdTokensMap.set(networkClientId, set);
/**
* Starts polling for a networkClientId
*
* @param networkClientId - The networkClientId to start polling for
* @returns void
*/
start(networkClientId: NetworkClientId) {
const innerPollToken = random();
if (this.#networkClientIdTokensMap.has(networkClientId)) {
const set = this.#networkClientIdTokensMap.get(networkClientId);
set?.add(innerPollToken);
} else {
const set = new Set<string>();
set.add(innerPollToken);
this.#networkClientIdTokensMap.set(networkClientId, set);
}
this.#poll(networkClientId);
return innerPollToken;
}
this.#poll(networkClientId);
return innerPollToken;
}

/**
* Stops polling for all networkClientIds
*/
stopAll() {
this.networkClientIdTokensMap.forEach((tokens, _networkClientId) => {
tokens.forEach((token) => {
this.stop(token);
/**
* Stops polling for all networkClientIds
*/
stopAll() {
this.#networkClientIdTokensMap.forEach((tokens, _networkClientId) => {
tokens.forEach((token) => {
this.stop(token);
});
});
});
}

/**
* Stops polling for a networkClientId
*
* @param pollingToken - The polling token to stop polling for
*/
stop(pollingToken: string) {
if (!pollingToken) {
throw new Error('pollingToken required');
}
let found = false;
this.networkClientIdTokensMap.forEach((tokens, networkClientId) => {
if (tokens.has(pollingToken)) {
found = true;
this.networkClientIdTokensMap
.get(networkClientId)
?.delete(pollingToken);
if (this.networkClientIdTokensMap.get(networkClientId)?.size === 0) {
clearTimeout(this.intervalIds[networkClientId]);
delete this.intervalIds[networkClientId];
this.networkClientIdTokensMap.delete(networkClientId);
this.messagingSystem.publish(
`${this.name}:pollingComplete`,
networkClientId,
);

/**
* Stops polling for a networkClientId
*
* @param pollingToken - The polling token to stop polling for
*/
stop(pollingToken: string) {
if (!pollingToken) {
throw new Error('pollingToken required');
}
let found = false;
this.#networkClientIdTokensMap.forEach((tokens, networkClientId) => {
if (tokens.has(pollingToken)) {
found = true;
this.#networkClientIdTokensMap
.get(networkClientId)
?.delete(pollingToken);
if (this.#networkClientIdTokensMap.get(networkClientId)?.size === 0) {
clearTimeout(this.#intervalIds[networkClientId]);
delete this.#intervalIds[networkClientId];
this.#networkClientIdTokensMap.delete(networkClientId);
this.#callbacks.get(networkClientId)?.forEach((callback) => {
callback(networkClientId);
});
this.#callbacks.get(networkClientId)?.clear();
}
}
});
if (!found) {
throw new Error('pollingToken not found');
}
});
if (!found) {
throw new Error('pollingToken not found');
}
}

/**
* Executes the poll for a networkClientId
*
* @param networkClientId - The networkClientId to execute the poll for
*/
abstract executePoll(networkClientId: NetworkClientId): Promise<void>;
/**
* Executes the poll for a networkClientId
*
* @param networkClientId - The networkClientId to execute the poll for
*/
abstract executePoll(networkClientId: NetworkClientId): Promise<void>;

#poll(networkClientId: NetworkClientId) {
if (this.intervalIds[networkClientId]) {
clearTimeout(this.intervalIds[networkClientId]);
delete this.intervalIds[networkClientId];
#poll(networkClientId: NetworkClientId) {
if (this.#intervalIds[networkClientId]) {
clearTimeout(this.#intervalIds[networkClientId]);
delete this.#intervalIds[networkClientId];
}
this.#intervalIds[networkClientId] = setTimeout(async () => {
try {
shanejonas marked this conversation as resolved.
Show resolved Hide resolved
await this.executePoll(networkClientId);
} catch (error) {
console.error(error);
}
this.#poll(networkClientId);
}, this.#intervalLength);
}
this.intervalIds[networkClientId] = setTimeout(async () => {
try {
await this.executePoll(networkClientId);
} catch (error) {
console.error(error);

onPollingComplete(
networkClientId: NetworkClientId,
Copy link
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(

Copy link
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

shanejonas marked this conversation as resolved.
Show resolved Hide resolved
callback: (networkClientId: NetworkClientId) => void,
) {
if (this.#callbacks.has(networkClientId)) {
this.#callbacks.get(networkClientId)?.add(callback);
} else {
const set = new Set<typeof callback>();
set.add(callback);
this.#callbacks.set(networkClientId, set);
}
this.#poll(networkClientId);
}, this.#intervalLength);
}
}
return PollingControllerBase;
}

export const PollingController = PollingControllerMixin(BaseControllerV2);
export const PollingControllerV1 = PollingControllerMixin(BaseController);
Loading