-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Conversation
Size Change: +745 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick look, and I left some initial feedback. This is going to be an excellent addition ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, Mario 🙂
I just have a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Mario! 🙂
Excellent, this looks great and is more than enough for v1. We probably should also update https://github.com/WordPress/gutenberg/tree/trunk/packages/scripts#build and https://github.com/WordPress/gutenberg/tree/trunk/packages/scripts#start to reflect all the changes. Let me try to apply changes directly to the branch. |
render
files in the build folder
@SantosGuillamot and @luisherranz, I drafted some basic documention changes. Feel free to apply any updates you think would make it better. It's good to go from my perspective 🎉 |
@@ -632,7 +632,7 @@ return array( | |||
Starting in the WordPress 5.8 release, it is possible to instruct WordPress to enqueue scripts and styles for a block type only when rendered on the frontend. It applies to the following asset fields in the `block.json` file: | |||
|
|||
- `script` | |||
- `viewScript` (when the block defines `render_callback` during registration in PHP, then the block author is responsible for enqueuing the script) | |||
- `viewScript` (when the block defines `render_callback` during registration in PHP or a `render` field in its `block.json`, then the script is registered but the block author is responsible for enqueuing it) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍🏻
Aside: Sharing for inspiration. It would be great to remove that limitation/exception and offer an alternative way to enqueue those view scripts on demand. Related Track ticket: https://core.trac.wordpress.org/ticket/56470.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, something like that would be neat 🙂
@@ -93,13 +97,13 @@ This is how you execute the script with presented setup: | |||
|
|||
- `npm run build` - builds the code for production. | |||
- `npm run build:custom` - builds the code for production with two entry points and a custom output directory. Paths for custom entry points are relative to the project root. | |||
- `npm run build:copy-php` - builds the code for production and opts into copying PHP files from the `src` directory and its subfolders to the output directory. | |||
- `npm run build:copy-php` - builds the code for production and opts into copying all PHP files from the `src` directory and its subfolders to the output directory. By default, only PHP files listed in the `render` field in the detected `block.json` files get copied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, only PHP files listed in the
render
field in the detectedblock.json
files get copied.
As I understand from #42430 the new render
field in block.json
only accepts a path for just one file.
Shouldn't this say something like...
By default, only the PHP file defined for the
render
field (if any) in the detectedblock.json
get copied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be multiple block.json
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it's perfect in the way it's written 👍
@@ -368,14 +376,14 @@ This is how you execute the script with presented setup: | |||
- `npm start` - starts the build for development. | |||
- `npm run start:hot` - starts the build for development with "Fast Refresh". The page will automatically reload if you make changes to the files. | |||
- `npm run start:custom` - starts the build for development which contains two entry points and a custom output directory. Paths for custom entry points are relative to the project root. | |||
- `npm run start:copy-php` - starts the build for development and opts into copying PHP files from the `src` directory and its subfolders to the output directory. | |||
- `npm run start:copy-php` - starts the build for development and opts into copying all PHP files from the `src` directory and its subfolders to the output directory. By default, only PHP files listed in the `render` field in the detected `block.json` files get copied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, only PHP files listed in the
render
field in the detectedblock.json
files get copied.
As I understand from #42430 the new render
field in block.json
only accepts a path for just one file.
Shouldn't this say something like...
By default, only the PHP file defined for the
render
field (if any) in the detectedblock.json
get copied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see what you mean. It's hard to explain as it's one file per render,
but it might be in multiple block.json
files processed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as #43917 (comment)
In the meantime, I opened PR against WordPress core to ensure this feature lands in the upcoming 6.1 release: WordPress/wordpress-develop#3222 😄 |
? '**/{block.json,*.php}' | ||
: '**/block.json'; | ||
// Get paths of the `render` props included in `block.json` files | ||
const renderPaths = getRenderPropPaths(); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
).
There was a problem hiding this comment.
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
:
process.env.WP_SRC_DIRECTORY = hasArgInCLI( '--webpack-src-dir' ) |
There was a problem hiding this comment.
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
🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
Future versions of this package may change what webpack and Babel plugins we bundle, default configs, etc.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
:
What?
Thanks to this recent Pull Request, now you can use a PHP file, defined in the
render
prop, instead of the traditionalrender_callback
function. This PR aims to add that file automatically to the build folder, which isn't happening right now.Why?
As discussed here, right now it seems possible to do it manually, but it should work automatically.
How?
We changed the WebPack configuration to detect and copy the
.php
file defined in theblock.json
:webpack.config.js
file to differentiate between JS and PHP files. In this second case, we are just filtering the PHP files to include only the one defined in therender
prop, unlessprocess.env.WP_COPY_PHP_FILES_TO_DIST
is set to true.getRenderPropPaths
), to retrieve the file paths of all therender
props defined in the blocks. This is used to compare in thewebpack.config.js
file.Thanks @luisherranz for the help figuring out how this should work.
Testing Instructions
Not sure what is the best way to test it in the Gutenberg plugin. I found this problem working on custom blocks in an external plugin (in the BHE repo), and I tested the code there. After adding this code, the
render.php
files were included in the build folder.