Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(27255): allow local modification for remote feature flags #29696
base: main
Are you sure you want to change the base?
feat(27255): allow local modification for remote feature flags #29696
Changes from 11 commits
d4f2f3f
887ed35
4cef487
5e3788b
5c703f2
35fe1f0
473a3bd
caa4b4f
0bff314
d4e79a7
8e88e7f
b1a52ae
b2b5862
a5ea54f
5308bed
2cc1560
248ec01
40f3c07
3a16019
77ad1e5
b09f6a5
9487c17
51b6de6
f3c8e38
2c5ab32
945c42a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: #26260I should note that in the issue linked about I actually explain why I don't like all these config options:
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
: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 anymanifest.json
property (not just_flags
), and it doesn't further complicate where build config information comes from.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. ThisMANIFEST_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that recently it has become the standard for js libraries and applications to use
.(js|ts)
for configuration.prettier.config.ts
in Add support for TS config files prettier/prettier#16828 :DI would propose something like:
Similarly to the drizzle config linked above, it would only have one export default (nothing else), and would look something like:
What do you guys think about this approach? @davidmurdoch @HowardBraham
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itsyoboieltr I would vote for this if the goal is to eventually replace
.metamaskrc
withmetamask.config.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, not environment variables, but the properties included in the object returned by
browser.runtime.getManifest()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
David previously wrote:
This seems good to me. Is anyone opposed to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:Tried to modify a variable "INFURA_PROD_PROJECT_ID" that wasn't declared in builds.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:MANIFEST_OVERRIDES=.manifest-overrides.json
in the env file.metamaskrc
.manifest.json
file and apply overrides from the new manifestFlags system.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in f3c8e38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, addressed 3a16019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manifestFlags
to manifest file (old build system)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manifestFlags
to manifest file (webpack build system)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of reading in the
.manifest-flags.json
file at the top level scope, I think it's best to justtry
to read it synchronously here.Also, hiding
JSON.parse
errors from the end user bycatch
ing them will hide formatting errors from the developer, which I don't think they'd like. Probably better to onlytry
/catch
the attempt toreadFileSync
, and then only ignore the caughterror
if the file does not exist (theerror.code
property will be"ENOENT"
); all other errorsreadFileSync
can throw the developer will probably need to know about.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add sync reading + error catching here a5ea54f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use better types here? maybe something like
testFlagForThreshold: Record<string, any>
( or maybeunknown
instead ofany
, 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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, so it's an array of
name
,value
, andscope
properties? I think this type should reflect thatThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MOCK_CUSTOMIZED_REMOTE_FEATURE_FLAGS