Add feature flags for Phase 2

[talldan]

Description

An attempt at introducing feature flags for phase 2 (see #11016).

The proposal is to use webpack's define plugin (https://webpack.js.org/plugins/define-plugin/) to replace instances of process.env.GUTENBERG_PHASE with an integer denoting the phase (1 or 2) during the build.

Within the codebase, the GUTENBERG_PHASE variable would be used to conditionally avoid executing code:

// file A
function futurePhaseTwoFeature() {
  // implementation
}

export const phaseTwoFeature = process.env.GUTENBERG_PHASE === 2 ? futurePhaseTwoFeature : undefined;

// file B
import { phaseTwoFeature } from 'file-a';

// ...
if ( process.env.GUTENBERG_PHASE === 2 ) {
  phaseTwoFeature();
}

When building for core, the majority code would be stripped out by webpack's minfication/dead code elimination process.

This PR also introduces docs and linting rules to ensure that process.env.GUTENBERG_PHASE is used in the correct way.

[noisysocks]

Thanks for getting started on this! Nothing moves the conversation forward like putting up some strawman code 😀

It'd be great to consider this a request for collaboration, I'm not completely familiar with the release processes, so maybe not the perfect person to deliver a finalised version of this.

@youknowriad and @gziolo should be able to give us some direction here once they're back from their meetup. Paging @WordPress/gutenberg-core, too.

Introduce a new 'editor-configuration' package. It has a README that includes a usage example.

We probably want to give this a more generic name as editor-configuration implies it's only useful for the editor. In Phase 2 we'll be using Gutenberg components into other parts of WP Admin.

The new package contains three configuration files (development.js, plugin.js, and production.js)

My first reaction is that overloading NODE_ENV with a meaning that isn't seen elsewhere in the JavaScript ecosystem will cause confusion and bite us later.

Perhaps we could have two environment variables, e.g. NODE_ENV=production|development and GUTENBERG_ENV=plugin|core?

Or perhaps we ought to ditch what Calypso does and go back to something like Riad first suggested, e.g. NODE_ENV=production|development and GUTENBERG_PHASE=1|2?

Dead code elimination (i.e. not shipping code for disabled features). I looked into getting this working. Firstly, I don't think the technique here is works in terms of static analysis because the config file is determined at runtime

What do we need for static analysis to work? How complex can an expression be before Webpack gives up on trying to eliminate it as dead code?

It also seems like webpack may require the sideEffects: false flag in the package.json for our packages

What's involved in getting sideEffects added to all our packages?

[talldan]

We probably want to give this a more generic name as editor-configuration implies it's only useful for the editor. In Phase 2 we'll be using Gutenberg components into other parts of WP Admin.

True, hadn't thought about that. Let's see how this develops before renaming it.

What do we need for static analysis to work? How complex can an expression be before Webpack gives up on trying to eliminate it as dead code? ... Or perhaps we ought to ditch what Calypso does and go back to something like Riad first suggested, e.g. NODE_ENV=production|development and GUTENBERG_PHASE=1|2?

Hard to say. I think there would definitely need to be no runtime logic, which would require some build time magic. It might also preclude using a function like getFeatureFlag, which would be a shame.

A tricky aspect of handling this at build time is that (from my understanding) the integration with core has its own separate webpack setup:

• Development - code is transpiled by babel, Gutenberg's webpack does the bundling
• Plugin - code is transpiled by babel, Gutenberg's webpack does the bundling, then bundle distributed as a plugin.
• Production - code is transpiled by babel and published to npm packages, Core consumes the packages and core webpack does the bundling.

Perhaps that leaves an option of a webpack or babel plugin as the best way to inject the correct flags at build time?

What's involved in getting sideEffects added to all our packages?

Thinking some more on it, I'm a bit unclear on how much this will help. I think it just ensures unused exports are culled from the final bundle, which isn't helpful for feature flags since exports or imports can't be conditional. The webpack docs are definitely not very clear on the exact behaviour.

[aduth]

Introduce a new 'editor-configuration' package. It has a README that includes a usage example.

We probably want to give this a more generic name as editor-configuration implies it's only useful for the editor. In Phase 2 we'll be using Gutenberg components into other parts of WP Admin.

This was my first thought as well. I see no reason this needs to be specific to editor at all. I think we could come up with a convention around naming specific features to allow some subgrouping (editor/some-awesome-feature), but it seems unnecessary at the module level.

[mcsf]

Or perhaps we ought to ditch what Calypso does and go back to something like Riad first suggested, e.g. NODE_ENV=production|development and GUTENBERG_PHASE=1|2?

I agree with a GUTENBERG_PHASE=1|2 flag for clarity in terms of goals.

Also, can we take a step back and figure out what the minimum (and simplest) we can do here is?

Calypso offers a series of environments from least to most production-ready, and features become exposed to more environments as they mature. But strictly speaking that doesn't fall within the scope of feature flags for Gutenberg phases, and also Gutenberg has its own production line for maturing features. Here's how I see it:

Calypso	Gutenberg
development	master branch
staging	RC plugin
production	plugin

We could argue that pushing updates to core WordPress is a subsequent step based on the plugin release.

Wouldn't the above make things a little easier for us? It would mean that the only real branching hinges on one flag, GUTENBERG_PHASE. Meanwhile, NODE_ENV is left to determine whether the code should be compiled for packaging or for ease of development.

In short, we should be clear about our goals for feature flags:

• The first goal is to have one codebase from which we can develop the next phase of Gutenberg while keeping the ability to maintain Phase 1 as released in WP 5.x. This goal is consensual and is described in Feature flag to separate Phase 2 features from packages updates targeted as Core fixes/polish. #11016.
• A hypothetical second goal could be to have certain features be left to incubate in development and skip plugin releases until they are ready. This can be extremely handy on one hand, and gives us control, but gives us fragmentation on the other. Its pitfalls include: a maintenance/context-switch/decision overhead, more targets to test things against (meaning more potential testing gaps), etc. Do we want it?

[noisysocks]

A hypothetical second goal could be to have certain features be left to incubate in development and skip plugin releases until they are ready.

I don't think it's necessary (yet) but it's worth noting that GUTENBERG_PHASE could easily accomodate it by having GUTENBERG_PHASE=3 for very experimental features. Then, we configure:

• .npm run publish to compile the scripts with GUTENBERG_PHASE=1
• ./bin/build-package-zip.sh to compile the scripts with GUTENBERG_PHASE=2
• npm run dev to compile the scripts with GUTENBERG_PHASE=3

Or for super experimental features one could even gate the feature to GUTENBERG_PHASE=4 where it can only be seen when manually running GUTENBERG_PHASE=4 npm run dev.

All unnecessary food for thought. My only point is that the idea scales fairly well 🙂

[noisysocks]

Expanding on #13324 (comment), I thought I'd lay out a proposal for how the release process using GUTENBERG_PHASE would look:

• The Gutenberg master branch is the single source of truth and contains the code that ends up both in Gutenberg plugin releases (via ./bin/build-plugin-zip.sh) and WordPress Core releases (via npm run publish).
• We gate new features that should appear in the Gutenberg plugin and not WordPress Core using the GUTENBERG_PHASE environment variable, e.g.

if ( process.env.GUTENBERG_PHASE >= 2) {
	// The RSS block is under development, so only load it in the Gutenberg plugin and not WP Core
	registerBlockType( 'core/rss', rssSettings );
}

• We configure Webpack to inject the GUTENBERG_PHASE constant into our scripts at build time and to eliminate dead code that is unreachable because of a GUTENBERG_PHASE check.
• When building packages for WordPress Core (npm run publish), we set GUTENBERG_PHASE=1. We do this when there is a new WordPress beta.
• When building the Gutenberg plugin zip (./bin/build-plugin-zip.sh), we set GUTENBERG_PHASE=2. We do this every two weeks.

Considerations:

• Will have to configure Travis CI to test both GUTENBERG_PHASE=1 and GUTENBERG_PHASE=2.
• Will have to educate developers that, unless it's gated, code merged to master will land in the next WordPress release.
• No more g-minor.

[talldan]

Based on the discussion, I've reworked this to use webpack's define plugin to inject a GUTENBERG_PHASE global.

Dead code elimination seems to work. I did a quick test by setting the phase to 1, then implementing an early return from a function:

if ( GUTENBERG_PHASE < 2 ) {
    return;
}

It looked like none of the code in the function body was bundled (I tested by searching for a class name string, which wasn't present at all in the bundled code). 🎉

Using the define plugin does add a restriction that other consumers of packages would also have to use it (or a similar solution). For example, core's webpack implementation could hard-code this value to 1.

Thoughts on this approach?

[mcsf]

Ouch, ignore my closing this issue. My keyboard magic with Vimium went a little too far. 😅

[mcsf]

Thanks, this looks a lot nicer.

Using the define plugin does add a restriction that other consumers of packages would also have to use it (or a similar solution). For example, core's webpack implementation could hard-code this value to 1.

I'm not sure we would have a good way to enforce the presence of the env var for third-party consumers: since they can use any config for the bundling, we can't require anything in webpack, and since they can build and consume packages independently, we can't very reliably throw runtime errors if the var is missing.

It seems safer to just assume 1 is the default value. Is it worth adding an ESLint rule for any reads of the variable? For instance, requiring that any reads need to be part of a strict equality check for a value greater than 1. Examples:

Valid:

if ( process.env.GUTENBERG_PHASE === 2 ) {

if (
    process.env.GUTENBERG_PHASE === 2 ||
    process.env.GUTENBERG_PHASE === 3
) {

Invalid:

if ( process.env.GUTENBERG_PHASE === 1 ) {

if ( process.env.GUTENBERG_PHASE ) {

if ( process.env.GUTENBERG_PHASE > 1 ) {

[noisysocks]

Using the define plugin does add a restriction that other consumers of packages would also have to use it (or a similar solution). For example, core's webpack implementation could hard-code this value to 1.

@talldan and I were discussing this yesterday and we concluded that it's arguably fine that we publish code on npm that requires webpack.DefinePlugin since this is exactly what React does, for example. We can communicate this requirement through documentation similar to React's.

The alternative is that we modify npm run build:packages to use Babel plugins that define process.env.GUTENBERG_PHASE and eliminate dead code. This means that third parties are completely unable to use Phase 2 code, though.

It seems safer to just assume 1 is the default value. Is it worth adding an ESLint rule for any reads of the variable? For instance, requiring that any reads need to be part of a strict equality check for a value greater than 1.

That's an interesting idea. I think I'm into it. At the very least it makes sense to forbid if ( GUTENBERG_PHASE === 1 ).

One consideration: What will it look like when we "ship" Phase 2 to WordPress Core? Would we simply remove all of the if ( GUTENBERG_PHASE === 2 ) checks?

[noisysocks]

Assuming 1 and using ESLint to prevent GUTENBERG_PHASE === 1 checks doesn't prevent a ReferenceError from being thrown when GUTENBERG_PHASE is referenced.

Some off-the-bat ideas:

1. We could require that third parties using WordPress packages use webpack.DefinePlugin to set GUTENBERG_PHASE. Pros: No effort on our part. Cons: Difficult for third parties.

2. Have npm run build:scripts prepend if ( typeof GUTENBERG_PHASE === 'undefined' ) { GUTENBERG_PHASE = 1; } to all of the package JavaScript files that we generate. Pros: No effort for third parties. Cons: Fairly heavy handed, probably negatively affects bundle size and parse time.

3. Have ESLint enforce that our feature gates look like this:

   if ( typeof GUTENBERG_PHASE !== 'undefined' && GUTENBERG_PHASE === 2) {

   Pros: No effort for third parties. Cons: We have to put up with fairly verbose checks.

4. Implement a Babel plugin which transforms feature gates that look like this:

   if ( isGutenbergPhase( 2 ) ) {

   Into code that looks like this:

   if ( typeof GUTENBERG_PHASE !== 'undefined' && GUTENBERG_PHASE === 2 ) {

   Pros: No effort for third parties. Cons: A lot of ✨ magic ✨.

I'm leaning towards (3).

[talldan]

With the define plugin we can explicitly replace only window.GUTENBERG_PHASE, and then use linting as enforcement about the usage:

new DefinePlugin( {
    'window.GUTENBERG_PHASE': JSON.stringify( process.env.GUTENBERG_PHASE || 1 ),
} ),

☝️ that will only replace instances of window.GUTENBERG_PHASE, which would avoid any errors being thrown for those without the define plugin.

Implement a Babel plugin which transforms feature gates that look like this:

if ( isGutenbergPhase( 2 ) ) {

This is a bit crazy, but it's also possible to do something like this with the define plugin:

		new DefinePlugin( {
			'window.isGutenbergPhase': `( ( phaseToCompare ) => phaseToCompare === ${ process.env.GUTENBERG_PHASE || 1 } )`,
		} ),

if ( window.isGutenbergPhase( 2 ) ) is replaced with an immediately invoked function. Dead code elimination still seems to work as well (I imagine the minifier unwraps the function, then eliminates it). That would mean less need for linting, but has the downside that it's harder to implement for third parties. It might not be possible at all with some non-webpack bundlers.

[talldan]

Hey @mcsf thanks for the comments. Linting is a good idea, it would also mean there's less chance of dead code elimination being bypassed if the usage is enforced.

Is there a preference for process.env.GUTENBERG_PHASE over window.GUTENBERG_PHASE?

I did notice we already have a line of code that references process.env.NODE_ENV, so it seem there's an assumption that process.env is provided by whatever build process is bundling the code:

gutenberg/packages/components/src/tooltip/index.js

Line 94 in ab0acc3

if ( 'development' === process.env.NODE_ENV ) {

[mcsf]

Is there a preference for process.env.GUTENBERG_PHASE over window.GUTENBERG_PHASE?

I did notice we already have a line of code that references process.env.NODE_ENV, so it seem there's an assumption that process.env is provided by whatever build process is bundling the code:

I didn't think too hard about it, but I did go along with our prior art.

Some off-the-bat ideas:

I also lean towards idea number 3, with whatever syntax fits best (typeof et al.)!

[talldan]

@gziolo Thanks for the reviews, really appreciate the help 😄

The merge conflict should now be fixed. 👍

The only thing I'd say is that it'd be good to do some testing with how this works downstream (checking that everything works as expected when feature flagged code is integrated in core, or when it's consumed by plugins). That could always come after this is merged.

[gziolo]

The only thing I'd say is that it'd be good to do some testing with how this works downstream (checking that everything works as expected when feature flagged code is integrated in core, or when it's consumed by plugins). That could always come after this is merged.

Let's merge and test once published to npm. In the worst case scenario, we will publish patch later but I personally feel pretty confident about the proposed approach.

🚢

[gziolo]

I shared some details about this PR in #core-js channel on WordPress Slack (link requires registration at https://make.wordpress.org/chat/):
https://wordpress.slack.com/archives/C5UNMSU4R/p1550231607150600

[aduth]

JSON.stringify is often used with DefinePlugin because the substitution is verbatim, and thus for something like a string, you'd want to be certain you're replacing it with a string, not an identifier token (the difference between foo( bar ); and foo( "bar" )). For a number, though, the verbatim substitution is fine, i.e. there's no difference with or without JSON.stringify (it's foo( 10 ) with or without JSON.stringify).

[aduth]

There's no error if this line is removed?