Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Add Feature Flags #1500

Merged
merged 6 commits into from
Feb 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .distignore
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ wp-cli.local.yml
yarn.lock
tests
vendor
config
node_modules
*.sql
*.tar.gz
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ build-style
languages/*
!languages/README.md
wc-admin.zip
includes/feature-config.php

# Directories/files that may appear in your environment
.DS_Store
Expand Down
1 change: 1 addition & 0 deletions bin/build-plugin-zip.sh
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ fi

# Run the build.
status "Generating build... 👷‍♀️"
npm run build:feature-config
npm run build
npm run docs

Expand Down
28 changes: 28 additions & 0 deletions bin/generate-feature-config.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php
/**
* Generates an array of feature flags, based on the config used by the client application.
*
* @package WooCommerce Admin
*/

$phase = isset( $_SERVER['WC_ADMIN_PHASE'] ) ? $_SERVER['WC_ADMIN_PHASE'] : ''; // WPCS: sanitization ok.
if ( ! in_array( $phase, array( 'development', 'plugin', 'core' ) ) ) {
$phase = 'core';
}
$config_json = file_get_contents( 'config/' . $phase . '.json' );
$config = json_decode( $config_json );

$write = "<?php\n";
$write .= "// WARNING: Do not directly edit this file.\n";
$write .= "// This file is auto-generated as part of the build process and things may break.\n";
$write .= "function wc_admin_get_feature_config() {\n";
$write .= "\treturn array(\n";
foreach ( $config->features as $feature => $bool ) {
$write .= "\t\t'{$feature}' => " . ( $bool ? 'true' : 'false' ) . ",\n";
}
$write .= "\t);\n";
$write .= "}\n";

$config_file = fopen( 'includes/feature-config.php', 'w' );
fwrite( $config_file, $write );
fclose( $config_file );
6 changes: 3 additions & 3 deletions client/analytics/report/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import withSelect from 'wc-api/with-select';
const REPORTS_FILTER = 'woocommerce-reports-list';

const getReports = () => {
const reports = applyFilters( REPORTS_FILTER, [
const reports = [
{
report: 'revenue',
title: __( 'Revenue', 'wc-admin' ),
Expand Down Expand Up @@ -86,9 +86,9 @@ const getReports = () => {
title: __( 'Downloads', 'wc-admin' ),
component: DownloadsReport,
},
] );
];

return reports;
return applyFilters( REPORTS_FILTER, reports );
};

class Report extends Component {
Expand Down
52 changes: 30 additions & 22 deletions client/layout/controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,44 +21,52 @@ import Dashboard from 'dashboard';
import DevDocs from 'devdocs';

const getPages = () => {
const pages = [
{
const pages = [];

if ( window.wcAdminFeatures.devdocs ) {
pages.push( {
container: DevDocs,
path: '/devdocs',
wpOpenMenu: 'toplevel_page_woocommerce',
wpClosedMenu: 'toplevel_page_wc-admin--analytics-revenue',
} );
pages.push( {
container: DevDocs,
path: '/devdocs/:component',
wpOpenMenu: 'toplevel_page_woocommerce',
wpClosedMenu: 'toplevel_page_wc-admin--analytics-revenue',
} );
}

if ( window.wcAdminFeatures.dashboard ) {
pages.push( {
container: Dashboard,
path: '/',
wpOpenMenu: 'toplevel_page_woocommerce',
wpClosedMenu: 'toplevel_page_wc-admin--analytics-revenue',
},
{
} );
}

if ( window.wcAdminFeatures.analytics ) {
pages.push( {
container: Analytics,
path: '/analytics',
wpOpenMenu: 'toplevel_page_wc-admin--analytics-revenue',
wpClosedMenu: 'toplevel_page_woocommerce',
},
{
} );
pages.push( {
container: AnalyticsSettings,
path: '/analytics/settings',
wpOpenMenu: 'toplevel_page_wc-admin--analytics-revenue',
wpClosedMenu: 'toplevel_page_woocommerce',
},
{
} );
pages.push( {
container: AnalyticsReport,
path: '/analytics/:report',
wpOpenMenu: 'toplevel_page_wc-admin--analytics-revenue',
wpClosedMenu: 'toplevel_page_woocommerce',
},
{
container: DevDocs,
path: '/devdocs',
wpOpenMenu: 'toplevel_page_woocommerce',
wpClosedMenu: 'toplevel_page_wc-admin--analytics-revenue',
},
{
container: DevDocs,
path: '/devdocs/:component',
wpOpenMenu: 'toplevel_page_woocommerce',
wpClosedMenu: 'toplevel_page_wc-admin--analytics-revenue',
},
];
} );
}

return pages;
};
Expand Down
3 changes: 1 addition & 2 deletions client/layout/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,7 @@ class Layout extends Component {
const { isEmbeded, ...restProps } = this.props;
return (
<div className="woocommerce-layout">
<Slot name="header" />

{ window.wcAdminFeatures[ 'activity-panels' ] && <Slot name="header" /> }
<div className="woocommerce-layout__primary" id="woocommerce-layout__primary">
<Notices />
{ ! isEmbeded && (
Expand Down
4 changes: 4 additions & 0 deletions client/layout/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
}
}

.woocommerce-feature-disabled-activity-panels .woocommerce-layout__primary {
margin-top: 20px;
}

.woocommerce-layout .woocommerce-layout__main {
padding-right: $fallback-gutter-large;
padding-right: $gutter-large;
Expand Down
8 changes: 8 additions & 0 deletions config/core.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"features": {
"activity-panels": false,
"analytics": false,
"dashboard": false,
"devdocs": false
}
}
8 changes: 8 additions & 0 deletions config/development.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"features": {
"activity-panels": true,
"analytics": true,
"dashboard": true,
"devdocs": true
}
}
8 changes: 8 additions & 0 deletions config/plugin.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"features": {
"activity-panels": false,
"analytics": true,
"dashboard": true,
"devdocs": false
}
}
1 change: 1 addition & 0 deletions docs/_sidebar.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
* [Overview](/)
* [Components](components/)
* [Feature Flags](feature-flags)
* [Data](data)
* [Documentation](documentation)
* [Layout](layout)
Expand Down
47 changes: 47 additions & 0 deletions docs/feature-flags.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# Feature Flags

Features inside the `wc-admin` repository can be in various states of completeness. In addition to the development copy of `wc-admin`, feature plugin versions are bundled, and code is merged to WooCommerce core. To provide a way for improved control over how these features are released in these different environments, `wc-admin` has a system for feature flags.

We currently support the following environments:

| Environment | Description |
|-------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| development | Development - All features should be enabled in development. These flags are also used in both JS and PHP tests. Ran using `npm start`. |
| plugin | Plugin - A packaged release of the featured plugin, for GitHub WordPress.org. Ran using `npm run-script build:release`. | |
| core | Core - assets/files ready and stable enough for core merge. @todo No build process exists yet.


## 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.
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Please add new feature flags alphabetically so they are easy to find.

## Basic Use - Client

The `window.wcAdminFeatures` constant is a global variable containing the feature flags.

Instances of `window.wcAdminFeatures` are replaced during the webpack build process by using webpack's [define plugin](https://webpack.js.org/plugins/define-plugin/). Using `webpack` for this allows us to eliminate dead code when making minified/production builds (`plugin`, or `core` environments).

To check if a feature is enabled, you can simplify check the boolean value of a feature:

```
{ window.wcAdminFeatures[ 'activity-panels' ] && <ActivityPanel /> }
Copy link
Contributor

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

```

We also expose CSS classes on the `body` tag, so that you can target specific feature states:

```
<body class="wp-admin woocommerce-page woocommerce-feature-disabled-analytics woocommerce-feature-enabled-activity-panels ....">
```

## Basic Use - Server

Feature flags are also available via PHP. To ensure these are consistent with the built client assets, `includes/feature-flags.php` is generated by the plugin build process or `npm start`. Do not edit `includes/feature-flags.php` directly.

To check if a feature is enabled, you can use the `wc_admin_is_feature_enabled()`:

```
if ( wc_admin_is_feature_enabled( 'activity-panels' ) ) {
Copy link
Contributor

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()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add_action( 'admin_header', 'wc_admin_activity_panel' );
}
```
Loading