Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Build: Include Core blocks' render and variations files #63311

Merged
merged 11 commits into from
Sep 10, 2024
Merged

Build: Include Core blocks' render and variations files #63311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4487adf
Build: Include render files
ockham Jul 9, 2024
7dee693
Move PhpFilePathsPlugin to separate file
ockham Jul 16, 2024
8dc7a58
Re-use PhpFilePathsPlugin
ockham Jul 16, 2024
f1d7830
Parametrize context
ockham Jul 16, 2024
8f7b0c2
Fix import path
ockham Jul 16, 2024
1edc3b0
Fix paths
ockham Jul 16, 2024
5a635e9
Also copy variations PHP files
ockham Jul 17, 2024
aaa23d3
Update CHANGELOG.md
gziolo Sep 6, 2024
030c3c0
Update CHANGELOG.md
gziolo Sep 6, 2024
8a4fdaf
Revert "Update CHANGELOG.md"
ockham Sep 9, 2024
916ab9b
Revert "Update CHANGELOG.md"
ockham Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 5 additions & 46 deletions packages/scripts/config/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ const RtlCssPlugin = require( 'rtlcss-webpack-plugin' );
const TerserPlugin = require( 'terser-webpack-plugin' );
const { realpathSync } = require( 'fs' );
const { sync: glob } = require( 'fast-glob' );
const { validate } = require( 'schema-utils' );

/**
* WordPress dependencies
Expand All @@ -32,11 +31,11 @@ const {
hasPostCSSConfig,
getWordPressSrcDirectory,
getWebpackEntryPoints,
getPhpFilePaths,
getAsBooleanFromENV,
getBlockJsonModuleFields,
getBlockJsonScriptFields,
fromProjectRoot,
PhpFilePathsPlugin,
} = require( '../utils' );

const isProduction = process.env.NODE_ENV === 'production';
Expand All @@ -50,49 +49,6 @@ const hasExperimentalModulesFlag = getAsBooleanFromENV(
'WP_EXPERIMENTAL_MODULES'
);

const phpFilePathsPluginSchema = {
type: 'object',
properties: {
props: {
type: 'array',
items: {
type: 'string',
},
},
},
};

/**
* The plugin recomputes PHP file paths once on each compilation. It is necessary to avoid repeating processing
* when filtering every discovered PHP file in the source folder. This is the most performant way to ensure that
* changes in `block.json` files are picked up in watch mode.
*/
class PhpFilePathsPlugin {
/**
* PHP file paths from `render` and `variations` props found in `block.json` files.
*
* @type {string[]}
*/
static paths;

constructor( options = {} ) {
validate( phpFilePathsPluginSchema, options, {
name: 'PHP File Paths Plugin',
baseDataPath: 'options',
} );

this.options = options;
}

apply( compiler ) {
const pluginName = this.constructor.name;

compiler.hooks.thisCompilation.tap( pluginName, () => {
this.constructor.paths = getPhpFilePaths( this.options.props );
} );
}
}

const cssLoaders = [
{
loader: MiniCSSExtractPlugin.loader,
Expand Down Expand Up @@ -345,7 +301,10 @@ const scriptConfig = {
cleanStaleWebpackAssets: false,
} ),

new PhpFilePathsPlugin( { props: [ 'render', 'variations' ] } ),
new PhpFilePathsPlugin( {
context: getWordPressSrcDirectory(),
props: [ 'render', 'variations' ],
} ),
new CopyWebpackPlugin( {
patterns: [
{
Expand Down
13 changes: 7 additions & 6 deletions packages/scripts/utils/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,22 +351,23 @@ function getWebpackEntryPoints( buildType ) {
/**
* Returns the list of PHP file paths found in `block.json` files for the given props.
*
* @param {string[]} props The props to search for in the `block.json` files.
* @param {string} context The path to search for `block.json` files.
* @param {string[]} props The props to search for in the `block.json` files.
* @return {string[]} The list of PHP file paths.
*/
function getPhpFilePaths( props ) {
function getPhpFilePaths( context, props ) {
// Continue only if the source directory exists.
if ( ! hasProjectFile( getWordPressSrcDirectory() ) ) {
if ( ! hasProjectFile( context ) ) {
return [];
}

// Checks whether any block metadata files can be detected in the defined source directory.
const blockMetadataFiles = glob( '**/block.json', {
absolute: true,
cwd: fromProjectRoot( getWordPressSrcDirectory() ),
cwd: fromProjectRoot( context ),
} );

const srcDirectory = fromProjectRoot( getWordPressSrcDirectory() + sep );
const srcDirectory = fromProjectRoot( context + sep );

return blockMetadataFiles.flatMap( ( blockMetadataFile ) => {
const blockJson = JSON.parse( readFileSync( blockMetadataFile ) );
Expand Down Expand Up @@ -396,7 +397,7 @@ function getPhpFilePaths( props ) {
) }" listed in "${ blockMetadataFile.replace(
fromProjectRoot( sep ),
''
) }". File is located outside of the "${ getWordPressSrcDirectory() }" directory.`
) }". File is located outside of the "${ context }" directory.`
)
);
continue;
Expand Down
2 changes: 2 additions & 0 deletions packages/scripts/utils/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const {
getBlockJsonModuleFields,
getBlockJsonScriptFields,
} = require( './block-json' );
const { PhpFilePathsPlugin } = require( './php-file-paths-plugin' );

module.exports = {
fromProjectRoot,
Expand All @@ -55,5 +56,6 @@ module.exports = {
hasPostCSSConfig,
hasPrettierConfig,
hasProjectFile,
PhpFilePathsPlugin,
spawnScript,
};
60 changes: 60 additions & 0 deletions packages/scripts/utils/php-file-paths-plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/**
* External dependencies
*/
const { validate } = require( 'schema-utils' );

/**
* Internal dependencies
*/
const { getPhpFilePaths } = require( './config' );

const phpFilePathsPluginSchema = {
type: 'object',
properties: {
context: {
type: 'string',
},
props: {
type: 'array',
items: {
type: 'string',
},
},
},
};

/**
* The plugin recomputes PHP file paths once on each compilation. It is necessary to avoid repeating processing
* when filtering every discovered PHP file in the source folder. This is the most performant way to ensure that
* changes in `block.json` files are picked up in watch mode.
*/
class PhpFilePathsPlugin {
/**
* PHP file paths from `render` and `variations` props found in `block.json` files.
*
* @type {string[]}
*/
static paths;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably need to change this so there won't be any collisions between class instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm being overly cautious here. My concern is basically that if these paths are stored as a static class variable, they might get polluted. However, I'm not sure if the latter is realistic. While I can think of npm run dev running in parallel with another job that builds some 3rd-party package, that might not be enough to pollute class-level vars.

(OTOH, making it non-static -- in the context of a Webpack plugin -- seems to be non-trivial.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked exactly the same before and no issues were reported. As far as I understand it, the webpack config is executed only once when you run wp-scripts start. It's when the plugin get instatiated, all subsequent changes to paths happen through th webpack lifecycle events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked exactly the same before and no issues were reported.

It wasn't used for Core blocks -- i.e. Gutenberg code itself -- before, so my concern was vaguely that e.g. running the GB build in watch mode might collide with running a wp-scripts command to build a third-party block.

As far as I understand it, the webpack config is executed only once when you run wp-scripts start. It's when the plugin get instatiated, all subsequent changes to paths happen through th webpack lifecycle events.

Yeah, I guess we should be fine 🤔


constructor( options = {} ) {
validate( phpFilePathsPluginSchema, options, {
name: 'PHP File Paths Plugin',
baseDataPath: 'options',
} );

this.options = options;
}

apply( compiler ) {
const pluginName = this.constructor.name;

compiler.hooks.thisCompilation.tap( pluginName, () => {
this.constructor.paths = getPhpFilePaths(
this.options.context,
this.options.props
);
} );
}
}

module.exports = { PhpFilePathsPlugin };
37 changes: 29 additions & 8 deletions tools/webpack/blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
const CopyWebpackPlugin = require( 'copy-webpack-plugin' );
const { join, sep } = require( 'path' );
const fastGlob = require( 'fast-glob' );
const { realpathSync } = require( 'fs' );

/**
* WordPress dependencies
*/
const DependencyExtractionWebpackPlugin = require( '@wordpress/dependency-extraction-webpack-plugin' );
const { PhpFilePathsPlugin } = require( '@wordpress/scripts/utils' );

/**
* Internal dependencies
Expand Down Expand Up @@ -90,6 +92,10 @@ module.exports = [
plugins: [
...plugins,
new DependencyExtractionWebpackPlugin( { injectPolyfill: false } ),
new PhpFilePathsPlugin( {
context: './packages/block-library/src/',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably add the other paths used as from arg to the CopyWebpackPlugin instance as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an internal plugin that will never be used outside so I don't think we need to spend more time on it if it works as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you did. It's also imported to use within the Gutenberg plugin. Hmm, that complicates it a little bit. In general, @wordpress/scripts should only offer scripts and default configs. We discourage using internal utils and extending configs as they are subject to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant with my comment is that the CopyWebpackPlugin initialization includes other directories that might contain blocks (not just packages/block-library/src, but also packages/edit-widgets/src/blocks and packages/widgets/src/blocks):

gutenberg/tools/webpack/blocks.js

Lines 121 to 128 in 18164f0

Object.entries( {
'./packages/block-library/src/':
'build/block-library/blocks/',
'./packages/edit-widgets/src/blocks/':
'build/edit-widgets/blocks/',
'./packages/widgets/src/blocks/':
'build/widgets/blocks/',
} ).flatMap( ( [ from, to ] ) => [

AFAICS, it's just three blocks total across those two directories, but in theory, they could at some point include a render or variations field in one of their block.jsons, which is why I said we might want to include them in PhpFilePathsPlugin as well.

On the downside, this would make the logic in PhpFilePathsPlugin even more complicated 😕 So maybe it's okay to omit them.

Ah, I see what you did. It's also imported to use within the Gutenberg plugin. Hmm, that complicates it a little bit. In general, @wordpress/scripts should only offer scripts and default configs. We discourage using internal utils and extending configs as they are subject to change.

Hmm, are you saying we shouldn't altogether be doing what we're doing in this PR? AFAICS, we have some instances where we're doing "deep" imports from @wordpress/scripts in other parts of GB, e.g. this. (I thought deep imports might be fine, especially if the relevant symbols aren't exported publicly at package level?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep imports work perfectly fine. It’s manageable inside the Gutenberg monorepo as breaking changes have a high chance of bubbling up during the development. However it should be discouraged for usage through npm. In effect, we should never support requests to make it backward compatible on that level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS, it's just three blocks total across those two directories, but in theory, they could at some point include a render or variations field in one of their block.jsons, which is why I said we might want to include them in PhpFilePathsPlugin as well.

Got it! Initially, I didn’t think about it because I assumed these changes apply exclusively to the @wordpress/scripts package. I personally would be in favor of landing changes in the current shape and try a follow up if you feel strongly about covering blocks located in different packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'd be more than happy to land this without further ado. If anything, I was trying to gauge if this has the potential of breaking things, but I guess that chance is pretty minimal.

I'll set the PR as ready for review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to give it a round of testing this week before approving it.

props: [ 'render', 'variations' ],
} ),
new CopyWebpackPlugin( {
patterns: [].concat(
[
Expand Down Expand Up @@ -127,17 +133,32 @@ module.exports = [
'build/widgets/blocks/',
} ).flatMap( ( [ from, to ] ) => [
{
from: `${ from }/**/index.php`,
from: `${ from }/**/*.php`,
to( { absoluteFilename } ) {
const [ , dirname ] = absoluteFilename.match(
new RegExp(
`([\\w-]+)${ escapeRegExp(
sep
) }index\\.php$`
const [ , dirname, basename ] =
absoluteFilename.match(
new RegExp(
`([\\w-]+)${ escapeRegExp(
sep
) }([\\w-]+)\\.php$`
)
);

if ( basename === 'index' ) {
return join( to, `${ dirname }.php` );
}
return join( to, dirname, `${ basename }.php` );
},
filter: ( filepath ) => {
return (
filepath.endsWith( sep + 'index.php' ) ||
PhpFilePathsPlugin.paths.includes(
realpathSync( filepath ).replace(
/\\/g,
'/'
)
)
);

return join( to, `${ dirname }.php` );
},
transform: ( content ) => {
const prefix = 'gutenberg_';
Expand Down
Loading