Skip to content
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

Nav Unification: uses experiment to set a feature flag #47527

Merged
merged 8 commits into from
Nov 25, 2020
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
19 changes: 19 additions & 0 deletions client/config/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,25 @@ if ( config.isEnabled( 'myFeature' ) ) {
}
```

### config.enable( key )

Enable a feature.

```js
import config from 'calypso/config';

config.enable( 'myFeature' );
```
### config.disable( key )

Disable a feature.

```js
import config from 'calypso/config';

config.disable( 'myFeature' );
```

The key should always be a literal string not a variable so that down the road
we can process the compiled scripts and remove code for disabled features in
production.
Expand Down
2 changes: 2 additions & 0 deletions client/config/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,5 @@ const configApi = createConfig( configData );
export default configApi;
export const isEnabled = configApi.isEnabled;
export const enabledFeatures = configApi.enabledFeatures;
export const enable = configApi.enable;
export const disable = configApi.disable;
14 changes: 13 additions & 1 deletion client/layout/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ import { getCurrentOAuth2Client } from 'calypso/state/oauth2-clients/ui/selector
import LayoutLoader from './loader';
import wooDnaConfig from 'calypso/jetpack-connect/woo-dna-config';
import { withCurrentRoute } from 'calypso/components/route';
import QueryExperiments from 'calypso/components/data/query-experiments';
import { getVariationForUser } from 'calypso/state/experiments/selectors';

/**
* Style dependencies
Expand Down Expand Up @@ -80,6 +82,8 @@ class Layout extends Component {
.classList.add( `is-${ this.props.colorSchemePreference }` );
}
}

// This code should be removed when the nav-unification project has been rolled out to 100% of the customers.
if ( config.isEnabled( 'nav-unification' ) ) {
window.addEventListener( 'scroll', scrollCallback );
window.addEventListener( 'resize', scrollCallback );
Expand Down Expand Up @@ -158,7 +162,7 @@ class Layout extends Component {
config.isEnabled( 'woocommerce/onboarding-oauth' ) &&
isWooOAuth2Client( this.props.oauth2Client ) &&
this.props.wccomFrom,
'is-nav-unification': config.isEnabled( 'nav-unification' ),
'is-nav-unification': this.props.navUnificationVariation === 'treatment',
} );

const optionalBodyProps = () => {
Expand All @@ -171,9 +175,16 @@ class Layout extends Component {
return optionalProps;
};

if ( this.props.navUnificationVariation === 'treatment' ) {
config.enable( 'nav-unification' );
} else {
config.disable( 'nav-unification' );
}

const { shouldShowAppBanner } = this.props;
return (
<div className={ sectionClass }>
<QueryExperiments />
<BodySectionCssClass
group={ this.props.sectionGroup }
section={ this.props.sectionName }
Expand Down Expand Up @@ -314,6 +325,7 @@ export default compose(
shouldQueryAllSites: currentRoute && currentRoute !== '/jetpack/connect/authorize',
isNewLaunchFlow,
isCheckoutFromGutenboarding,
navUnificationVariation: getVariationForUser( state, 'nav_unification' ),
};
} )
)( Layout );
27 changes: 27 additions & 0 deletions client/lib/create-config/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,37 @@ const isEnabled = ( data ) => ( /** @type {string} */ feature ) =>
const enabledFeatures = ( data ) => () =>
Object.keys( data.features ).filter( ( feature ) => !! data.features[ feature ] );

/**
* Enables a specific feature.
*
* @param {object} data the json environment configuration to use for getting config values
* @public
*/
const enable = ( data ) => ( /** @type {string} */ feature ) => {
if ( data.features ) {
Copy link
Contributor

@getdave getdave Nov 19, 2020

Choose a reason for hiding this comment

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

In dev mode should we consider throwing a warning in the console when the feature being enabled/disabled is not currently defined such as

if ( 'development' === process.env.NODE_ENV ) {
		throw new ReferenceError(
			`Could not find config value for key '${ key }'\n` +
				"Please make sure that if you need it then it has a default value assigned in 'config/_shared.json'"
		);
	}

This is what the primary config does already when you're trying to get a top level key. We could consider same for features as a developer aid.

data.features[ feature ] = true;
}
};

/**
* Disables a specific feature.
*
* @param {object} data the json environment configuration to use for getting config values
* @public
*/

const disable = ( data ) => ( /** @type {string} */ feature ) => {
if ( data.features ) {
data.features[ feature ] = false;
}
};

module.exports = ( data ) => {
const configApi = config( data );
configApi.isEnabled = isEnabled( data );
configApi.enabledFeatures = enabledFeatures( data );
configApi.enable = enable( data );
configApi.disable = disable( data );

return configApi;
};
60 changes: 60 additions & 0 deletions client/lib/create-config/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,64 @@ describe( 'index', () => {
} );
} );
} );

describe( 'config utilities', () => {
let config;

beforeEach( () => {
config = createConfig( {
features: {
flagA: false,
flagB: false,
flagC: true,
},
} );
} );

afterEach( () => {
config = null;
} );

describe( 'isEnabled', () => {
test( 'it correctly reports status of features', () => {
expect( config.isEnabled( 'flagA' ) ).to.be.false;
expect( config.isEnabled( 'flagC' ) ).to.be.true;
} );

test( 'it defaults to "false" when feature is not defined', () => {
expect( config.isEnabled( 'flagXYZ' ) ).to.be.false;
} );
} );

describe( 'enable', () => {
test( 'it can enable features which are not yet set', () => {
config.enable( 'flagD' );
config.enable( 'flagE' );
expect( config.isEnabled( 'flagD' ) ).to.be.true;
expect( config.isEnabled( 'flagE' ) ).to.be.true;
} );

test( 'it can toggle existing features to enable them', () => {
config.enable( 'flagA' );
expect( config.isEnabled( 'flagA' ) ).to.be.true;
} );
} );

describe( 'disable', () => {
test( 'it can toggle existing features to disable them', () => {
config.disable( 'flagC' );
expect( config.isEnabled( 'flagC' ) ).to.be.false;
} );

test( 'it retains existing disable setting for features that are already disabled', () => {
config.disable( 'flagA' );
expect( config.isEnabled( 'flagA' ) ).to.be.false;
} );

test( 'it will handle setting new features to a initial disabled state', () => {
config.disable( 'flagZXY' );
expect( config.isEnabled( 'flagZXY' ) ).to.be.false;
} );
} );
} );
} );