This repository has been archived by the owner on Jul 12, 2024. It is now read-only.
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.
Add Feature Flags #1500
Add Feature Flags #1500
Changes from all commits
473439b
ab706a2
f129156
81607dc
e106aaa
45badff
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 think the Calypso feature flags allow you to omit a flag from a config, and if it is not present it is considered that the feature is not enabled for that env. I believe initially you had to be explicit in adding a flag in all configs like it is setup here but changed during a re-write.
I'm fine with making this a requirement myself, but just figured I'd mention the path that happened on Calypso for reference.
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 just checked, and you actually can omit flags and still have it default to the feature being off (undefined). So nothing will break if we forget to do this.
However, I would say let's keep the documentation line here. I like us having these be explicit in each env so you can quickly open the file and see what's up.
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 nice that it works that way. I'm fine with it as-is, just commenting about prior work on the same topic.
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 like the simplicity of just checking the global - but wondering if you considered adding a util method like
isFeatureEnabled
or something along those lines. Probably would result in more code throughout the app, but the only benefit I see there is being able to update the logic in one file vs dozens if/when we need to change it for some reason.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.
@timmyc I definitely considered a utility method here but couldn't quite get it to work 100%. I just spent a bit more time confirming this.
The trade off here basically boils down to webpack's code elimination.
Right now, with this branch, if webpack encountered this line and the flag was turned off, this code would not end up in the plugin/production bundle. I tried a few different methods to try to get it to phase a function, but the code ends up being bundled (even if not displayed to the user).
One potential solution (also raised in the Gutenberg thread -- which I think they decided against) is to write a custom babel plugin that could transform these function calls into the global check before webpack hits it.
I'm personally fine just checking a variable. The extra work would basically be for different syntax since these flags should ever only be boolean values (so the function logic should never really change anyway). What do you think?
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 in-depth explanation - I hadn't considered / wasn't aware of the webpack implications here. Totally okay with it knowing that now.
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.
Building upon the comment above, we could even make the method name the same in JS
wcAdminIsFeatureEnabled()
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.
See #1500 (comment)