Add Feature Flags #1500

justinshreve · 2019-02-07T20:34:54Z

Closes #1345.

This PR adds feature flags to `wc-admin 🎉.

This allows us to enable/disable features depending on the enviornment. Right now, the environments this supports are development, plugin, and core. Think of development, staging, and production in Calypso for example. You can access the flags from both PHP and JS. The approach I used on the JS side also allows Webpack to do dead code elimination like Gutenberg. For more information on all this, view the documentation that is a part of this PR.

I have based this approach off of a mix of the proposed Gutenberg PR (WordPress/gutenberg#13324) and Calypso's implementation. I prefer checking against flags in the code, instead of a specific "phase" of the plugin.

Some notes/thoughts for the future:

How should this effect documentation (if it at all)? I don't think we will be using these flags in packages (packages have versioning) so I don't think we need to do any pruning of documents just yet... but if we start documenting a feature, we might want to prune that out for "plugin documentation" or "core documentation".
API conditional file loading. I haven't changed any of the API loading in this PR. Some endpoints are used across features. Since these endpoints are also versioned from core, it seems like we can leave loading alone too.
Should we have two separate plugin environments? I imagine at some point, we may be making changes to a released WordPress.org feature plugin, and at the same time might be cutting a GitHub preview release. Should we think about adding a beta environment as well?
I added a core.json config file, but our core merge process is TBD so this is more of a reminder.

Screenshots

Screenshot of devdocs and activity panel disabled in the bundled plugin:

Detailed test instructions:

Review the documentation. Does it match the usage in this PR?
Run npm test and make sure all tests pass.
Run phpunit and make sure all tests pass.
Run npm start (close any current running build, since the commands have changed)
Click around various parts of wc-admin and WooCommerce pages. Functionally, in the development environment, nothing should have changed.
Run npm run-script build:release and build a wc-admin.zip plugin. Upload this to a different test site (wpsandbox for example). Enable wc-admin.
Verify that the activity panel does not display.
Verify that the devdocs does not display.
Verify that you can see the dashboard and analytics pages.

…ags.

joshuatf

So much 🔥 🔥 in this PR. Awesome work, Justin!

Left a few comments in the code, but overall this looks great.

How should this effect documentation (if it at all)?

I agree probably not at all for the time being. I could be wrong, but I don't think that flags should affect packages in any way.

API conditional file loading. I haven't changed any of the API loading in this PR. Some endpoints are used across features. Since these endpoints are also versioned from core, it seems like we can leave loading alone too.

I think it's probably okay as is right now, but I left a comment about a possible issue. We override core endpoints if they use the same namespace we could cause issue if they ever match (but currently we're all v4 and core is using v3).

Should we have two separate plugin environments?

I like the idea of having a beta environment as well. Seems like something we'll use.

client/layout/controller.js

config/plugin.json

lib/admin.php

bin/generate-feature-config.php

webpack.config.js

joshuatf · 2019-02-08T02:51:20Z

lib/admin.php

-		'dashicons-chart-bar',
-		56 // After WooCommerce & Product menu items.
-	);
+	if ( wc_admin_is_feature_enabled( 'analytics' ) ) {


Just thinking out loud here, do we also want to wrap php includes (mostly API files) with this?

I'm thinking it's mostly okay as the API isn't something most users will encounter, but if they're disabling because of a conflict in WC core where our API files override, this might cause an issue.

I'm thinking it's mostly okay

I think so too, and wrapping the endpoints in different checks just might be confusing/more trouble than it is worth. For example, dashboard relies on the same endpoints as analytics, and I think there is some overlap between the activity panel too (adding additional fields to a response for example).

but if they're disabling because of a conflict in WC core where our API files override, this might cause an issue.

Since these are versioned from the core endpoints I think users shouldn't notice an issue, unless they are testing plugin/development, (i.e these are v4 and anything "real" will use v3). As long as we keep bumping the namespace in wc-admin whenever we do a big core merge we should be OK -- So when core hits v4, we should use v5.

timmyc

This is looking great Justin - really think this will be a great addition in core as well to allow us to actively develop JS functionality there ( thinking even around blocks ) and keep things feature flagged. Few comments, but will take this for a proper test tomorrow morning.

client/layout/controller.js

timmyc · 2019-02-08T03:07:36Z

docs/feature-flags.md

+
+## Adding a new flag
+
+Flags can be added to the files located in the `config/` directory. Make sure to add a flag for each environment and explicitly set the flag to false.


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.

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.

Ah nice that it works that way. I'm fine with it as-is, just commenting about prior work on the same topic.

docs/feature-flags.md

timmyc · 2019-02-08T03:09:52Z

docs/feature-flags.md

+To check if a feature is enabled, you can simplify check the boolean value of a feature:
+
+```
+{ window.wcAdminFeatures[ 'activity-panels' ] && <ActivityPanel /> }


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.

@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?

✨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.

timmyc · 2019-02-08T03:10:43Z

docs/feature-flags.md

+To check if a feature is enabled, you can use the `wc_admin_is_feature_enabled()`:
+
+```
+if ( wc_admin_is_feature_enabled( 'activity-panels' ) ) {


Building upon the comment above, we could even make the method name the same in JS wcAdminIsFeatureEnabled()

See #1500 (comment)

lib/admin.php

package.json

timmyc · 2019-02-08T03:15:25Z

Tagging @ryelle for any input here - I could see this being pretty useful in Woo core too for blocks development once everything lands in core.

…o-generated PHP warning.

justinshreve · 2019-02-08T18:29:19Z

I like the idea of having a beta environment as well. Seems like something we'll use.

I added a @todo for this to webpack here: e106aaa

We don't currently have a separate build process for a beta version, but when we do we can easily add a new phase and pass it to the build script.

justinshreve · 2019-02-08T18:37:38Z

@timmyc @joshuatf Thank you both for the feedback here! I've addressed some comments, and answered others above.

Aljullu

This is amazing @justinshreve, good job! 🎉

I added a couple of questions regarding the code, but overall it looks awesome.

bin/generate-feature-config.php

package.json

tests/js/setup-globals.js

webpack.config.js

…mplify test mock, remove empty webpack line.

justinshreve · 2019-02-11T18:31:10Z

Thanks for the review @Aljullu! Your comments have been addressed.

timmyc

Thanks for the updates Justin - just went through the full test instructions and all is working wonderfully! I'd love to get this in, and use the flag'ed build for 0.7.0's release 🎉

justinshreve added 3 commits February 7, 2019 15:12

First pass at adding feature flags. Accessible over PHP and JS.

473439b

Hook up dev config to test enviornments, hook up features to their fl…

ab706a2

…ags.

Cleanup some code and add documentation. Remove beta config.

f129156

justinshreve added [Status] Needs Review tool: monorepo infrastructure type: documentation This issue is a request for better documentation. labels Feb 7, 2019

justinshreve self-assigned this Feb 7, 2019

justinshreve requested a review from a team February 7, 2019 20:34

joshuatf reviewed Feb 8, 2019

View reviewed changes

timmyc reviewed Feb 8, 2019

View reviewed changes

justinshreve added 2 commits February 8, 2019 13:19

Handle PR Feedback: Add devdocs flag, remove categories flag, fix aut…

81607dc

…o-generated PHP warning.

Add todo for beta phase.

e106aaa

Aljullu reviewed Feb 10, 2019

View reviewed changes

bin/generate-feature-config.php Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

tests/js/setup-globals.js Outdated Show resolved Hide resolved

webpack.config.js Outdated Show resolved Hide resolved

Handle PR feedback: Simplify is_array check, update build command, si…

45badff

…mplify test mock, remove empty webpack line.

timmyc approved these changes Feb 12, 2019

View reviewed changes

timmyc added [Status] Ready to Merge and removed [Status] Needs Review labels Feb 12, 2019

justinshreve merged commit 0c4c17e into master Feb 12, 2019

justinshreve deleted the add/feature-flags branch February 12, 2019 20:02

rrennick mentioned this pull request Feb 13, 2019

add build:feature-config to readme instructions #1545

Merged

timmyc mentioned this pull request Feb 20, 2019

Adds StoreAlerts component #1627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Feature Flags #1500

Add Feature Flags #1500

justinshreve commented Feb 7, 2019 •

edited

Loading

joshuatf left a comment

joshuatf Feb 8, 2019

justinshreve Feb 8, 2019

timmyc left a comment

timmyc Feb 8, 2019

justinshreve Feb 8, 2019

timmyc Feb 11, 2019

timmyc Feb 8, 2019

justinshreve Feb 8, 2019

timmyc Feb 11, 2019

timmyc Feb 8, 2019

justinshreve Feb 8, 2019

timmyc commented Feb 8, 2019

justinshreve commented Feb 8, 2019

justinshreve commented Feb 8, 2019

Aljullu left a comment

justinshreve commented Feb 11, 2019

timmyc left a comment


		## Adding a new flag

		Flags can be added to the files located in the `config/` directory. Make sure to add a flag for each environment and explicitly set the flag to false.

Add Feature Flags #1500

Add Feature Flags #1500

Conversation

justinshreve commented Feb 7, 2019 • edited Loading

Screenshots

Detailed test instructions:

joshuatf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmyc commented Feb 8, 2019

justinshreve commented Feb 8, 2019

justinshreve commented Feb 8, 2019

Aljullu left a comment

Choose a reason for hiding this comment

justinshreve commented Feb 11, 2019

timmyc left a comment

Choose a reason for hiding this comment

justinshreve commented Feb 7, 2019 •

edited

Loading