Expand Feature flags #2591
Expand Feature flags #2591
Conversation
senadir
force-pushed
the
add/more-feature-flags
branch
from
afe1a4a to
00d5fff
Compare
update this rule to match the new flags.update this rule to match the new flags. https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/00d5fff13f57fb6956ff7bcc50a1c64193f9fd27/bin/eslint-plugin-woocommerce/rules/feature-flag.js#L38-L41🚀 This comment was generated by the automations bot based on a
|
|
Size Change: +1.75 kB (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
senadir
requested review from
mikejolley and
nerrad
and removed request for
a team
senadir
added
the
skip-changelog
| @@ -51,4 +51,6 @@ const settings = { | |||
| }, | |||
| }; | |||
|
|
|||
| registerBlockType( 'woocommerce/cart', settings ); | |||
| if ( WOOCOMMERCE_BLOCKS_PHASE > 1 ) { | |||
There was a problem hiding this comment.
Do you think it would be useful to add a util function for this that supports named properties? e.g. is_build( 'experimental' )
There was a problem hiding this comment.
yeah now that we don’t have dead code elimination this could be useful, and it could help enforce tighter guidelines like throwing an error if the flag you provided doesn’t exist.
There was a problem hiding this comment.
I think if we're going to add convenience utility functions, it'd be nicer to do something like isExperimentalBuild() or isStableBuild().
There was a problem hiding this comment.
where should those methods live 🤔
There was a problem hiding this comment.
where should those methods live
Good question! Maybe block settings (since that's where WOOCOMMERCE_BLOCKS_PHASE comes from)?
| @@ -51,4 +51,6 @@ const settings = { | |||
| }, | |||
| }; | |||
|
|
|||
| registerBlockType( 'woocommerce/cart', settings ); | |||
| if ( WOOCOMMERCE_BLOCKS_PHASE > 1 ) { | |||
| registerBlockType( 'woocommerce/cart', settings ); | |||
There was a problem hiding this comment.
We could instead add a wrapper around registerBlockType that supports a phase argument. It might make registration easier and improve DRY, and remove the need to import the settings everywhere.
There was a problem hiding this comment.
Love this! Maybe registerGatedBlockType?
| // eslint-disable-next-line woocommerce/feature-flag | ||
| process.env.WOOCOMMERCE_BLOCKS_PHASE || 'experimental' | ||
| ), | ||
| } ), | ||
| ], | ||
| resolve, | ||
| }; |
There was a problem hiding this comment.
Might want to get someone else to review webpack who knows more :)
src/Assets.php
Outdated
|
|
||
| if ( 'experimental' === WOOCOMMERCE_BLOCKS_PHASE ) { | ||
| if ( WOOCOMMERCE_BLOCKS_PHASE > 2 ) { |
There was a problem hiding this comment.
Again a corresponding PHP utility function might be useful. It's also clearer to read code if the phases are named - but you don't need to remove the numbered phases from behind the scenes, they are fine.
| if ( WOOCOMMERCE_BLOCKS_PHASE > 1 ) { | ||
| self::register_script( 'wc-checkout-block', plugins_url( self::get_block_asset_build_path( 'checkout' ), __DIR__ ), $block_dependencies ); | ||
| self::register_script( 'wc-cart-block', plugins_url( self::get_block_asset_build_path( 'cart' ), __DIR__ ), $block_dependencies ); | ||
| } |
There was a problem hiding this comment.
At some point we should move script registration to the block classes - I always forget to add them here :)
There was a problem hiding this comment.
Agreed, there's a bunch of things I'd like to do around that eventually (including incorporating using block.json files which can contain all the things around the blocks and thus make the initialization code more dynamic).
| } | ||
| define( 'WOOCOMMERCE_BLOCKS_PHASE', $flag ); | ||
|
|
||
| $allowed_flags = [ '1', '2', '3' ]; |
There was a problem hiding this comment.
Is there a reason we use strings here instead of numbers?
There was a problem hiding this comment.
Environment variables and ini values are treated as strings, to avoid casting and recasting things all over the place, I assume everything is a string and when I define the flag I turn it to an int.
| @@ -20,6 +20,7 @@ | |||
|
|
|||
| $minimum_wp_version = '5.2'; | |||
|
|
|||
| define( 'WC_BLOCKS_IS_FEATURE_PLUGIN', true ); | |||
There was a problem hiding this comment.
Maybe check if this is defined first just in case.
There was a problem hiding this comment.
Agreed! That would also allow for overriding in wp-config.php if necessary.
| @@ -51,4 +51,6 @@ const settings = { | |||
| }, | |||
| }; | |||
|
|
|||
| registerBlockType( 'woocommerce/cart', settings ); | |||
| if ( WOOCOMMERCE_BLOCKS_PHASE > 1 ) { | |||
There was a problem hiding this comment.
I think if we're going to add convenience utility functions, it'd be nicer to do something like isExperimentalBuild() or isStableBuild().
| @@ -51,4 +51,6 @@ const settings = { | |||
| }, | |||
| }; | |||
|
|
|||
| registerBlockType( 'woocommerce/cart', settings ); | |||
| if ( WOOCOMMERCE_BLOCKS_PHASE > 1 ) { | |||
| registerBlockType( 'woocommerce/cart', settings ); | |||
There was a problem hiding this comment.
Love this! Maybe registerGatedBlockType?
| }; | ||
|
|
||
| const mainEntry = | ||
| process.env.WOOCOMMERCE_BLOCKS_PHASE === 'stable' | ||
| // env variables are strings, so we compare against a string, so we need to parse it. | ||
| parseInt( process.env.WOOCOMMERCE_BLOCKS_PHASE, 10 ) < 3 | ||
| ? stableMainEntry |
There was a problem hiding this comment.
So this means that we still have dead code removal on releases for anything marked experimental right? Might be a good idea to also check that process.env.WOOCOMMERCE_BLOCKS_PHASE is defined before using here? If it's undefined won't this cause a ReferenceError (especially since it looks like we no longer define a default for process.env.WOOCOMMERCE_BLOCKS_PHASE)?
There was a problem hiding this comment.
It shouldn’t cause a Reference Error because process.env is always defined, WOOCOMMERCE_BLOCKS_PHASE not being defined will result in experimental files being generated which is safer than them not being generated right?
There was a problem hiding this comment.
I am okay with experimental files being built if WOOCOMMERCE_BLOCKS_PHASE is not defined, that's not what I'm calling out here. I thought we might have an issue if parseInt returned NaN but it looks like we won't:
So all is okay here (assuming node behaves the same).
| if ( WOOCOMMERCE_BLOCKS_PHASE > 1 ) { | ||
| self::register_script( 'wc-checkout-block', plugins_url( self::get_block_asset_build_path( 'checkout' ), __DIR__ ), $block_dependencies ); | ||
| self::register_script( 'wc-cart-block', plugins_url( self::get_block_asset_build_path( 'cart' ), __DIR__ ), $block_dependencies ); | ||
| } |
There was a problem hiding this comment.
Agreed, there's a bunch of things I'd like to do around that eventually (including incorporating using block.json files which can contain all the things around the blocks and thus make the initialization code more dynamic).
| @@ -20,6 +20,7 @@ | |||
|
|
|||
| $minimum_wp_version = '5.2'; | |||
|
|
|||
| define( 'WC_BLOCKS_IS_FEATURE_PLUGIN', true ); | |||
There was a problem hiding this comment.
Agreed! That would also allow for overriding in wp-config.php if necessary.
senadir
force-pushed
the
add/more-feature-flags
branch
from
5f19afa to
a5b647d
Compare
|
This is ready for another review now. |
|
The last commits adds:
registerExperimentalBlockType( name, settings ) // returns the block or undefined
registerFeaturePluginBlockType( name, settings ) // returns the block or undefined
isExperimentalBuild() // returns Boolean
isFeaturePluginBuild() // returns Booleanit also adds helpers in PHP Package::is_experimental_build(); // returns Boolean
Package::is_feature_plugin_build(); // returns Boolean |
There was a problem hiding this comment.
Looking really good Nadir 👏 ! Thanks for handling all the feedback and steadily improving this pull.
I've got a few comments and the only one I'm really concerned about the most is the new methods in src/Package.php. Once we get that comment addressed this should be ready to go.
| @@ -0,0 +1,57 @@ | |||
| /** | |||
There was a problem hiding this comment.
👍 on putting these in their own file (and the name is great)!
| /** | ||
| * External dependencies | ||
| */ | ||
| import { registerBlockType } from '@wordpress/blocks'; |
There was a problem hiding this comment.
One concern I have here is that we are now loading this api on the frontend for anything using block-settings right? is that problematic? I think I might have been the one who suggested putting this api in block-settings so that could be my fault here.
It doesn't look like it's affected build sizes significantly, so maybe this would be okay (since @woocommerce/block-settings is essentially just an alias for the path and thus it should be tree-shakeable).
There was a problem hiding this comment.
I had similar conerns, but I'm assuming this is tree shakeable, bundle bot didn’t report any extra weight so It’s safe.
| * | ||
| * @return {?Object} The block, if it has been successfully registered; | ||
| * otherwise `undefined`. | ||
| */ |
There was a problem hiding this comment.
Thanks for being so thorough with the comment blocks here 👏 !
nerrad
added
the
status: blocker
senadir
force-pushed
the
add/more-feature-flags
branch
from
5850dd0 to
6a93d15
Compare
This reverts commit 25a2964.
senadir
force-pushed
the
add/more-feature-flags
branch
from
6be775f to
43572e7
Compare
closes #2451
This PR changes and expand our feature flags to make them more dynamic and to work with the core/plugin/dev model, so it does the following:
It changes our flags names from
stableandexperimentalto numbers, this is a requirement so we can have collapsable flags, so we have 3 flags:experimentalis now3.pluginis now2.coreis now1.for most cases you always compare up, this means if you want something to work only in dev you check if the flag is bigger than 2
WOOCOMMERCE_BLOCKS_PHASE > 2, if you want something to be in the feature plugin you compare for bigger than 1WOOCOMMERCE_BLOCKS_PHASE > 1.It updates our build process slightly, core and stable files and blocks should live
stableMainEntryandstableFrontEndEntry, this means things like Cart, Checkout, and other blocks, new unstable blocks like Single Product should live inexperimentalMainEntryorexperimentalFrontEndEntry, those files are only built if you runnpm run buildandnpm start, they won't be built if you runnpm run deploy, you can force not building them locally by passing a different flag soWOOCOMMERCE_BLOCKS_PHASE=2 npm run build.There's now a constant named
WC_BLOCKS_IS_FEATURE_PLUGIN, it should be defined if we're in running feature plugin and won't be defined when running from core, in most cases, you won't use this constant.in PHP you will still use
WOOCOMMERCE_BLOCKS_PHASEto define your features, same as PHP with same exact flags.In JS, you won’t be using
process.env.WOOCOMMERCE_BLOCKS_PHASEanymore, you will have to useWOOCOMMERCE_BLOCKS_PHASEa constant exported from blocks settingswoocommerce/feature-flageslint rule is disabled until we merge this and update it.Our deployment command
npm run deploywill now appendWOOCOMMERCE_BLOCKS_PHASE=2this means when you deploy, it will work on the context of feature plugin, but WooCommrce core will consume it on the context of Core automatically.How should you gate your code?
In most cases, you will have code that is:
WOOCOMMERCE_BLOCKS_PHASE.experimental, it will be either:experimentalMainEntryorexperimentalFrontEndEntry.WOOCOMMERCE_BLOCKS_PHASE.Test instructions:
Build the plugin normally
npm run build, you should be able to add any block, including Cart and Checkout and Single Product.Build the plugin in feature plugin mode
WOOCOMMERCE_BLOCKS_PHASE=2 npm run build, you should be able to add any block, including Cart and Checkout but not Single Product.Build plugin in Core mode
WOOCOMMERCE_BLOCKS_PHASE=1 npm run build, you should be able to add our core blocks, and not Cart, Checkout, or Single Product.If you want to further test, you can try to create a zip
WOOCOMMERCE_BLOCKS_PHASE=2 npm run package-pluginto generate a zip that you can upload to other websites, it would simulate a feature plugin.