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

Scripts: Change webpack configuration to include render files in the build folder #43917

Merged
merged 7 commits into from
Sep 9, 2022
+96 −18
Merged

Scripts: Change webpack configuration to include render files in the build folder #43917

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f20c355
Adapt WebPack to include render files in build
SantosGuillamot Sep 6, 2022
c0992c0
Ensure getRenderPropPaths returns always an array
SantosGuillamot Sep 7, 2022
291d942
Remove log already handled in entry points
SantosGuillamot Sep 7, 2022
8f7fd61
Update README.md
gziolo Sep 8, 2022
9e4dc01
Update CHANGELOG.md
gziolo Sep 8, 2022
71df6dd
Merge branch 'trunk' into fix/include-render-files-in-build
gziolo Sep 8, 2022
d532e2e
Inform that viewScript is not enqueued when using the render filed
luisherranz Sep 9, 2022
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
19 changes: 15 additions & 4 deletions packages/scripts/config/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const {
hasCssnanoConfig,
hasPostCSSConfig,
getWebpackEntryPoints,
getRenderPropPaths,
} = require( '../utils' );

const isProduction = process.env.NODE_ENV === 'production';
Expand All @@ -36,9 +37,8 @@ if ( ! browserslist.findConfig( '.' ) ) {
}
const hasReactFastRefresh = hasArgInCLI( '--hot' ) && ! isProduction;

const copyWebpackPatterns = process.env.WP_COPY_PHP_FILES_TO_DIST
? '**/{block.json,*.php}'
: '**/block.json';
// Get paths of the `render` props included in `block.json` files
const renderPaths = getRenderPropPaths();
luisherranz marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I use wordpress-scripts in our build system, and it doesn't work anymore. The whole thing is more complex, but basically, there is what it does: https://github.com/loxK/wordpress-scripts-test

import wpWebpackConfig from '@wordpress/scripts/config/webpack.config.js'
import CopyWebpackPlugin from 'copy-webpack-plugin'
import {resolve} from "path";

export default {
    ...wpWebpackConfig,
    entry: './dir/file.js',
    output: {
        filename: 'file.js',
        path: resolve( process.cwd(), 'assets/js' ),
    },
    plugins: [
        ...(( wpWebpackConfig?.plugins || [] ).filter(plugin => ! (plugin instanceof CopyWebpackPlugin))),

    ]
}
» npm run build                               

> build
> webpack

[webpack-cli] Failed to load '/home/www/test/webpack.config.js' config
[webpack-cli] TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
    at new NodeError (node:internal/errors:387:5)
    at validateString (node:internal/validators:121:11)
    at Object.join (node:path:1172:7)
    at fromProjectRoot (/home/www/test/node_modules/@wordpress/scripts/utils/file.js:13:7)
    at hasProjectFile (/home/www/test/node_modules/@wordpress/scripts/utils/file.js:16:14)
    at getRenderPropPaths (/home/www/test/node_modules/@wordpress/scripts/utils/config.js:312:9)
    at Object.<anonymous> (/home/www/test/node_modules/@wordpress/scripts/config/webpack.config.js:41:21)
    at Module._compile (node:internal/modules/cjs/loader:1126:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1180:10)
    at Module.load (node:internal/modules/cjs/loader:1004:32) {
  code: 'ERR_INVALID_ARG_TYPE'
}

Any clue on what to do to make it work again ?

Copy link
Member

Choose a reason for hiding this comment

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

Hey @loxK. I've just cloned your repository, done a npm install (using node 16) and npm run build, and it worked fine.

I'm not sure what may be going on. Could you please try removing the node_module folder and running npm install again? Or giving us more precise instructions to reproduce the problem?

Copy link
Contributor

@loxK loxK Sep 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You need to upgrade @wordpress/scripts to 24.1.0 by editing packages.json and npm i, in the repository the wordpress-scripts version is fixed to 24.0.0.

I upgraded it in the repository, git pull it and npm i

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha.

For some reason, process.env.WP_SRC_DIRECTORY is undefined in your configuration. I'll investigate why.

It wasn't failing before because you overwrote the entry points (which also access process.env.WP_SRC_DIRECTORY).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it's because you're using "build": "webpack" and not "build": "wp-scripts build" in your package.json.

wp-scripts build initializes process.env.WP_SRC_DIRECTORY:

gutenberg/packages/scripts/scripts/build.js

Line 27 in 0a0598f

process.env.WP_SRC_DIRECTORY = hasArgInCLI( '--webpack-src-dir' )

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, but I don't know the answer. You need to find a way to switch back to wp-scripts, or you'd have to take a look at its internals to understand what it's doing before it loads webpack to do that yourself. You're also welcome to open a PR if you think there's something broken inside wp-scripts 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Damn breaking change in a minor release version number. Hours of work ahead ...
That sucks.

Copy link
Member

Choose a reason for hiding this comment

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

Extending webpack is not covered with semver, especially when not used in conjunction with wp-scripts.

From the docs:

Future versions of this package may change what webpack and Babel plugins we bundle, default configs, etc.

Copy link
Member

Choose a reason for hiding this comment

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

By the way, if you have needs that wp-scripts don't support yet and you think they'd be useful for a wide range of people, I encourage you to open an issue/PR to add those improvements to wp-scripts 🙂

I also encourage you to open a PR if you think there's something broken or that could be done in a better way inside wp-scripts.

Copy link
Member

Choose a reason for hiding this comment

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

For reference, @loxK opened a PR to change the way the webpack config access the value of process.env.WP_SRC_DIRECTORY:


const cssLoaders = [
{
Expand Down Expand Up @@ -232,7 +232,7 @@ const config = {
new CopyWebpackPlugin( {
patterns: [
{
from: copyWebpackPatterns,
from: '**/block.json',
context: process.env.WP_SRC_DIRECTORY,
noErrorOnMissing: true,
transform( content, absoluteFrom ) {
Expand Down Expand Up @@ -265,6 +265,17 @@ const config = {
return content;
},
},
{
from: '**/*.php',
context: process.env.WP_SRC_DIRECTORY,
noErrorOnMissing: true,
filter: ( filepath ) => {
return (
process.env.WP_COPY_PHP_FILES_TO_DIST ||
renderPaths.includes( filepath )
gziolo marked this conversation as resolved.
Show resolved Hide resolved
);
},
},
],
} ),
// The WP_BUNDLE_ANALYZER global variable enables a utility that represents
Expand Down
66 changes: 66 additions & 0 deletions packages/scripts/utils/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,10 +302,76 @@ function getWebpackEntryPoints() {
};
}

/**
* Returns the list of paths included in the `render` props by scanning the `block.json` files.
*
* @return {Array} The list of all the `render` prop paths included in `block.json` files.
*/
function getRenderPropPaths() {
// Continue only if the source directory exists.
if ( ! hasProjectFile( process.env.WP_SRC_DIRECTORY ) ) {
log(
chalk.yellow(
`Source directory "${ process.env.WP_SRC_DIRECTORY }" was not found. Please confirm there is a "src" directory in the root or the value passed to --webpack-src-dir is correct.`
)
);
gziolo marked this conversation as resolved.
Show resolved Hide resolved
return {};
}

// Checks whether any block metadata files can be detected in the defined source directory.
const blockMetadataFiles = glob(
`${ process.env.WP_SRC_DIRECTORY }/**/block.json`,
{
absolute: true,
}
);

if ( blockMetadataFiles.length > 0 ) {
const srcDirectory = fromProjectRoot(
process.env.WP_SRC_DIRECTORY + sep
);

const renderPaths = [];
blockMetadataFiles.forEach( ( blockMetadataFile ) => {
const { render } = JSON.parse( readFileSync( blockMetadataFile ) );
if ( render && render.startsWith( 'file:' ) ) {
// Removes the `file:` prefix.
const filepath = join(
dirname( blockMetadataFile ),
render.replace( 'file:', '' )
);

// Takes the path without the file extension, and relative to the defined source directory.
if ( ! filepath.startsWith( srcDirectory ) ) {
log(
chalk.yellow(
`Skipping "${ render.replace(
'file:',
''
) }" listed in "${ blockMetadataFile.replace(
fromProjectRoot( sep ),
''
) }". File is located outside of the "${
process.env.WP_SRC_DIRECTORY
}" directory.`
)
);
return;
}
renderPaths.push( filepath );
}
} );
if ( Object.keys( renderPaths ).length > 0 ) {
return renderPaths;
}
}
}

module.exports = {
getJestOverrideConfigFile,
getWebpackArgs,
getWebpackEntryPoints,
getRenderPropPaths,
hasBabelConfig,
hasCssnanoConfig,
hasJestConfig,
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 @@ -14,6 +14,7 @@ const {
getJestOverrideConfigFile,
getWebpackArgs,
getWebpackEntryPoints,
getRenderPropPaths,
hasBabelConfig,
hasCssnanoConfig,
hasJestConfig,
Expand All @@ -34,6 +35,7 @@ module.exports = {
getPackageProp,
getWebpackArgs,
getWebpackEntryPoints,
getRenderPropPaths,
hasArgInCLI,
hasBabelConfig,
hasCssnanoConfig,
Expand Down