Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd feature flags for Phase 2 #13324
Conversation
talldan
self-assigned this
Jan 15, 2019
talldan
requested review from
gziolo and
noisysocks
Jan 15, 2019
noisysocks
requested a review
from
youknowriad
Jan 16, 2019
This comment has been minimized.
This comment has been minimized.
Thanks for getting started on this! Nothing moves the conversation forward like putting up some strawman code
@youknowriad and @gziolo should be able to give us some direction here once they're back from their meetup. Paging @WordPress/gutenberg-core, too.
We probably want to give this a more generic name as
My first reaction is that overloading Perhaps we could have two environment variables, e.g. Or perhaps we ought to ditch what Calypso does and go back to something like Riad first suggested, e.g.
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?
What's involved in getting |
This comment has been minimized.
This comment has been minimized.
True, hadn't thought about that. Let's see how this develops before renaming it.
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 A tricky aspect of handling this at build time is that (from my understanding) the integration with core has its own separate webpack setup:
Perhaps that leaves an option of a webpack or babel plugin as the best way to inject the correct flags at build time?
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. |
youknowriad
requested a review
from WordPress/gutenberg-core
Jan 16, 2019
This comment has been minimized.
This comment has been minimized.
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 ( |
This comment has been minimized.
This comment has been minimized.
I agree with a 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:
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, In short, we should be clear about our goals for feature flags:
|
This comment has been minimized.
This comment has been minimized.
I don't think it's necessary (yet) but it's worth noting that
Or for super experimental features one could even gate the feature to All unnecessary food for thought. My only point is that the idea scales fairly well |
This comment has been minimized.
This comment has been minimized.
Expanding on #13324 (comment), I thought I'd lay out a proposal for how the release process using
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 );
}
Considerations:
|
talldan
force-pushed the
try/feature-flags
branch
from
4cb0a39
to
53eb3a1
Jan 24, 2019
This comment has been minimized.
This comment has been minimized.
Based on the discussion, I've reworked this to use webpack's define plugin to inject a Dead code elimination seems to work. I did a quick test by setting the phase to
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 Thoughts on this approach? |
mcsf
closed this
Jan 24, 2019
mcsf
reopened this
Jan 24, 2019
This comment has been minimized.
This comment has been minimized.
Ouch, ignore my closing this issue. My keyboard magic with Vimium went a little too far. |
This comment has been minimized.
This comment has been minimized.
Thanks, this looks a lot nicer.
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 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 ) { |
This comment has been minimized.
This comment has been minimized.
@talldan and I were discussing this yesterday and we concluded that it's arguably fine that we publish code on npm that requires The alternative is that we modify
That's an interesting idea. I think I'm into it. At the very least it makes sense to forbid One consideration: What will it look like when we "ship" Phase 2 to WordPress Core? Would we simply remove all of the |
This comment has been minimized.
This comment has been minimized.
Assuming Some off-the-bat ideas:
I'm leaning towards (3). |
This comment has been minimized.
This comment has been minimized.
With the define plugin we can explicitly replace only
This is a bit crazy, but it's also possible to do something like this with the define plugin:
|
This comment has been minimized.
This comment has been minimized.
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 I did notice we already have a line of code that references |
This comment has been minimized.
This comment has been minimized.
I didn't think too hard about it, but I did go along with our prior art.
I also lean towards idea number 3, with whatever syntax fits best ( |
talldan
force-pushed the
try/feature-flags
branch
2 times, most recently
from
eef83e5
to
b7d2a5e
Jan 31, 2019
talldan
added some commits
Feb 5, 2019
talldan
force-pushed the
try/feature-flags
branch
from
a960fe0
to
57731f5
Feb 15, 2019
This comment has been minimized.
This comment has been minimized.
@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. |
This comment has been minimized.
This comment has been minimized.
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
merged commit c1235d4
into
master
Feb 15, 2019
1 check passed
gziolo
deleted the
try/feature-flags
branch
Feb 15, 2019
This comment has been minimized.
This comment has been minimized.
I shared some details about this PR in #core-js channel on WordPress Slack (link requires registration at https://make.wordpress.org/chat/): |
Feb 18, 2019
This was referenced
mukeshpanchal27
added a commit
to mukeshpanchal27/gutenberg
that referenced
this pull request
Feb 26, 2019
youknowriad
added a commit
that referenced
this pull request
Mar 6, 2019
youknowriad
added a commit
that referenced
this pull request
Mar 6, 2019
aduth
reviewed
Mar 7, 2019
new DefinePlugin( { | ||
// Inject the `GUTENBERG_PHASE` global, used for feature flagging. | ||
// eslint-disable-next-line @wordpress/gutenberg-phase | ||
'process.env.GUTENBERG_PHASE': JSON.stringify( parseInt( process.env.npm_package_config_GUTENBERG_PHASE, 10 ) || 1 ), |
This comment has been minimized.
This comment has been minimized.
aduth
Mar 7, 2019
Member
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
reviewed
Mar 7, 2019
@@ -105,6 +106,11 @@ const config = { | |||
], | |||
}, | |||
plugins: [ | |||
new DefinePlugin( { | |||
// Inject the `GUTENBERG_PHASE` global, used for feature flagging. | |||
// eslint-disable-next-line @wordpress/gutenberg-phase |
talldan commentedJan 15, 2019
•
edited
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
or2
) during the build.Within the codebase, the
GUTENBERG_PHASE
variable would be used to conditionally avoid executing code: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.