Nav Unification: uses experiment to set a feature flag

[cpapazoglou]

Changes proposed in this Pull Request

Since that nav-unification project is touching many different parts of the code (eg search for nav-unification) it wouldn't make sense trying to create different components in order to utilise the experiment component

wp-calypso/client/components/experiment/readme.md

Lines 19 to 37 in a87658f

	<Experiment name={ 'experiment_name' }>
	<DefaultVariation name="A">
	Shows this variation when:
	<ol>
	<li>The user doesn't have a variation assigned to them.</li>
	<li>The experiment doesn't exist.</li>
	<li>Before the experiment goes live</li>
	</ol>
	</DefaultVariation>
	<Variation name="B">Show variation B if the user is assigned this variation.</Variation>
	<LoadingVariations>
	Show this when we don't know the variation because we haven't called the API yet to get
	variations.
	<br />
	Protip: If this experiment is in the middle or end of a flow, try putting
	<code>&lt;QueryExperiments&gt;</code> at the beginning of your flow to ensure the variation
	assignments are preloaded.
	</LoadingVariations>
	</Experiment>

Also, changes have to kick in really soon cause they are affecting the whole layout (masterbar, navbar).

• Use experiments internal tools to override feature flags in layout component

Testing instructions

• apply this patch and load calypso
• you should see the same UI as in production
• go to nav_unification experiment and click on treatment (paYJgx-Zj-p2#comment-1406)
• reload calypso twice, you should be seeing the nav unification UI / UX. Sometimes the api call is not triggered in subsequent requests.

Builds together with D52906-code, please test accordingly.

Fixes #47044

[matticbot]

Test live: https://calypso.live/?branch=update/set-feature-flag-upon-experiment

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~1078 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
entry-main                 +3207 B  (+0.2%)     +983 B  (+0.3%)
entry-login                 +126 B  (+0.0%)      +33 B  (+0.0%)
entry-gutenboarding         +126 B  (+0.0%)      +29 B  (+0.0%)
entry-domains-landing       +126 B  (+0.0%)      +33 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~8835 bytes removed 📉 [gzipped])

name                    parsed_size           gzip_size
pages                       -2768 B  (-1.0%)     -884 B  (-1.1%)
checkout                    -2768 B  (-0.2%)     -811 B  (-0.2%)
woocommerce                  -727 B  (-0.0%)     -253 B  (-0.0%)
stats                        -727 B  (-0.1%)     -276 B  (-0.2%)
site-blocks                  -727 B  (-0.2%)     -254 B  (-0.3%)
signup                       -727 B  (-0.1%)     -270 B  (-0.2%)
settings-security            -727 B  (-0.2%)     -276 B  (-0.3%)
settings-jetpack             -727 B  (-0.3%)     -276 B  (-0.4%)
settings                     -727 B  (-0.1%)     -255 B  (-0.2%)
security                     -727 B  (-0.2%)     -254 B  (-0.2%)
scan                         -727 B  (-0.2%)     -290 B  (-0.3%)
reader                       -727 B  (-0.1%)     -287 B  (-0.2%)
purchases                    -727 B  (-0.1%)     -266 B  (-0.1%)
purchase-product             -727 B  (-0.2%)     -231 B  (-0.2%)
privacy                      -727 B  (-0.3%)     -254 B  (-0.3%)
plans                        -727 B  (-0.1%)     -280 B  (-0.1%)
people                       -727 B  (-0.2%)     -258 B  (-0.2%)
notification-settings        -727 B  (-0.2%)     -254 B  (-0.3%)
me                           -727 B  (-0.3%)     -254 B  (-0.4%)
jetpack-connect              -727 B  (-0.1%)     -269 B  (-0.1%)
jetpack-cloud-settings       -727 B  (-0.4%)     -280 B  (-0.5%)
home                         -727 B  (-0.1%)     -252 B  (-0.2%)
help                         -727 B  (-0.1%)     -254 B  (-0.2%)
happychat                    -727 B  (-0.3%)     -254 B  (-0.3%)
concierge                    -727 B  (-0.2%)     -254 B  (-0.3%)
backup                       -727 B  (-0.2%)     -290 B  (-0.2%)
activity                     -727 B  (-0.1%)     -291 B  (-0.2%)
account-close                -727 B  (-0.2%)     -254 B  (-0.3%)
account                      -727 B  (-0.2%)     -254 B  (-0.2%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~3739 bytes removed 📉 [gzipped])

name                                             parsed_size           gzip_size
async-load-calypso-blocks-editor-checkout-modal      -2768 B  (-0.3%)     -792 B  (-0.3%)
async-load-signup-steps-plans                        -2041 B  (-0.9%)     -667 B  (-1.0%)
async-load-signup-steps-domains                      -2041 B  (-0.8%)     -594 B  (-0.9%)
async-load-quick-language-switcher                    -727 B  (-1.1%)     -302 B  (-1.5%)
async-load-design-playground                          -727 B  (-0.0%)     -281 B  (-0.1%)
async-load-design-blocks                              -727 B  (-0.0%)     -290 B  (-0.0%)
async-load-design                                     -727 B  (-0.0%)     -272 B  (-0.1%)
async-load-calypso-my-sites-sidebar                   -727 B  (-0.5%)     -252 B  (-0.6%)
async-load-calypso-blocks-jitm                        -727 B  (-8.3%)     -289 B  (-9.9%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[mreishus]

I made a simple config enable/disable API here w/ proof it works: #47535

[cpapazoglou]

I made a simple config enable/disable API here w/ proof it works: #47535

Thanks for wrangling this @mreishus! I cherry picked the needed code and removed the hacky method!

[getdave]

I tested this and with the treatment active I seemed to be getting the Nav Unification without having to provide a flag in the URL like before (I assume this is what I'm testing?). When I turned the treatment off it went back to the Nav we currently have in Production for users. This seemed correct.

That said I noticed some oddities.

x2 Plugins links

Not sure why this is happening, but it's happening for both Calypso and WPAdmin and there are x2 Plugins entries in the API response

(Ignore) Transfering to WPAdmin didn't match design wise

Update this was due to outdated testing instructions. With new instructions this works!

Not sure what's happening here but when I click on Feedback (or any items that takes me to WPAdmin) i get a totally different UI.

I have the diff D52906-code applied to my SB.

[getdave]

Hope you don't mind I added some unit tests.

Also wondering if you should update your helpers to throw some helpful warnings when we are in DEV environment mode? You'd need to add tests for these scenarios as well.

[getdave]

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.

[getdave]

I re-tested with your update instructions (😉 ) and now the Calypso -> WPAdmin flow works perfectly and I don't need any flags in the URL 🥳

However, I still get x2 Plugins links - not sure why this is happening? as per my first review. You can see in the gif above.

[cpapazoglou]

I have updated the diff that resolves the duplicate menus. Since this is handled in D52906-code and this PR works, @getdave how would you feel approving this one?

Here are the Check code style results that fails

<checkstyle version="4.3">
<file name="/home/teamcity-3/buildAgent/work/c4a9d5b38c1dacde/client/config/README.md">
<error line="37" column="1" severity="warning" message="Insert `⏎` (prettier/prettier)" source="eslint.rules.prettier/prettier"/>
</file>
<file name="/home/teamcity-3/buildAgent/work/c4a9d5b38c1dacde/client/config/index.js"/>
<file name="/home/teamcity-3/buildAgent/work/c4a9d5b38c1dacde/client/layout/index.jsx"/>
<file name="/home/teamcity-3/buildAgent/work/c4a9d5b38c1dacde/client/lib/create-config/index.js">
<error line="61" column="0" severity="error" message="Expected @param names to be "data". Got "feature, data". (jsdoc/check-param-names)" source="eslint.rules.jsdoc/check-param-names"/>
<error line="64" column="0" severity="error" message="Invalid JSDoc tag name "api". (jsdoc/check-tag-names)" source="eslint.rules.jsdoc/check-tag-names"/>
<error line="72" column="0" severity="error" message="Expected @param names to be "data". Got "feature, data". (jsdoc/check-param-names)" source="eslint.rules.jsdoc/check-param-names"/>
<error line="74" column="0" severity="error" message="Invalid JSDoc tag name "api". (jsdoc/check-tag-names)" source="eslint.rules.jsdoc/check-tag-names"/>
<error line="87" column="0" severity="error" message="Invalid JSDoc tag name "api". (jsdoc/check-tag-names)" source="eslint.rules.jsdoc/check-tag-names"/>
</file>
<file name="/home/teamcity-3/buildAgent/work/c4a9d5b38c1dacde/client/lib/create-config/test/index.js"/>
</checkstyle>

and these are the lines in question

wp-calypso/client/lib/create-config/index.js

Lines 58 to 88 in d673712

	/**
	* Checks whether a specific feature is enabled.
	*
	* @param {string} feature Feature name
	* @param {object} data the json environment configuration to use for getting config values
	* @returns {boolean} True when feature is enabled.
	* @api public
	*/
	const isEnabled = ( data ) => ( feature ) =>
	( data.features && !! data.features[ feature ] ) || false;
	/**
	* Enables a specific feature.
	*
	* @param {string} feature Feature name
	* @param {object} data the json environment configuration to use for getting config values
	* @api public
	*/
	const enable = ( data ) => ( feature ) => {
	if ( data.features ) {
	data.features[ feature ] = true;
	}
	};
	/**
	* Disables a specific feature.
	*
	* @param {string} feature Feature name
	* @param {object} data the json environment configuration to use for getting config values
	* @api public
	*/

I am not sure how these can be resolved (and they existed prior to this PR), any thoughts?

[getdave]

Approving on the basis that this worked last thing yesterday and @cpapazoglou advises that's he's fixed the duplicate Plugins menu item issue in the associated diff.

[cpapazoglou]

Deployed r217327-wpcom