feat(27255): allow local modification for remote feature flags #29696
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
@metamaskbot update-policies |
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
|
Policy update failed. You can review the logs or retry the policy update here |
DDDDDanica
force-pushed
the
feature/remote-feature-flags-manifest-adaption
branch
2 times, most recently
from
1e9217f to
1905574
Compare
|
@metamaskbot update-policies |
|
Policy update failed. You can review the logs or retry the policy update here |
DDDDDanica
force-pushed
the
feature/remote-feature-flags-manifest-adaption
branch
from
1905574 to
ced7bf4
Compare
|
@metamaskbot update-policies |
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
DDDDDanica
force-pushed
the
feature/remote-feature-flags-manifest-adaption
branch
5 times, most recently
from
704d4ab to
511c2d5
Compare
|
@metamaskbot update-policies |
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
DDDDDanica
force-pushed
the
feature/remote-feature-flags-manifest-adaption
branch
from
6ccfac6 to
804e3d7
Compare
|
@metamaskbot update-policies |
Builds ready [804e3d7]
Page Load Metrics (1723 ± 60 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
Builds ready [16c667e]
Page Load Metrics (1860 ± 60 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
Walk through the PR changes from here - |
| /** | ||
| * Feature flags to control business logic behavior | ||
| */ | ||
| remoteFeatureFlags?: { |
There was a problem hiding this comment.
- Include the type of
remoteFeatureFlagsin manifest as optional value. We includetestFlagForThresholdthat's created for testing purpose in remote feature flag API .
development/build/manifest.js
Outdated
| @@ -11,6 +11,7 @@ const baradDurManifest = isManifestV3 | |||
| ? require('../../app/manifest/v3/_barad_dur.json') | |||
| : require('../../app/manifest/v2/_barad_dur.json'); | |||
| const { loadBuildTypesConfig } = require('../lib/build-type'); | |||
| const manifestFlags = require('../../manifest-flags.json'); | |||
There was a problem hiding this comment.
- Read customized manifestFlags from
manifest-flags.jsonso we can override the one from controller (API) - old build system
| @@ -1,3 +1,5 @@ | |||
| import manifestFlags from '../../../../../manifest-flags.json'; | |||
There was a problem hiding this comment.
- Read customized manifestFlags from
manifest-flags.jsonso we can override the one from controller (API) - webpack build
development/build/manifest.js
Outdated
| @@ -47,8 +48,10 @@ function createManifestTasks({ | |||
| browserVersionMap[platform], | |||
| await getBuildModifications(buildType, platform), | |||
| customArrayMerge, | |||
| { | |||
There was a problem hiding this comment.
- Add customized local
manifestFlagsto manifest file (old build system)
Builds ready [8e88e7f]
Page Load Metrics (1748 ± 112 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
development/build/manifest.js
Outdated
| async function loadManifestFlags() { | ||
| try { | ||
| return JSON.parse( | ||
| await fs.readFile( | ||
| path.join(__dirname, '../../.manifest-flags.json'), | ||
| 'utf8', | ||
| ), | ||
| ); | ||
| } catch (error) { | ||
| return { remoteFeatureFlags: {} }; | ||
| } | ||
| } | ||
|
|
||
| // Initialize with default value | ||
| let manifestFlags = { remoteFeatureFlags: {} }; | ||
|
|
||
| // Load flags asynchronously | ||
| loadManifestFlags().then((flags) => { | ||
| manifestFlags = flags; | ||
| }); | ||
|
|
||
| module.exports = createManifestTasks; | ||
|
|
There was a problem hiding this comment.
While I don't think the race condition this introduces will ever show its face because the rest of the build process is slow, it's still adding a race condition, which generally makes me uncomfortable. It also hides parsing errors the dev may have made in their .manifest-flags.json file. I think reading synchronously is fine.
There was a problem hiding this comment.
Thanks for the suggestion, addressed 3a16019
| * @param browserManifest - The Chrome extension manifest object to modify | ||
| */ | ||
| function addManifestFlags(browserManifest: chrome.runtime.Manifest) { | ||
| browserManifest._flags = manifestFlags; |
There was a problem hiding this comment.
Instead of reading in the .manifest-flags.json file at the top level scope, I think it's best to just try to read it synchronously here.
Also, hiding JSON.parse errors from the end user by catching them will hide formatting errors from the developer, which I don't think they'd like. Probably better to only try/catch the attempt to readFileSync, and then only ignore the caught error if the file does not exist (the error.code property will be "ENOENT"); all other errors readFileSync can throw the developer will probably need to know about.
There was a problem hiding this comment.
Add sync reading + error catching here a5ea54f
| testFlagForThreshold: { | ||
| name: string; | ||
| value: string; | ||
| }; |
There was a problem hiding this comment.
can we use better types here? maybe something like testFlagForThreshold: Record<string, any> ( or maybe unknown instead of any, but that may complicate some things).
Or if you really want to go the extra mile, you can define the type as any valid JSON:
type JSONValue =
| string
| number
| boolean
| null
| JSONValue[]
| { [key: string]: JSONValue };
There was a problem hiding this comment.
For this one, the name and value are the same in request, and controller will choose one of the value to return based on threshold => https://client-config.api.cx.metamask.io/v1/flags?client=extension&distribution=main&environment=dev
There was a problem hiding this comment.
ah, so it's an array of name, value, and scope properties? I think this type should reflect that
ui/selectors/selectors.js
Outdated
| * Otherwise returns the remote feature flags from the MetaMask state that's retrieved from controller. | ||
| * | ||
| * @param {object} state - The MetaMask state object | ||
| * @returns {object} The remote feature flags object containing feature flag key-value pairs |
There was a problem hiding this comment.
The changes you linked to is not in the current HEAD of the branch
ui/selectors/selectors.js
Outdated
| * This allows for both static (manifest) and dynamic (state) feature flag configuration. | ||
| * | ||
| * @param {object} state - The MetaMask state object | ||
| * @returns {object} Combined feature flags object with manifest flags taking precedence over state flags |
.manifest-flags.json.dist
Outdated
| @@ -0,0 +1,5 @@ | |||
| // This configuration file is used to manage values passing to _flags that will be injected into manifest.json | |||
There was a problem hiding this comment.
I had left a long comment on my previous review about how I do not like the idea of adding yet another build config option to our already complex and long list of build config options, but I can't find it in my previous review, maybe due to force pushes deleting the file I had previously left the comment on?
Anyway, I'll try to recreate it again now:
From the top of my head we have the following build-time config options: build.yml, metamaskrc, metamaskrc.dist, CLI options, ENV options, webpack --config, there is a PR that adds a test-time fixtures config option, manifestOverrides file in build.yml (e.g.,: ./app/build-types/beta/manifest/), and probably even more.
Unfortunately the webpack build doesn't currently read the manifestOverrides property to parse those overrides, but if we could prioritize getting that working in webpack (cc @desi, @danjm, @itsyoboieltr , @gauthierpetetin) that may be the best place to put this new config option. Related issue: #26260
I should note that in the issue linked about I actually explain why I don't like all these config options:
The reason this hasn't been done yet is because I think we should go about it differently. It seems that we sometimes use manifest.json overrides files for customizing fields, sometimes we use build.yml fields, and sometimes we use environment variables. Each way of customization doesn't always overlap with the other ways, but sometimes it does, I think? It's very confusing.
I'd like to eliminate one of these ways of customization in the primary build system, with the webpack build in mind, and then implement it in webpack.
Note: I'm not going to block the PR on this, though I suspect that if this does get merged with this new build config file (.manifest-flags.json) we'll never go back to try to simplify it, and we'll be forever burdened by an even more confusing and complex build configurations than we already have now.
Actually, another potential option I just thought of is to add a field to .metamaskrc:
; A JSON config file that can be used to override the default manifest values.
; e.g., `.manifest-overrides.json` where the contents might be something like:
; {
; "_flags": {
; "remoteFeatureFlags": { }
; }
; }
; Note: properties are shallow merged into the manifest.json file.
MANIFEST_OVERRIDES=.manifest-overrides.jsonIf we do it this way we wouldn't have to add an extra "noisy" config files to the top level directory, developers could have multiple config files they could switch between just by updating the MANIFEST_OVERRIDES property, developers can modify any manifest.json property (not just _flags), and it doesn't further complicate where build config information comes from.
There was a problem hiding this comment.
Yeah, I would also like to reduce the number of configuration methods.
Danica and I discussed putting JSON directly in .metamaskrc, but the major disadvantage, as we discovered in a pair programming session, is that it's not easy to write JSON in that file. This MANIFEST_OVERRIDES is an interesting approach.
I agree that we should be able to override anything in the manifest.
And I would love to get rid of .metamaskrc entirely.
Last thought here... maybe we should use a .js file instead of a .json file, mostly for comments support.
There was a problem hiding this comment.
I wouldn't go to JS for this (unless we have a linting rule that disallows actual JS in it :-) ). I thought about using JSON with comments as well. We could parse with JSON5 which supports comments, but won't get us in the situation where our config files are Turing Complete 😓 (which isn't always horrible, but can end in madness). If we do switch to JSON5 (or any "JSON with comments" flavor), we'll need to be sure we update our .gitattributes file so CSVode/IDEs and github know how to highlight the comment lines correctly.
There was a problem hiding this comment.
I would say that recently it has become the standard for js libraries and applications to use .(js|ts) for configuration.
- prettier.config.(js|ts)
- Fun fact, I implemented
prettier.config.tsin Add support for TS config files prettier/prettier#16828 :D
- Fun fact, I implemented
- eslint.config.(js|ts)
- jest.config.(js|ts)
- drizzle.config.(js|ts)
I would propose something like:
- metamask.config.(js|ts)
Similarly to the drizzle config linked above, it would only have one export default (nothing else), and would look something like:
import { defineConfig } from "metamask-config";
export default defineConfig({
// you can put comments here for context
build: "beta",
// typescript types and jsdoc will further help understanding, so likely comments are not even needed
out: "./dist",
...
});What do you guys think about this approach? @davidmurdoch @HowardBraham
There was a problem hiding this comment.
@itsyoboieltr I would vote for this if the goal is to eventually replace .metamaskrc with metamask.config.ts
There was a problem hiding this comment.
Well, not environment variables, but the properties included in the object returned by browser.runtime.getManifest()
There was a problem hiding this comment.
David previously wrote:
Actually, another potential option I just thought of is to add a field to .metamaskrc:
; A JSON config file that can be used to override the default manifest values.
; e.g.,.manifest-overrides.jsonwhere the contents might be something like:
; {
; "_flags": {
; "remoteFeatureFlags": { }
; }
; }
; Note: properties are shallow merged into the manifest.json file.
MANIFEST_OVERRIDES=.manifest-overrides.json
If we do it this way we wouldn't have to add an extra "noisy" config files to the top level directory, developers could have multiple config files they could switch between just by updating the MANIFEST_OVERRIDES property, developers can modify any manifest.json property (not just _flags), and it doesn't further complicate where build config information comes from.
This seems good to me. Is anyone opposed to this?
There was a problem hiding this comment.
Yes, to modify "the properties included in the object returned by browser.runtime.getManifest()"
I'm okay with the MANIFEST_OVERRIDES=.manifest-overrides.json proposal, but I would like to (in the longer-term) move to a model where .metamaskrc:
- contains only secrets and no configuration
- never gives errors like
Tried to modify a variable "INFURA_PROD_PROJECT_ID" that wasn't declared in builds.yml
There was a problem hiding this comment.
Thank you all for your valuable comments and suggestions ❤️.
I agree with David's suggestion that we need to streamline how manifest overrides are handled, where developers could manage overrides in a more centralized and simpler manner.
This is what I believe is a good approach to implement the customized way to overwrite manifest.json:
- Add a template (.manifest-overrides.json) to add customized values for development.
- Read these values from
MANIFEST_OVERRIDES=.manifest-overrides.jsonin the env file.metamaskrc. - Read customized values from the manifest file during the building process.
- Allow modification of the manifest.json file in the dist directory directly after the build process as well.
- To that end, introduce a selector mechanism to read from the
manifest.jsonfile and apply overrides from the new manifestFlags system.
There was a problem hiding this comment.
Addressed in f3c8e38
…st and revert changes on this test
Builds ready [b2b5862]
Page Load Metrics (1726 ± 111 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
README.md
Outdated
| @@ -56,6 +56,17 @@ If you are not a MetaMask Internal Developer, or are otherwise developing on a f | |||
| - If debugging MetaMetrics, you'll need to add a value for `SEGMENT_WRITE_KEY` [Segment write key](https://segment.com/docs/connections/find-writekey/), see [Developing on MetaMask - Segment](./development/README.md#segment). | |||
| - If debugging unhandled exceptions, you'll need to add a value for `SENTRY_DSN` [Sentry Dsn](https://docs.sentry.io/product/sentry-basics/dsn-explainer/), see [Developing on MetaMask - Sentry](./development/README.md#sentry). | |||
| - Optionally, replace the `PASSWORD` value with your development wallet password to avoid entering it each time you open the app. | |||
| - Duplicate `manifest-flags.json.dist` within the root and rename it to `manifest-flags.json` by running `cp .manifest-flags.json{.dist,}`. This file is used to add flags to `.manifest.json` build files for the extension. You can add flags to the file to be used in the build process, for example: | |||
There was a problem hiding this comment.
No action required yet, but when we figure out what to call the file, we have to be consistent everywhere. Sometimes in the code and documents it has a dot in front, and sometimes it doesn't.
This is an optional step (not everyone who develops locally has to do this immediately) and that should be specified.
…anifest-flags.json file sync way in webpack build
Builds ready [77ad1e5]
Page Load Metrics (2081 ± 87 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [b09f6a5]
Page Load Metrics (1884 ± 76 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| // Mock fs.readFileSync | ||
| const fs = require('fs'); | ||
| const originalReadFileSync = fs.readFileSync; | ||
| fs.readFileSync = () => JSON.stringify(mockFlags); |
There was a problem hiding this comment.
NIT:
Node's testing library has mocking built it. I think the right way to do this is to create a before block within this new 'manifest flags in development mode' describe block:
// (note: this is untested)
const originalReadFileSync = fs.readFileSync;
before("mock `.manifest-flags.json`", () => {
mock.method(fs, 'readFileSync', (path: string, options: object) => {
// only override reads for `.manifest-flags.json`:
if (path === resolve(__dirname, <the path to the file here>)) {
// mock `.manifest-flags.json`
return JSON.stringify(mockFlags);
}
return originalReadFileSync(path, options);
});
});
after(() => mock.restoreAll());| } | ||
| }); | ||
|
|
||
| it('handles missing manifest flags file', () => { |
There was a problem hiding this comment.
love that you are testing the error conditions! can you also add a test for the case where the fs.readFileSync error is NOT ENOENT?
| let manifestFlags = { remoteFeatureFlags: {} }; | ||
|
|
||
| try { | ||
| const fs = require('fs'); |
There was a problem hiding this comment.
| const fs = require('fs'); | |
| const fs = require('node:fs'); |
node: is the new way of importing node's "stdlib" modules, and its technically faster as node checks for the prefix before all other logic.
There was a problem hiding this comment.
That's very useful to know, added in f3c8e38
development/build/manifest.js
Outdated
| async function loadManifestFlags() { | ||
| try { | ||
| return JSON.parse( | ||
| await fs.readFile( | ||
| path.join(__dirname, '../../.manifest-flags.json'), | ||
| 'utf8', | ||
| ), | ||
| ); | ||
| } catch (error) { | ||
| return { remoteFeatureFlags: {} }; | ||
| } | ||
| } | ||
|
|
||
| const manifestFlags = loadManifestFlags(); |
There was a problem hiding this comment.
Did you mean to make loadManifestFlags synchronous?
There was a problem hiding this comment.
Does it make sense to use fs.readFileSync and make it synchronous anyway?
Builds ready [51b6de6]
Page Load Metrics (1598 ± 72 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…d from .dist; refactor fs to stdlib modules
| @@ -56,6 +56,8 @@ If you are not a MetaMask Internal Developer, or are otherwise developing on a f | |||
| - If debugging MetaMetrics, you'll need to add a value for `SEGMENT_WRITE_KEY` [Segment write key](https://segment.com/docs/connections/find-writekey/), see [Developing on MetaMask - Segment](./development/README.md#segment). | |||
| - If debugging unhandled exceptions, you'll need to add a value for `SENTRY_DSN` [Sentry Dsn](https://docs.sentry.io/product/sentry-basics/dsn-explainer/), see [Developing on MetaMask - Sentry](./development/README.md#sentry). | |||
| - Optionally, replace the `PASSWORD` value with your development wallet password to avoid entering it each time you open the app. | |||
| - If developing with remote feature flags and you want to override the flags in the build process, you can add a `.manifest-overrides.json` file to the root of the project and set `MANIFEST_OVERRIDES=.manifest-overrides.json` in `.metamaskrc` to the path of the file. This file is used to add flags to `manifest.json` build files for the extension. You can also modify the `_flags.remoteFeatureFlags` in `manifest.json` file to tweak the flags after the build process. | |||
There was a problem hiding this comment.
Trying to make the instructions slightly clearer
| - If developing with remote feature flags and you want to override the flags in the build process, you can add a `.manifest-overrides.json` file to the root of the project and set `MANIFEST_OVERRIDES=.manifest-overrides.json` in `.metamaskrc` to the path of the file. This file is used to add flags to `manifest.json` build files for the extension. You can also modify the `_flags.remoteFeatureFlags` in `manifest.json` file to tweak the flags after the build process. | |
| - If developing with remote feature flags and you want to override the flags in the build process, you can add a `.manifest-overrides.json` file to the root of the project and set `MANIFEST_OVERRIDES=.manifest-overrides.json` in `.metamaskrc` to the path of the file. This file is used to add flags to `manifest.json` build files for the extension. You can also modify the `_flags.remoteFeatureFlags` in the built version of `manifest.json` in the `dist/browser` folder to tweak the flags after the build process (these changes will get overwritten when you build again). |
| @@ -52,6 +52,9 @@ notes.txt | |||
| .metamaskrc | |||
| .metamaskprodrc | |||
|
|
|||
| # Customized manifest configuration | |||
| .manifest-overrides.json | |||
There was a problem hiding this comment.
I would suggest ignoring .manifest-overrides*.json
Because remember we want people to be able to create a few of these and toggle between them, plus be able to put them in folders, and gitignore all of these.
Description
Note
You can now follow the steps here to review the PR.
Local Feature Flag Override System
This PR implements a system that allows developers to override remote feature flags locally through
manifest.json, providing more flexibility in development and testing environments.Key Features
Local Feature Flag Override
remoteFeatureFlagvalues by defining them inmanifest.jsonTesting Integration
remoteFeatureFlagobjects to be passed within testwithFixturesDeveloper Validation
This enhancement streamlines the development workflow by providing local control over feature flags without requiring changes to the controller or deployment configurations.
Usage Example
A. Local build
remoteFeatureFlagsinmanifest.json:B. e2e test
Add the customized value in
Related issues
Fixes: #27255
Manual testing steps
Please check <### Usage Example> above to check how to test manually.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist