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

Added options to PollingController to help handle cases where the Controller need more than just networkClientId #1776

Merged
merged 19 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
03e93b3
Added options to PollingController to help handle cases where the Con…
shanejonas Oct 4, 2023
bf36d98
Fixed feedback from PR around `id` variable name and expanding on tes…
shanejonas Oct 5, 2023
10d36e2
Merge branch 'main' into polling-controller-generic-options
shanejonas Oct 5, 2023
7234504
Update packages/polling-controller/src/PollingController.test.ts
shanejonas Oct 5, 2023
2c0c219
Fixed test to match calls + renamed outerPollToken
shanejonas Oct 5, 2023
472119d
Merge branch 'main' into polling-controller-generic-options
shanejonas Oct 5, 2023
f6fdd42
Fixed sorting for options to generate a unique key + fix type
shanejonas Oct 5, 2023
01192af
Fixed if statement around callbacks
shanejonas Oct 6, 2023
556f1cf
Added JSDoc for `getKey` for PollingController
shanejonas Oct 6, 2023
b8c0e00
Update packages/polling-controller/src/PollingController.test.ts
shanejonas Oct 6, 2023
ac6b7a7
Merge branch 'main' into polling-controller-generic-options
shanejonas Oct 6, 2023
4286bbc
Merge branch 'main' into polling-controller-generic-options
shanejonas Oct 10, 2023
4919cb3
Added test case for different networkClientId + same options and fixe…
shanejonas Oct 10, 2023
95d54e8
Changed JSON.stringify to fast-json-stable-stringify
shanejonas Oct 10, 2023
585544e
Fixed linting
shanejonas Oct 10, 2023
5da575d
Changed options type to Json and cleaned up naming of id/key for Poll…
shanejonas Oct 11, 2023
bed5016
Update packages/polling-controller/src/PollingController.test.ts
shanejonas Oct 11, 2023
d35f471
Fixed naming with #pollingTokenSets
shanejonas Oct 11, 2023
b689e51
Merge branch 'main' into polling-controller-generic-options
adonesky1 Oct 11, 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
1 change: 1 addition & 0 deletions packages/polling-controller/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"@metamask/network-controller": "^14.0.0",
"@metamask/utils": "^8.1.0",
"@types/uuid": "^8.3.0",
"fast-json-stable-stringify": "^2.1.0",
"uuid": "^8.3.2"
},
"devDependencies": {
Expand Down
68 changes: 52 additions & 16 deletions packages/polling-controller/src/PollingController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,40 @@ describe('PollingController', () => {
await Promise.resolve();
expect(controller.executePoll).toHaveBeenCalledTimes(2);
});
it('should start and stop polling sessions for different networkClientIds with the same options', async () => {
jest.useFakeTimers();

class MyGasFeeController extends PollingController<any, any, any> {
executePoll = createExecutePollMock();
}
const mockMessenger = new ControllerMessenger<any, any>();

const controller = new MyGasFeeController({
messenger: mockMessenger,
metadata: {},
name: 'PollingController',
state: { foo: 'bar' },
});
const pollToken1 = controller.startPollingByNetworkClientId('mainnet', {
address: '0x1',
});
controller.startPollingByNetworkClientId('mainnet', { address: '0x2' });
controller.startPollingByNetworkClientId('sepolia', { address: '0x2' });
jest.advanceTimersByTime(TICK_TIME);
await Promise.resolve();
expect(controller.executePoll).toHaveBeenCalledTimes(3);
controller.stopPollingByNetworkClientId(pollToken1);
jest.advanceTimersByTime(TICK_TIME);
await Promise.resolve();
expect(controller.executePoll).toHaveBeenCalledTimes(5);
expect(controller.executePoll.mock.calls).toMatchObject([
['mainnet', { address: '0x1' }],
['mainnet', { address: '0x2' }],
['sepolia', { address: '0x2' }],
['mainnet', { address: '0x2' }],
['sepolia', { address: '0x2' }],
]);
});
});
describe('multiple networkClientIds', () => {
it('should poll for each networkClientId', async () => {
Expand All @@ -231,16 +265,16 @@ describe('PollingController', () => {
jest.advanceTimersByTime(TICK_TIME);
await Promise.resolve();
expect(controller.executePoll.mock.calls).toMatchObject([
['mainnet'],
['rinkeby'],
['mainnet', {}],
['rinkeby', {}],
]);
jest.advanceTimersByTime(TICK_TIME);
await Promise.resolve();
expect(controller.executePoll.mock.calls).toMatchObject([
['mainnet'],
['rinkeby'],
['mainnet'],
['rinkeby'],
['mainnet', {}],
['rinkeby', {}],
['mainnet', {}],
['rinkeby', {}],
]);
controller.stopAllPolling();
});
Expand All @@ -267,27 +301,29 @@ describe('PollingController', () => {
expect(controller.executePoll.mock.calls).toMatchObject([]);
jest.advanceTimersByTime(TICK_TIME);
await Promise.resolve();
expect(controller.executePoll.mock.calls).toMatchObject([['mainnet']]);
expect(controller.executePoll.mock.calls).toMatchObject([
['mainnet', {}],
]);
jest.advanceTimersByTime(TICK_TIME);
await Promise.resolve();
expect(controller.executePoll.mock.calls).toMatchObject([
['mainnet'],
['sepolia'],
['mainnet', {}],
['sepolia', {}],
]);
jest.advanceTimersByTime(TICK_TIME);
await Promise.resolve();
expect(controller.executePoll.mock.calls).toMatchObject([
['mainnet'],
['sepolia'],
['mainnet'],
['mainnet', {}],
['sepolia', {}],
['mainnet', {}],
]);
jest.advanceTimersByTime(TICK_TIME);
await Promise.resolve();
expect(controller.executePoll.mock.calls).toMatchObject([
['mainnet'],
['sepolia'],
['mainnet'],
['sepolia'],
['mainnet', {}],
['sepolia', {}],
['mainnet', {}],
['sepolia', {}],
]);
});
});
Expand Down
94 changes: 61 additions & 33 deletions packages/polling-controller/src/PollingController.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,25 @@
import { BaseController, BaseControllerV2 } from '@metamask/base-controller';
import type { NetworkClientId } from '@metamask/network-controller';
import type { Json } from '@metamask/utils';
import stringify from 'fast-json-stable-stringify';
import { v4 as random } from 'uuid';

// Mixin classes require a constructor with an `...any[]` parameter
// See TS2545
type Constructor = new (...args: any[]) => object;

/**
* Returns a unique key for a networkClientId and options. This is used to group networkClientId polls with the same options
* @param networkClientId - The networkClientId to get a key for
* @param options - The options used to group the polling events
* @returns The unique key
*/
export const getKey = (
networkClientId: NetworkClientId,
options: Json,
): PollingGroupId => `${networkClientId}:${stringify(options)}`;

type PollingGroupId = `${NetworkClientId}:${string}`;
Copy link
Contributor

@adonesky1 adonesky1 Oct 11, 2023

Choose a reason for hiding this comment

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

[nit] last thing is maybe we should align with the pollingTokenSet verbiage -> PollingTokenSetId?

/**
* PollingControllerMixin
*
Expand All @@ -20,10 +34,9 @@ function PollingControllerMixin<TBase extends Constructor>(Base: TBase) {
*
*/
abstract class PollingControllerBase extends Base {
readonly #networkClientIdTokensMap: Map<NetworkClientId, Set<string>> =
new Map();
readonly #pollingGroupIds: Map<PollingGroupId, Set<string>> = new Map();
Copy link
Contributor

@mcmire mcmire Oct 11, 2023

Choose a reason for hiding this comment

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

pollingGroupIds sounds like this variable holds an array of ids. What do you think about:

Suggested change
readonly #pollingGroupIds: Map<PollingGroupId, Set<string>> = new Map();
readonly #pollTokenGroups: Map<PollingGroupId, Set<string>> = new Map();

On the other hand, it seems like when we use this variable we really care about the poll tokens. Also, intervalIds and callbacks seem to be named after the values that they hold, and so maybe it makes sense for this variable to follow the same pattern:

Suggested change
readonly #pollingGroupIds: Map<PollingGroupId, Set<string>> = new Map();
readonly #pollTokens: Map<PollingGroupId, Set<string>> = new Map();


readonly #intervalIds: Record<NetworkClientId, NodeJS.Timeout> = {};
readonly #intervalIds: Record<PollingGroupId, NodeJS.Timeout> = {};

#callbacks: Map<
NetworkClientId,
Expand All @@ -49,27 +62,34 @@ function PollingControllerMixin<TBase extends Constructor>(Base: TBase) {
* Starts polling for a networkClientId
*
* @param networkClientId - The networkClientId to start polling for
* @param options - The options used to group the polling events
* @returns void
*/
startPollingByNetworkClientId(networkClientId: NetworkClientId) {
startPollingByNetworkClientId(
networkClientId: NetworkClientId,
options: Json = {},
) {
const innerPollToken = random();
if (this.#networkClientIdTokensMap.has(networkClientId)) {
const set = this.#networkClientIdTokensMap.get(networkClientId);
set?.add(innerPollToken);

const key = getKey(networkClientId, options);

const pollingGroupId = this.#pollingGroupIds.get(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right? I thought each value in #pollingGroupIds is set of poll tokens. It seems that key here is really the polling group ID.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah currently the type you have returned for getKey is PollingGroupId, so presumably you'd use the pollingGroupId to get a pollingGroup or pollingTokenGroup or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea really it should be pollingTokenSet, and #pollingGroupIds should be #pollingTokenSets or #pollingTokensByPollingGroupId

Copy link
Contributor

@adonesky1 adonesky1 Oct 11, 2023

Choose a reason for hiding this comment

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

pollingTokenSet

👍

#pollingTokenSets

I'd vote either this or just pollingTokens per @mcmire 's comment

Copy link
Contributor

Choose a reason for hiding this comment

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

probably pollingTokenSets though since it would be kindof weird for pollingTokenSet to be a subset of pollingTokens?

if (pollingGroupId) {
pollingGroupId.add(innerPollToken);
} else {
const set = new Set<string>();
set.add(innerPollToken);
this.#networkClientIdTokensMap.set(networkClientId, set);
this.#pollingGroupIds.set(key, set);
}
this.#poll(networkClientId);
this.#poll(networkClientId, options);
return innerPollToken;
}

/**
* Stops polling for all networkClientIds
*/
stopAllPolling() {
this.#networkClientIdTokensMap.forEach((tokens, _networkClientId) => {
this.#pollingGroupIds.forEach((tokens, _networkClientId) => {
tokens.forEach((token) => {
this.stopPollingByNetworkClientId(token);
});
Expand All @@ -86,20 +106,18 @@ function PollingControllerMixin<TBase extends Constructor>(Base: TBase) {
throw new Error('pollingToken required');
}
let found = false;
this.#networkClientIdTokensMap.forEach((tokens, networkClientId) => {
this.#pollingGroupIds.forEach((tokens, key) => {
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);
tokens.delete(pollingToken);
if (tokens.size === 0) {
clearTimeout(this.#intervalIds[key]);
delete this.#intervalIds[key];
this.#pollingGroupIds.delete(key);
this.#callbacks.get(key)?.forEach((callback) => {
callback(key);
});
this.#callbacks.get(networkClientId)?.clear();
this.#callbacks.get(key)?.clear();
}
}
});
Expand All @@ -112,24 +130,29 @@ function PollingControllerMixin<TBase extends Constructor>(Base: TBase) {
* Executes the poll for a networkClientId
*
* @param networkClientId - The networkClientId to execute the poll for
* @param options - The options passed to startPollingByNetworkClientId
*/
abstract executePoll(networkClientId: NetworkClientId): Promise<void>;
abstract executePoll(
shanejonas marked this conversation as resolved.
Show resolved Hide resolved
networkClientId: NetworkClientId,
options: Json,
): Promise<void>;

#poll(networkClientId: NetworkClientId) {
if (this.#intervalIds[networkClientId]) {
clearTimeout(this.#intervalIds[networkClientId]);
delete this.#intervalIds[networkClientId];
#poll(networkClientId: NetworkClientId, options: Json) {
const key = getKey(networkClientId, options);
if (this.#intervalIds[key]) {
clearTimeout(this.#intervalIds[key]);
delete this.#intervalIds[key];
}
// setTimeout is not `await`ing this async function, which is expected
// We're just using async here for improved stack traces
// eslint-disable-next-line @typescript-eslint/no-misused-promises
this.#intervalIds[networkClientId] = setTimeout(async () => {
this.#intervalIds[key] = setTimeout(async () => {
try {
await this.executePoll(networkClientId);
await this.executePoll(networkClientId, options);
} catch (error) {
console.error(error);
}
this.#poll(networkClientId);
this.#poll(networkClientId, options);
}, this.#intervalLength);
}

Expand All @@ -138,17 +161,22 @@ function PollingControllerMixin<TBase extends Constructor>(Base: TBase) {
*
* @param networkClientId - The networkClientId to listen for polling complete events
* @param callback - The callback to execute when polling is complete
* @param options - The options used to group the polling events
*/
onPollingCompleteByNetworkClientId(
networkClientId: NetworkClientId,
callback: (networkClientId: NetworkClientId) => void,
options: Json = {},
) {
if (this.#callbacks.has(networkClientId)) {
this.#callbacks.get(networkClientId)?.add(callback);
} else {
const key = getKey(networkClientId, options);
const callbacks = this.#callbacks.get(key);

if (callbacks === undefined) {
const set = new Set<typeof callback>();
set.add(callback);
this.#callbacks.set(networkClientId, set);
this.#callbacks.set(key, set);
} else {
callbacks.add(callback);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2164,6 +2164,7 @@ __metadata:
"@types/jest": ^27.4.1
"@types/uuid": ^8.3.0
deepmerge: ^4.2.2
fast-json-stable-stringify: ^2.1.0
jest: ^27.5.1
ts-jest: ^27.1.4
typedoc: ^0.24.8
Expand Down
Loading