Skip to content

Commit 5a84737

Browse files
DDDDDanicametamaskbotHowardBrahamdavidmurdoch
authored
feat(27255): allow local modification for remote feature flags (#29696)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** > [!Note] > You can now follow the steps [here](#29696 (comment)) 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 1. **Local Feature Flag Override** - Developers can override `remoteFeatureFlag` values by defining them in `.manifest-overrides.json` - These overrides take precedence over values received from the controller - Accessible in all development environments: - Local builds - Webpack builds - E2E tests 2. **Testing Integration** - Leverages the manifest system introduced in [PR #26588](#26588) - Allows custom `remoteFeatureFlag` objects to be passed within test `withFixtures` - Simplifies feature flag testing scenarios 3. **Developer Validation** - Override values can be verified through the developer options panel - Provides immediate feedback on applied feature flag settings 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 1. Define overrides in `remoteFeatureFlags` in `.manifest-overrides.json`: ``` { "_flags": { "remoteFeatureFlags": { "testFlagForThreshold": { "name": "test-flag", "value": "121212" } } } } ``` 2. Verify in Developer Options: - Open extension - Click on the account menu (top-right corner) - Select "Settings" > "Developer Options" - Look for "Remote Feature Flags" section to verify your overrides <img width="1026" alt="Screenshot 2025-01-14 at 00 48 22" src="https://github.com/user-attachments/assets/76c39050-0a5f-4079-8d0a-266f7ccee9a9" /> ### B. e2e test Add the customized value in ``` fixtures: new FixtureBuilder() .withMetaMetricsController({ metaMetricsId: MOCK_META_METRICS_ID, participateInMetaMetrics: true, }) .build(), ``` <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 4. What is the improvement/solution? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29696?quickstart=1) ## **Related issues** Fixes: #27255 ## **Manual testing steps** Please check <### Usage Example> above to check how to test manually. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: MetaMask Bot <[email protected]> Co-authored-by: Howard Braham <[email protected]> Co-authored-by: David Murdoch <[email protected]>
1 parent eaa9b51 commit 5a84737

33 files changed

+577
-72
lines changed

.gitignore

+3
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,9 @@ notes.txt
5252
.metamaskrc
5353
.metamaskprodrc
5454

55+
# Customized manifest configuration
56+
.manifest-overrides*.json
57+
5558
# Test results
5659
test-results/
5760

.metamaskrc.dist

+10
Original file line numberDiff line numberDiff line change
@@ -50,3 +50,13 @@ BLOCKAID_PUBLIC_KEY=
5050
; API key used in Etherscan requests to prevent rate limiting.
5151
; Only applies to Mainnet and Sepolia.
5252
; ETHERSCAN_API_KEY=
53+
54+
; A JSON config file that can be used to override the default manifest values.
55+
; e.g., `.manifest-overrides.json` where the contents might be something like:
56+
; {
57+
; "_flags": {
58+
; "remoteFeatureFlags": { }
59+
; }
60+
; }
61+
; Note: Properties are shallow merged into the manifest.json file at build time.
62+
;MANIFEST_OVERRIDES=.manifest-overrides.json

README.md

+12
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,18 @@ If you are not a MetaMask Internal Developer, or are otherwise developing on a f
5656
- 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).
5757
- 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).
5858
- Optionally, replace the `PASSWORD` value with your development wallet password to avoid entering it each time you open the app.
59+
- 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.
60+
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).
61+
An example of this remote feature flag overwrite could be:
62+
63+
```json
64+
{
65+
"_flags": {
66+
"remoteFeatureFlags": { "testBooleanFlag": false }
67+
}
68+
}
69+
```
70+
5971
- Run `yarn install` to install the dependencies.
6072
- Build the project to the `./dist/` folder with `yarn dist` (for Chromium-based browsers) or `yarn dist:mv2` (for Firefox)
6173

app/scripts/lib/setupSentry.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ import * as Sentry from '@sentry/browser';
33
import { logger } from '@sentry/utils';
44
import browser from 'webextension-polyfill';
55
import { isManifestV3 } from '../../../shared/modules/mv3.utils';
6+
import { getManifestFlags } from '../../../shared/lib/manifestFlags';
67
import extractEthjsErrorMessage from './extractEthjsErrorMessage';
7-
import { getManifestFlags } from './manifestFlags';
88
import { filterEvents } from './sentry-filter-events';
99

1010
const projectLogger = createProjectLogger('sentry');

builds.yml

+2
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,8 @@ env:
303303

304304
# Uses yaml anchors to DRY - https://juju.is/docs/sdk/yaml-anchors-and-aliases
305305
- METAMASK_BUILD_TYPE_DEFAULT: *default
306+
# Path to a JSON file that will be used to override the default manifest values.
307+
- MANIFEST_OVERRIDES: null
306308

307309
###
308310
# Account Abstraction (EIP-4337)

development/build/config.js

+1
Original file line numberDiff line numberDiff line change
@@ -160,4 +160,5 @@ async function getConfig(buildType, environment) {
160160

161161
module.exports = {
162162
getConfig,
163+
fromIniFile,
163164
};

development/build/manifest.js

+31-1
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,35 @@ const { loadBuildTypesConfig } = require('../lib/build-type');
1515
const { TASKS, ENVIRONMENT } = require('./constants');
1616
const { createTask, composeSeries } = require('./task');
1717
const { getEnvironment, getBuildName } = require('./utils');
18+
const { fromIniFile } = require('./config');
1819

1920
module.exports = createManifestTasks;
2021

22+
async function loadManifestFlags() {
23+
const { definitions } = await fromIniFile(
24+
path.resolve(__dirname, '..', '..', '.metamaskrc'),
25+
);
26+
const manifestOverridesPath = definitions.get('MANIFEST_OVERRIDES');
27+
// default to undefined so that the manifest plugin can check if it was set
28+
let manifestFlags;
29+
if (manifestOverridesPath) {
30+
try {
31+
manifestFlags = await readJson(
32+
path.resolve(process.cwd(), manifestOverridesPath),
33+
);
34+
} catch (error) {
35+
// Only throw if error is not ENOENT (file not found) and manifestOverridesPath was provided
36+
if (error.code === 'ENOENT') {
37+
throw new Error(
38+
`Manifest override file not found: ${manifestOverridesPath}`,
39+
);
40+
}
41+
}
42+
}
43+
44+
return manifestFlags;
45+
}
46+
2147
function createManifestTasks({
2248
browserPlatforms,
2349
browserVersionMap,
@@ -28,6 +54,9 @@ function createManifestTasks({
2854
}) {
2955
// merge base manifest with per-platform manifests
3056
const prepPlatforms = async () => {
57+
const isDevelopment =
58+
getEnvironment({ buildTarget: entryTask }) === 'development';
59+
const manifestFlags = isDevelopment ? await loadManifestFlags() : undefined;
3160
return Promise.all(
3261
browserPlatforms.map(async (platform) => {
3362
const platformModifications = await readJson(
@@ -47,8 +76,9 @@ function createManifestTasks({
4776
browserVersionMap[platform],
4877
await getBuildModifications(buildType, platform),
4978
customArrayMerge,
79+
// Only include _flags if manifestFlags has content
80+
manifestFlags,
5081
);
51-
5282
modifyNameAndDescForNonProd(result);
5383

5484
const dir = path.join('.', 'dist', platform);

development/lib/get-manifest-flag.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { exec as callbackExec } from 'node:child_process';
66
import { hasProperty } from '@metamask/utils';
77
import { merge } from 'lodash';
88

9-
import type { ManifestFlags } from '../../app/scripts/lib/manifestFlags';
9+
import type { ManifestFlags } from '../../shared/lib/manifestFlags';
1010

1111
const exec = promisify(callbackExec);
1212
const PR_BODY_FILEPATH = path.resolve(

development/webpack/test/plugins.ManifestPlugin.test.ts

+116-2
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, it } from 'node:test';
1+
import { describe, it, afterEach } from 'node:test';
22
import assert from 'node:assert';
33
import { join } from 'node:path';
44
import { type Compilation } from 'webpack';
@@ -232,7 +232,7 @@ describe('ManifestPlugin', () => {
232232
function runTest(baseManifest: Combination<typeof manifestMatrix>) {
233233
const manifest = baseManifest as unknown as chrome.runtime.Manifest;
234234
const hasTabsPermission = (manifest.permissions || []).includes('tabs');
235-
const transform = transformManifest(args);
235+
const transform = transformManifest(args, false);
236236

237237
if (args.test && hasTabsPermission) {
238238
it("throws in test mode when manifest already contains 'tabs' permission", () => {
@@ -281,4 +281,118 @@ describe('ManifestPlugin', () => {
281281
}
282282
}
283283
});
284+
285+
describe('manifest flags in development mode', () => {
286+
const emptyTestManifest = {} as chrome.runtime.Manifest;
287+
const notEmptyTestManifest = {
288+
_flags: { remoteFeatureFlags: { testFlag: false, testFlag2: 'value1' } },
289+
} as unknown as chrome.runtime.Manifest;
290+
const mockFlags = { _flags: { remoteFeatureFlags: { testFlag: true } } };
291+
const manifestOverridesPath = 'testManifestOverridesPath.json';
292+
const fs = require('node:fs');
293+
const { mock } = require('node:test');
294+
const { resolve } = require('node:path');
295+
296+
afterEach(() => mock.restoreAll());
297+
298+
it('adds manifest flags in development mode with path provided and empty manifest', () => {
299+
mock.method(fs, 'readFileSync', (path: string, options: object) => {
300+
if (path === resolve(__dirname, '../../../', manifestOverridesPath)) {
301+
return JSON.stringify(mockFlags);
302+
}
303+
return fs.readFileSync.original(path, options);
304+
});
305+
const transform = transformManifest(
306+
{ lockdown: true, test: false },
307+
true,
308+
manifestOverridesPath,
309+
);
310+
assert(transform, 'transform should be truthy');
311+
312+
const transformed = transform(emptyTestManifest, 'chrome');
313+
console.log('Transformed:', transformed);
314+
assert.deepStrictEqual(
315+
transformed,
316+
mockFlags,
317+
'manifest should have flags in development mode',
318+
);
319+
});
320+
321+
it('overwrites existing manifest properties with override values but keeps original properties', () => {
322+
mock.method(fs, 'readFileSync', (path: string, options: object) => {
323+
if (path === resolve(__dirname, '../../../', manifestOverridesPath)) {
324+
return JSON.stringify(mockFlags);
325+
}
326+
return fs.readFileSync.original(path, options);
327+
});
328+
const transform = transformManifest(
329+
{ lockdown: true, test: false },
330+
true,
331+
manifestOverridesPath,
332+
);
333+
assert(transform, 'transform should be truthy');
334+
335+
const transformed = transform(notEmptyTestManifest, 'chrome');
336+
assert.deepStrictEqual(
337+
transformed,
338+
{
339+
_flags: {
340+
remoteFeatureFlags: {
341+
testFlag2: 'value1',
342+
testFlag: true,
343+
},
344+
},
345+
},
346+
'manifest should merge original properties with overrides, with overrides taking precedence',
347+
);
348+
});
349+
350+
it('handles missing manifest flags file with path provided', () => {
351+
mock.method(fs, 'readFileSync', () => {
352+
const error = new Error('File not found') as NodeJS.ErrnoException;
353+
error.code = 'ENOENT';
354+
throw error;
355+
});
356+
357+
const transform = transformManifest(
358+
{ lockdown: true, test: false },
359+
true,
360+
manifestOverridesPath,
361+
);
362+
assert(transform, 'transform should be truthy');
363+
364+
assert.throws(
365+
() => transform(emptyTestManifest, 'chrome'),
366+
{
367+
message: `Manifest override file not found: ${manifestOverridesPath}`,
368+
},
369+
'should throw when manifest override file is not found',
370+
);
371+
});
372+
373+
it('silently ignores non-ENOENT filesystem errors', () => {
374+
const transform = transformManifest(
375+
{ lockdown: true, test: false },
376+
true,
377+
manifestOverridesPath,
378+
);
379+
assert(transform, 'transform should be truthy');
380+
381+
const originalError = new Error(
382+
'Permission denied',
383+
) as NodeJS.ErrnoException;
384+
originalError.code = 'EACCES';
385+
386+
mock.method(fs, 'readFileSync', () => {
387+
throw originalError;
388+
});
389+
390+
const transformed = transform(emptyTestManifest, 'chrome');
391+
assert.deepStrictEqual(
392+
transformed._flags,
393+
undefined,
394+
'should not have flags when file read fails with non-ENOENT error',
395+
);
396+
});
397+
});
284398
});

development/webpack/test/webpack.config.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ ${Object.entries(env)
231231
assert.deepStrictEqual(manifestPlugin.options.description, null);
232232
assert.deepStrictEqual(manifestPlugin.options.zip, true);
233233
assert(manifestPlugin.options.zipOptions, 'Zip options should be present');
234-
assert.strictEqual(manifestPlugin.options.transform, undefined);
234+
assert.deepStrictEqual(manifestPlugin.options.transform, undefined);
235235

236236
const progressPlugin = instance.options.plugins.find(
237237
(plugin) => plugin && plugin.constructor.name === 'ProgressPlugin',

development/webpack/utils/plugins/ManifestPlugin/helpers.ts

+57-5
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import merge from 'lodash/merge';
12
/**
23
* Returns a function that will transform a manifest JSON object based on the
34
* given build args.
@@ -9,12 +10,20 @@
910
* @param args
1011
* @param args.lockdown
1112
* @param args.test
13+
* @param isDevelopment
14+
* @param manifestOverridesPath
1215
* @returns a function that will transform the manifest JSON object
1316
* @throws an error if the manifest already contains the "tabs" permission and
1417
* `test` is `true`
1518
*/
16-
export function transformManifest(args: { lockdown: boolean; test: boolean }) {
17-
const transforms: ((manifest: chrome.runtime.Manifest) => void)[] = [];
19+
export function transformManifest(
20+
args: { lockdown: boolean; test: boolean },
21+
isDevelopment: boolean,
22+
manifestOverridesPath?: string | undefined,
23+
) {
24+
const transforms: ((
25+
manifest: chrome.runtime.Manifest,
26+
) => chrome.runtime.Manifest | void)[] = [];
1827

1928
function removeLockdown(browserManifest: chrome.runtime.Manifest) {
2029
const mainScripts = browserManifest.content_scripts?.[0];
@@ -29,6 +38,48 @@ export function transformManifest(args: { lockdown: boolean; test: boolean }) {
2938
transforms.push(removeLockdown);
3039
}
3140

41+
/**
42+
* This function sets predefined flags in the manifest's _flags property
43+
* that are stored in the file specified by the `MANIFEST_OVERRIDES` build variable
44+
*
45+
* @param browserManifest - The Chrome extension manifest object to modify
46+
*/
47+
function addManifestFlags(browserManifest: chrome.runtime.Manifest): void {
48+
let manifestFlags;
49+
50+
if (manifestOverridesPath) {
51+
try {
52+
const fs = require('node:fs');
53+
const path = require('node:path');
54+
const manifestFlagsContent = fs.readFileSync(
55+
path.resolve(process.cwd(), manifestOverridesPath),
56+
'utf8',
57+
);
58+
manifestFlags = JSON.parse(manifestFlagsContent);
59+
} catch (error: unknown) {
60+
if (
61+
error instanceof Error &&
62+
'code' in error &&
63+
error.code === 'ENOENT'
64+
) {
65+
// Only throw if ENOENT and manifestOverridesPath was provided
66+
throw new Error(
67+
`Manifest override file not found: ${manifestOverridesPath}`,
68+
);
69+
}
70+
}
71+
}
72+
73+
if (manifestFlags) {
74+
merge(browserManifest, manifestFlags);
75+
}
76+
}
77+
78+
if (isDevelopment) {
79+
// Add manifest flags only for development builds
80+
transforms.push(addManifestFlags);
81+
}
82+
3283
function addTabsPermission(browserManifest: chrome.runtime.Manifest) {
3384
if (browserManifest.permissions) {
3485
if (browserManifest.permissions.includes('tabs')) {
@@ -41,16 +92,17 @@ export function transformManifest(args: { lockdown: boolean; test: boolean }) {
4192
browserManifest.permissions = ['tabs'];
4293
}
4394
}
95+
4496
if (args.test) {
4597
// test builds need "tabs" permission for switchToWindowWithTitle
4698
transforms.push(addTabsPermission);
4799
}
48100

49101
return transforms.length
50102
? (browserManifest: chrome.runtime.Manifest, _browser: string) => {
51-
const clone = structuredClone(browserManifest);
52-
transforms.forEach((transform) => transform(clone));
53-
return clone;
103+
const manifestClone = structuredClone(browserManifest);
104+
transforms.forEach((transform) => transform(manifestClone));
105+
return manifestClone;
54106
}
55107
: undefined;
56108
}

development/webpack/webpack.config.ts

+5-1
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,11 @@ const plugins: WebpackPluginInstance[] = [
131131
version: version.version,
132132
versionName: version.versionName,
133133
browsers: args.browser,
134-
transform: transformManifest(args),
134+
transform: transformManifest(
135+
args,
136+
isDevelopment,
137+
variables.get('MANIFEST_OVERRIDES') as string | undefined,
138+
),
135139
zip: args.zip,
136140
...(args.zip
137141
? {

0 commit comments

Comments
 (0)