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

shanejonas · 2023-10-04T20:43:55Z

Explanation

Adds options parameter passed around to allow a unique key to be derived per networkClientId + option combination. This handles cases where a Controller need more than just networkClientId to poll on, like an address for example.

References

Fixes https://github.com/MetaMask/MetaMask-planning/issues/1406

Changelog

`@metamask/polling-controller`

ADDED: options to PollingController Mixin to help handle cases where a Controller need more than just networkClientId

…troller need more than just networkClientId

packages/polling-controller/src/PollingController.ts

packages/polling-controller/src/PollingController.test.ts

…t to handle different options and stopping

packages/polling-controller/src/PollingController.test.ts

packages/polling-controller/src/PollingController.ts

packages/polling-controller/src/PollingController.test.ts

packages/polling-controller/src/PollingController.ts

packages/polling-controller/src/PollingController.test.ts

…d guards

packages/polling-controller/src/PollingController.test.ts

packages/polling-controller/src/PollingController.ts

adonesky1

LGTM. A few small nits. Still would like approval from @mcmire before we merge.

mcmire

My main question is around the name we want to choose for the keys in this.#networkClientIdTokensMap (and other places), however, I'd also like to see the options receive a more accurate type (or at the very least, a consistent type instead of sometimes any and sometimes object).

packages/polling-controller/src/PollingController.test.ts

packages/polling-controller/src/PollingController.ts

…ingController

adonesky1

Still wondering if we could add another test here (per this comment) but otherwise LGTM. Maybe lets do Elliot one last pass before merging.

packages/polling-controller/src/PollingController.test.ts

mcmire

I feel like there's still a bit of confusion on what things hold what things and what they should be called. I have a couple more comments on that.

mcmire · 2023-10-11T18:33:57Z

packages/polling-controller/src/PollingController.ts

@@ -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();


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();

mcmire · 2023-10-11T18:41:31Z

packages/polling-controller/src/PollingController.ts

+
+      const key = getKey(networkClientId, options);
+
+      const pollingGroupId = this.#pollingGroupIds.get(key);


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.

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

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

pollingTokenSet

👍

#pollingTokenSets

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

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

mcmire

Looks good! Sorry for all of the back and forth.

adonesky1

🎉

adonesky1 · 2023-10-11T21:07:43Z

packages/polling-controller/src/PollingController.ts

+  options: Json,
+): PollingGroupId => `${networkClientId}:${stringify(options)}`;
+
+type PollingGroupId = `${NetworkClientId}:${string}`;


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

…troller need more than just networkClientId (#1776) ## Explanation Adds `options` parameter passed around to allow a unique key to be derived per networkClientId + option combination. This handles cases where a Controller need more than just networkClientId to poll on, like an `address` for example. Fixes MetaMask/MetaMask-planning#1406 ## Changelog ### `@metamask/polling-controller` - **ADDED**: options to PollingController Mixin to help handle cases where a Controller need more than just networkClientId --------- Co-authored-by: Alex Donesky <[email protected]>

…d` -> `stopPollingByPollingToken` (#1810) Fast-follow to #1776, just a few (breaking) naming cleanups ### CHANGED - **BREAKING:** `executePoll` renamed to `_executePoll` - **BREAKING:** `stopPollingByNetworkClientId` renamed to `stopPollingByPollingToken`

Added options to PollingController to help handle cases where the Con…

03e93b3

…troller need more than just networkClientId

shanejonas requested a review from a team as a code owner October 4, 2023 20:43

adonesky1 reviewed Oct 4, 2023

View reviewed changes

packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Oct 4, 2023

View reviewed changes

packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Oct 4, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

shanejonas added 2 commits October 5, 2023 09:42

Fixed feedback from PR around id variable name and expanding on tes…

bf36d98

…t to handle different options and stopping

Merge branch 'main' into polling-controller-generic-options

10d36e2

shanejonas commented Oct 5, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

Update packages/polling-controller/src/PollingController.test.ts

7234504

adonesky1 reviewed Oct 5, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

adonesky1 requested review from mcmire, jiexi and BelfordZ October 5, 2023 17:44

Fixed test to match calls + renamed outerPollToken

2c0c219

jiexi reviewed Oct 5, 2023

View reviewed changes

packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved

jiexi reviewed Oct 5, 2023

View reviewed changes

packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved

shanejonas added 2 commits October 5, 2023 11:01

Merge branch 'main' into polling-controller-generic-options

472119d

Fixed sorting for options to generate a unique key + fix type

f6fdd42

mcmire reviewed Oct 5, 2023

View reviewed changes

shanejonas added 2 commits October 6, 2023 12:36

Fixed if statement around callbacks

01192af

Added JSDoc for getKey for PollingController

556f1cf

shanejonas commented Oct 6, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

shanejonas added 6 commits October 6, 2023 09:51

Update packages/polling-controller/src/PollingController.test.ts

b8c0e00

Merge branch 'main' into polling-controller-generic-options

ac6b7a7

Merge branch 'main' into polling-controller-generic-options

4286bbc

Added test case for different networkClientId + same options and fixe…

4919cb3

…d guards

Changed JSON.stringify to fast-json-stable-stringify

95d54e8

Fixed linting

585544e

adonesky1 requested review from mcmire and jiexi October 10, 2023 18:04

adonesky1 reviewed Oct 10, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Oct 10, 2023

View reviewed changes

packages/polling-controller/src/PollingController.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Oct 10, 2023

View reviewed changes

mcmire reviewed Oct 10, 2023

View reviewed changes

Changed options type to Json and cleaned up naming of id/key for Poll…

5da575d

…ingController

adonesky1 previously approved these changes Oct 11, 2023

View reviewed changes

shanejonas commented Oct 11, 2023

View reviewed changes

packages/polling-controller/src/PollingController.test.ts Outdated Show resolved Hide resolved

Update packages/polling-controller/src/PollingController.test.ts

bed5016

shanejonas dismissed adonesky1’s stale review via bed5016 October 11, 2023 18:38

mcmire reviewed Oct 11, 2023

View reviewed changes

Fixed naming with #pollingTokenSets

d35f471

mcmire approved these changes Oct 11, 2023

View reviewed changes

Merge branch 'main' into polling-controller-generic-options

b689e51

adonesky1 approved these changes Oct 11, 2023

View reviewed changes

adonesky1 reviewed Oct 11, 2023

View reviewed changes

adonesky1 merged commit d53d83a into main Oct 11, 2023

adonesky1 deleted the polling-controller-generic-options branch October 11, 2023 21:20

adonesky1 mentioned this pull request Oct 11, 2023

Rename executePoll -> _executePoll + stopPollingByNetworkClientId -> stopPollingByPollingToken #1810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shanejonas commented Oct 4, 2023

adonesky1 left a comment

mcmire left a comment

adonesky1 left a comment

mcmire left a comment

mcmire Oct 11, 2023 •

edited

Loading

mcmire Oct 11, 2023

adonesky1 Oct 11, 2023

shanejonas Oct 11, 2023

adonesky1 Oct 11, 2023 •

edited

Loading

adonesky1 Oct 11, 2023

mcmire left a comment

adonesky1 left a comment

adonesky1 Oct 11, 2023 •

edited

Loading

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

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


		const key = getKey(networkClientId, options);

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

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

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

Conversation

shanejonas commented Oct 4, 2023

Explanation

References

Changelog

@metamask/polling-controller

adonesky1 left a comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

adonesky1 left a comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

mcmire Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

mcmire Oct 11, 2023

Choose a reason for hiding this comment

adonesky1 Oct 11, 2023

Choose a reason for hiding this comment

shanejonas Oct 11, 2023

Choose a reason for hiding this comment

adonesky1 Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

adonesky1 Oct 11, 2023

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

adonesky1 left a comment

Choose a reason for hiding this comment

adonesky1 Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

`@metamask/polling-controller`

mcmire Oct 11, 2023 •

edited

Loading

adonesky1 Oct 11, 2023 •

edited

Loading

adonesky1 Oct 11, 2023 •

edited

Loading