WIP: Implement feature flags for Search Block #13829

talldan · 2019-02-12T07:12:42Z

Description

This is an offshoot of #13324, an attempt to implement feature flags from that PR for the new Search Block.

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

youknowriad · 2019-02-12T09:56:58Z

packages/block-library/src/search/edit.js

-			/>
-		</div>
-	);
+if ( process.env.GUTENBERG_PHASE === 2 ) {


Why do we need this here? Isn't the check in the block-library sufficient?

The ternary in block-library/index.js is enough to make sure the block isn't registed, the other if statements make sure the code is removed from the bundle if the phase isn't 2.

To add to that, my understanding is the other code (search/edit, search/index) would be eliminated automatically if we started using the sideEffects property in the package.json of our packages:
https://webpack.js.org/guides/tree-shaking/

The new webpack 4 release expands on this capability with a way to provide hints to the compiler via the "sideEffects" package.json property to denote which files in your project are "pure" and therefore safe to prune if unused.

I suppose it's a question of how far we want to go. If simply removing code from the public api is the goal, then it becomes a lot simpler to implement.

If the code needs to be eliminated entirely from the bundle, then the extra logic is needed in each file or we can try using sideEffects.

@blowery or @jsnajdr, I remember you talking about it in one of the PRs. What do you recommend?

There are no blockers to mark @wordpress/block-library with sideEffects: false. That would be much more convenient.

I tried with sideEffects disabled and it works great. I can't find any JS code inside bundled production file.

Yes, specifying sideEffects: false and doing the process.env.GUTENBERG_PHASE === 2 check only in the block library is the best way forward.

Isn't the check in the block-library sufficient?

Without sideEffect: false, the search block would still be imported and bundled. The transformed code would be:

import * as search from './search'; registerBlocks( [ false ? search : undefined ] );

The fact that the search import is not used is not a sufficient reason to eliminate it. The ./search module's code can have side effects (e.g., window.wp.search = { ... }) that need to be executed. The sideEffects: false flag is needed to tell webpack that the elimination is safe and desired.

By the way, the way how each block's name and settings are exported as distinct named exports and then treated as one object (import * as search) was surprising to see. It would be more natural to treat the whole { name, settings } object as one unit and export is as default. Or is there any case where the exports are cherry-picked, like in:

import { name as searchName } from '@wordpress/block-library/search';

?

By the way, the way how each block's name and settings are exported as distinct named exports and then treated as one object (import * as search) was surprising to see. It would be more natural to treat the whole { name, settings } object as one unit and export is as default. Or is there any case where the exports are cherry-picked, like in:

import { name as searchName } from '@wordpress/block-library/search';

?

Agreed, I think I coded it this way a long time ago and I don't remember why :) We are going to change it in the near future as we are finalizing how blocks can be registered on the server and client using shared settings with JSON definition. See #13693. There are going to be new challenges :)

Many thanks for explaining how Webpack uses sideEffects to eliminate dead code. I will investigate which packages can benefit from it. There are lots of packages which don't run any side-effects when they are loaded in the browser. I think the only issue we will have with those which register stores.

I think the only issue we will have with those which register stores.

That's right, we don't need to go far to see a typical module side effect 🙂 The store registrations, namely lack of control over the registration order, caused a few headaches in the Calypso integration project by the way.

Yep, every call to registerStore seems to cause side effects. We'd have to list those files or find a way to not cause side-effects.

Block registration seems ok though.

I've made an issue here to track adding the property:
#13910

gziolo · 2019-02-14T17:05:12Z

packages/e2e-tests/specs/blocks/search.test.js

+
+	it( 'can be created by typing "/quote"', async () => {
+		// Create a list with the slash block shortcut.
+		await clickBlockAppender();


Let's try to use feature flag also inside the test and see if it works.

Add new package for editor configuration, initially containing just feature flags Rework build commands to use correct NODE_ENV for feature flags Revert "Rework build commands to use correct NODE_ENV for feature flags" This reverts commit 4cb0a39. Revert "Add new package for editor configuration, initially containing just feature flags" This reverts commit 0c21fc2. Switch to using webpack define plugin to inject a global GUTENBERG_PHASE variable Iterate: use window.GUTENBERG_PHASE to avoid thrown errors from an undefined global Add custom eslint rule for usage of GUTENBERG_PHASE Disable new eslint rule when used in webpack config Add readme Include phase 2 features in e2e tests Allow use of GUTENBERG_PHASE in a ternary and update documentation. Add links to docs Minor docs changes Switch from window.GUTENBERG_PHASE to process.env.GUTENBERG_PHASE Update docs for feature flags. Move `Basic Use` section higher up, and simplify a sentence

…PHASE

jsnajdr · 2019-02-15T09:29:04Z

packages/eslint-plugin/docs/rules/gutenberg-phase.md

+
+- Only access `GUTENBERG_PHASE` via `process.env`, e.g. `process.env.GUTENBERG_PHASE`. This is required since webpack's define plugin only replaces exact matches of `process.env.GUTENBERG_PHASE` in the codebase.
+- The `GUTENBERG_PHASE` variable should only be used in a strict equality comparison with a number, e.g. `process.env.GUTENBERG_PHASE === 2` or `process.env.GUTENBERG_PHASE !== 2`. The value of the injected variable should always be a number, so this ensures the correct evaluation of the expression. Furthermore, when `process.env.GUTENBERG_PHASE` is undefined this comparison still returns either true (for `!==`) or false (for `===`), whereas both the `<` and `>` operators will always return false.
+- `GUTENBERG_PHASE` should only be used within the condition of an if statement, e.g. `if ( process.env.GUTENBERG_PHASE === 2 ) { // implement feature here }` or ternary `process.env.GUTENBERG_PHASE === 2 ? something : somethingElse`. This rule ensures code that is disabled through a feature flag is removed by dead code elimination.


Logical expressions are also supported and will work correctly:

process.env.GUTENBERG_PHASE === 2 && fun(); process.env.GUTENBERG_PHASE === 2 || fun();

jsnajdr · 2019-02-15T09:31:02Z

docs/designers-developers/developers/feature-flags.md

+
+When building the codebase for the plugin the variable will be replaced with the number literal `2`:
+```js
+if ( 2 === 2 ) {


To be more technically correct, webpack will evaluate the whole === expression and will replace it with true. That's what you will see in the unminified output.

jsnajdr · 2019-02-15T09:48:34Z

packages/block-library/src/search/edit.js

-			/>
-		</div>
-	);
+if ( process.env.GUTENBERG_PHASE === 2 ) {


Yes, specifying sideEffects: false and doing the process.env.GUTENBERG_PHASE === 2 check only in the block library is the best way forward.

Isn't the check in the block-library sufficient?

Without sideEffect: false, the search block would still be imported and bundled. The transformed code would be:

import * as search from './search'; registerBlocks( [ false ? search : undefined ] );

The fact that the search import is not used is not a sufficient reason to eliminate it. The ./search module's code can have side effects (e.g., window.wp.search = { ... }) that need to be executed. The sideEffects: false flag is needed to tell webpack that the elimination is safe and desired.

By the way, the way how each block's name and settings are exported as distinct named exports and then treated as one object (import * as search) was surprising to see. It would be more natural to treat the whole { name, settings } object as one unit and export is as default. Or is there any case where the exports are cherry-picked, like in:

import { name as searchName } from '@wordpress/block-library/search';

?

jsnajdr · 2019-02-15T09:49:22Z

packages/eslint-plugin/docs/rules/gutenberg-phase.md

+```
+
+```js
+if ( true || process.env.GUTENBERG_PHASE > 2 ) {


What kind of antipattern is this example trying to show?

talldan added the [Status] In Progress Tracking issues with work in progress label Feb 12, 2019

talldan self-assigned this Feb 12, 2019

talldan requested review from aduth, ajitbohra, chrisvanpatten, gziolo, jorgefilipecosta, mkaz, nerrad, noisysocks, oandregal, notnownikki, ntwb, Soean and youknowriad as code owners February 12, 2019 07:12

talldan mentioned this pull request Feb 12, 2019

Add feature flags for Phase 2 #13324

Merged

youknowriad reviewed Feb 12, 2019

View reviewed changes

gziolo reviewed Feb 14, 2019

View reviewed changes

talldan added 11 commits February 15, 2019 14:58

Ensure GUTENBERG_PHASE environment variable is available for webpack

2243548

Ensure GUTENBERG_PHASE is a number

1360a25

Ensure GUTENBERG_PHASE is set in unit tests

1476eac

Add feature flags for search block

652d711

Use <rootDir> in jest config

7c5280c

switch to using package.json config to define the value of GUTENBERG_…

f5bdc39

…PHASE

Add basic search block e2e test

c5c82f9

Sort custom lint rules alphabetically

e818a32

Add comment about GUTENBERG_PHASE

106d82d

Update jest config for GUTENBERG_PHASE

f52a902

talldan force-pushed the try/use-feature-flags-with-a-feature branch from e0b919c to e33dec5 Compare February 15, 2019 06:58

Add webpack as dependency to main package.json

1e80885

talldan force-pushed the try/use-feature-flags-with-a-feature branch from e33dec5 to 1e80885 Compare February 15, 2019 07:25

jsnajdr reviewed Feb 15, 2019

View reviewed changes

talldan mentioned this pull request Feb 18, 2019

Add sideEffects property to package.json for our packages #13910

Closed

talldan closed this Feb 18, 2019

talldan deleted the try/use-feature-flags-with-a-feature branch February 18, 2019 06:45

gziolo mentioned this pull request Oct 9, 2019

Packages: Add information for webpack about no side effects #17862

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Implement feature flags for Search Block #13829

WIP: Implement feature flags for Search Block #13829

talldan commented Feb 12, 2019

youknowriad Feb 12, 2019

talldan Feb 13, 2019

talldan Feb 13, 2019

gziolo Feb 14, 2019 •

edited

Loading

gziolo Feb 14, 2019

jsnajdr Feb 15, 2019

gziolo Feb 15, 2019

gziolo Feb 15, 2019

jsnajdr Feb 15, 2019

talldan Feb 18, 2019

gziolo Feb 14, 2019 •

edited

Loading

jsnajdr Feb 15, 2019

jsnajdr Feb 15, 2019

jsnajdr Feb 15, 2019

jsnajdr Feb 15, 2019

WIP: Implement feature flags for Search Block #13829

WIP: Implement feature flags for Search Block #13829

Conversation

talldan commented Feb 12, 2019

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Feb 14, 2019 • edited Loading

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

gziolo Feb 14, 2019 • edited Loading

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

gziolo Feb 14, 2019 •

edited

Loading

gziolo Feb 14, 2019 •

edited

Loading