-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add CLI support to set Feature Flag values #9554
Conversation
packages/core/parcel/src/cli.js
Outdated
let [name, val] = value.split('='); | ||
if (name in DEFAULT_FEATURE_FLAGS) { | ||
let featureFlagValue; | ||
console.log(typeof DEFAULT_FEATURE_FLAGS[name]); |
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.
Oops, accidentally left this in.. 🤦
packages/core/parcel/src/cli.js
Outdated
@@ -103,6 +104,30 @@ const commonOptions = { | |||
}, | |||
[], | |||
], | |||
'-F, --feature-flag <name=value>': [ |
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.
Nitpick: Honestly prefer without the -F
shorthand. People aren't going to type this by hand anyway so explicit is better.
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.
Also can you set multiple flags?
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 think it might need to be --feature-flag <name=value...>
as per this?
But the input would be like parcel build --feature-flag foo=true bar=false
whereas I normally prefer parcel build --feature-flag foo=true --feature-flag bar=false
but not sure how easy that would be
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.
The latter is what works out the box with commander
(i.e. parcel build --feature-flag foo=true --feature-flag bar=false
), that was my intention rather than being able to set multiple flags with one option (I would expect most of time time it'll be some sort of launcher script configuring these anyway, and if you're setting a flag by hand it's usually a single one.
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.
Perfect 👍
a21fe91
to
ac4f8f1
Compare
↪️ Pull Request
Follows up on #9551 and adds support to the CLI to pass
featureFlags
intoInitialParcelOptions
.The approach to this is a single
-F
or--feature-flag
flag that takes aname=value
argument. Validation is done to ensure the name is a valid feature flag, andvalue
matches the expected type - at the moment this is eitherstring
orboolean
, though I'd expectboolean
to be the most common.💻 Examples
parcel build --feature-flag makeItFaster=true src/index.js
🚨 Test instructions
Manually tested to ensure that validation and setting the arguments in
InitialParcelOptions
works as expected.✔️ PR Todo