feat(27254): implement new remote-feature-flag-controller #4931
Conversation
DDDDDanica
force-pushed
the
feature/27254-feature-flag-controller
branch
6 times, most recently
from
78af7f4
to
cd93f7a
Compare
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
DDDDDanica
force-pushed
the
feature/27254-feature-flag-controller
branch
from
cd93f7a
to
82f7997
Compare
DDDDDanica
force-pushed
the
feature/27254-feature-flag-controller
branch
2 times, most recently
from
78ca22b
to
a7a3cf1
Compare
DDDDDanica
force-pushed
the
feature/27254-feature-flag-controller
branch
9 times, most recently
from
0519340
to
fcc71a0
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
DDDDDanica
force-pushed
the
feature/27254-feature-flag-controller
branch
6 times, most recently
from
153ebc9
to
0fab160
Compare
DDDDDanica
force-pushed
the
feature/27254-feature-flag-controller
branch
from
f13e7b4
to
6ea8e51
Compare
|
I've coded a proposed alternative to service error handling, can be found here #4995 |
DDDDDanica
force-pushed
the
feature/27254-feature-flag-controller
branch
from
6ea8e51
to
e5e0b5c
Compare
| export const incomingTransactionsLogger = createModuleLogger( | ||
| projectLogger, | ||
| 'remote-feature-flag', | ||
| ); |
There was a problem hiding this comment.
Hmm, I see that you copied this pattern from transaction-controller. It seems that they are putting all of their logger objects in this file. I guess that is okay — there isn't clear guidance on this. But should we at least name this variable and the scope appropriately?
| export const incomingTransactionsLogger = createModuleLogger( | |
| projectLogger, | |
| 'remote-feature-flag', | |
| ); | |
| export const remoteFeatureFlagControllerLogger = createModuleLogger( | |
| projectLogger, | |
| 'remote-feature-flag-controller', | |
| ); |
As an alternative to this, you could put this variable at the top of remote-feature-flag-controller.ts itself and name it log, with the scope named after the class/function/file that's being logged. Here is another example to demonstrate this approach:
And so you could have a similar one for remote-feature-flag-controller like so:
export const log = createModuleLogger(projectLogger, 'RemoteFeatureFlagController');
There was a problem hiding this comment.
Your second suggestion has been applied here 440d170
| // === STATE === | ||
|
|
||
| export type RemoteFeatureFlagControllerState = { | ||
| remoteFeatureFlag: FeatureFlags; |
There was a problem hiding this comment.
I see that this was changed recently. Just want to double-check, should this be remoteFeatureFlags instead of remoteFeatureFlag, since there could be multiple?
| remoteFeatureFlag: FeatureFlags; | |
| remoteFeatureFlags: FeatureFlags; |
| } catch (error) { | ||
| log('Remote feature flag API request failed: %o', error); | ||
| reject(error); | ||
| throw error; |
There was a problem hiding this comment.
It seems that we are returning the promise in the successful case but not the error case. So I see why you added this line. But since we have a promise that we are rejecting, it seems that we ought to be able to use it somehow for all cases...
What if instead of a try/catch we add this when we set up the promise above:
const { promise, resolve, reject } = createDeferredPromise<FeatureFlags>({
suppressUnhandledRejection: true,
});
this.#inProgressFlagUpdate = promise;
promise.finally(() => {
this.#inProgressFlagUpdate = undefined;
});And now we can put return await promise as the very last thing in this method.
|
|
||
| export const controllerName = 'RemoteFeatureFlagController'; | ||
| export const DEFAULT_CACHE_DURATION = 24 * 60 * 60 * 1000; // 1 day | ||
| const log = createModuleLogger(projectLogger, 'ClientConfigApiService'); |
There was a problem hiding this comment.
Ah, I just noticed this. Perhaps we don't need the additional logger in logger.ts? Maybe we just need projectLogger there, and we can correct the scope of this logger:
| const log = createModuleLogger(projectLogger, 'ClientConfigApiService'); | |
| const log = createModuleLogger(projectLogger, 'RemoteFeatureFlagController'); |
There was a problem hiding this comment.
this was addressed in 440d170
| * | ||
| * @returns A promise that resolves to the current set of feature flags. | ||
| */ | ||
| async getRemoteFeatureFlag(): Promise<FeatureFlags> { |
There was a problem hiding this comment.
Similar to a previous comment, did we want to use getRemoteFeatureFlags since we're potentially grabbing multiple? Or is this fine? Just wanted to double-check the API is the way we want it.
There was a problem hiding this comment.
And some more pluralization was done here d0b81fa
…private: true` Co-authored-by: Elliot Winkler <[email protected]>
… name passed to createProjectLogger Co-authored-by: Elliot Winkler <[email protected]>
| @@ -0,0 +1,15 @@ | |||
| # `@metamask/remote-feature-flag-controller` | |||
|
|
|||
| Controller with caching, fallback, and privacy for managing feature flags via ClientConfigAPI. | |||
There was a problem hiding this comment.
This sentence is a little confusing, maybe we can re-word it to be more clear. I'm not sure what it means.
There was a problem hiding this comment.
The description in the JSDoc entry for the class seems better: "manages the retrieval and caching of remote feature flags"
| { | ||
| "name": "@metamask/remote-feature-flag-controller", | ||
| "version": "0.0.0", | ||
| "description": "Controller with caching, fallback, and privacy for managing feature flags via ClientConfigAPI", |
There was a problem hiding this comment.
Similar to the README, I don't find this to be a clear description
| "type": "git", | ||
| "url": "https://github.com/MetaMask/core.git" | ||
| }, | ||
| "license": "MIT", |
There was a problem hiding this comment.
The preference for new open source packages is to use a dual MIT and Apache 2.0 license. See here for an example: https://github.com/MetaMask/design-tokens/blob/f8c5e466964a4709708bf35c3663bce6fa55c049/package.json#L17
And note the two license files
There was a problem hiding this comment.
Updated to use both licenses in 2bb0ade
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
| return await this.#inProgressFlagUpdate; | ||
| } | ||
|
|
||
| const { promise, resolve, reject } = createDeferredPromise<FeatureFlags>({ |
There was a problem hiding this comment.
What is the purpose of using a deferred Promise here? We appear to want a promise representing the "fetch feature flags" call, but we already have that (it's returned by clientConfigApiService.fetchRemoteFeatureFlag).
We could do something like this instead:
this.#inProgressFlagUpdate = this.#clientConfigApiService.fetchRemoteFeatureFlags();
const serverData = await this.#inProgressFlagUpdate;
this.#inProgressFlagUpdate = undefined;
No deferred promise needed.
There was a problem hiding this comment.
Oh sorry, to handle the failure case:
this.#inProgressFlagUpdate = this.#clientConfigApiService.fetchRemoteFeatureFlags();
let serverData;
try {
serverData = await this.#inProgressFlagUpdate;
} finally {
this.#inProgressFlagUpdate = undefined;
}
| */ | ||
| async getRemoteFeatureFlag(): Promise<FeatureFlags> { | ||
| if (this.#disabled) { | ||
| return []; |
There was a problem hiding this comment.
Should we return the cached flags here instead? That wouldn't make a network request. The enabled/disabled state is meant to prevent network requests.
it seems odd to "hide" the cached values, since they can easily be retrieved via state instead.
There was a problem hiding this comment.
I agree with returning cached data when disabled is true. It was changed to worked that way in d0b81fa
| throw new Error('Failed to fetch remote feature flags'); | ||
| } | ||
|
|
||
| const data = await response.json(); |
There was a problem hiding this comment.
We should validate that it's an array. We seem to assume that without verification.
|
|
||
| /** Type representing the feature flags collection */ | ||
| export type FeatureFlag = { | ||
| [key: string]: Json; |
There was a problem hiding this comment.
How do you envision this would be used? i.e. if I wanted to look for a specific feature flag, how would I find it? Is there a name property I'd search for, and if so, why not specify it here so it can be more easily discovered?
There was a problem hiding this comment.
It would probably be best if feature flags was an object keyed by feature flag names
There was a problem hiding this comment.
but I see that it is an array everywhere, including the ADR, so perhaps best to leave that as is for now
There was a problem hiding this comment.
Proposed approach to add a name property to each feature flag in state is here 8a8be11
…atureFlags (plural) and clean up a couple of tests
danjm
force-pushed
the
feature/27254-feature-flag-controller
branch
from
889d359
to
6c4af68
Compare
Explanation
Following the ADR here
Adds a new controller,
remote-feature-flag-controllerthat fetches the remote feature flags and provide cache solution for consumers.References
Related to #27254
Changelog
@metamask/remote-feature-flag-controllerADDED: Initial release
Checklist