-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Include variations paths in build #63098
Scripts: Include variations paths in build #63098
Conversation
Size Change: -161 B (-0.01%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
daba1ca
to
3b18531
Compare
Flaky tests detected in f6335fe. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9941590851
|
To make this easier to test, we might want to consider fixing #63077 first. This will allow us to switch an existing Core block that currently uses Edit: It's a bit of a chicken-egg problem. Generally, unit-testing this is hard. Alternatively, we've considered using That script uses existing templates from the |
3b18531
to
f6335fe
Compare
f6335fe
to
9c48c06
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
d018c3e
to
5fca20b
Compare
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've been testing it and everything seems to work as expected 🙂 I left a few comments with the goal of learning more than considering them issues.
I've been testing it in an external plugin pointing to this version of wp-scripts
and this is what I tested:
- Using arrays in
variations
keep working as expected. - Using
variations
to point to a PHP file works as expected. I can see it include in thebuild
folder and variations appear in the editor inserter. - They work both with
build
andstart
. - The
render
file keeps working as expected. - I can include both a
render
file and avariations
file and they work as expected. - If I point a different property apart from
render
orvariation
to a PHP file, it isn't built. - Other non-PHP files like
view
or CSS work as expected.
function getRenderPropPaths() { | ||
return getPhpFilePaths( [ 'render' ] ); | ||
} |
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 assume we can't remove this function, right? Are these utils public? It seems getRenderPropPaths
is not used anywhere internally.
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 question. The function isn't exported from the @wordpress/scripts
package, but it seems like we have some precedent of recommending importing functions from a subdirectory of that package, e.g. here:
const defaultConfig = require( '@wordpress/scripts/config/webpack.config' );
This is the reason why I didn't remove it, however slim chances are that someone actually does this with getRenderPropPaths
.
However, your comment reminded me of the @wordpress/deprecated
package. Maybe I'll use that to annotate the function as deprecated, which should pave the way for removing it at a later time 🤔
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.
Okay 🙂 I agree probably nobody is using getRenderPropPaths
but the "compatibility" code is not complex either.
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.
Ah, it's a bit of a chicken-egg situation 😬 Seems like we can't require( '@wordpress/deprecated' )
in @wordpress/scripts
, as the latter package is needed to build the former 😅
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 might just keep it as-is for now (to ensure I'm not breaking any obscure third-party thing that's relying on this).
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 should be able to remove getRenderPropPaths
. We don't offer support for internal utils. I also don't expect this one would ever be necessary for 3rd party projects.
Overall, the idea always was that developers use mostly the scripts, but we started supporting also the default webpack config as it's where people tend to apply the changes the most often.
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.
Alright, I'll file a quick PR! Thank you for weighing in 😄
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 just a bit of a random thought but, is adding a field for each type of PHP file that might be needed the best path forward, instead of adding a more solution towards including any type of PHP file? {
"serverIncludes": [
"file:./variations.php",
"file:./components.php",
"file:./functions.php",
"..."
]
} I'm not sure how well adding individual fields per file type is going to scale if there are similar needs in the future. Again, just a random thought so happy to hear your opinions 🙂 |
Ah, TBH, that didn't really occur to me. I'm not sure we need (or should expect?) that many other server-side includes; plus it could be a bit tricky to have them depend on one another (e.g. The most compelling argument IMO is that for both In a similar vein, there's a defined requirement for AFAICT, the same cannot be said of more generic functions or components (as they aren't directly interfacing with the block). So for the time being, I don't see the need to change the established fields we're using for |
Remove function `getRenderPropPaths()` from the `@wordpress/scripts` package. As of #63098, it is no longer used anywhere in Core. It has also never been part of the package's public interface. Co-authored-by: ockham <[email protected]> Co-authored-by: gziolo <[email protected]>
Makes sense. Thanks for the extra context, @ockham 🙂 |
If a `block.json`'s `variations` field is set to a (PHP) file name, include that file in the build, i.e. prefix its functions with `gutenberg_` and copy the resulting file to the `build/block-library/blocks/` directory. Note that this applies to third-party blocks, but not Core blocks. Making it work for Core blocks is a separate issue: WordPress#63077. Co-authored-by: ockham <[email protected]> Co-authored-by: SantosGuillamot <[email protected]>
Remove function `getRenderPropPaths()` from the `@wordpress/scripts` package. As of WordPress#63098, it is no longer used anywhere in Core. It has also never been part of the package's public interface. Co-authored-by: ockham <[email protected]> Co-authored-by: gziolo <[email protected]>
What?
If a
block.json
'svariations
field is set to a (PHP) file name, include that file in the build, i.e. prefix its functions withgutenberg_
and copy the resulting file to thebuild/block-library/blocks/
directory.Note that this applies to third-party blocks, but not Core blocks. Making it work for Core blocks is a separate issue: #63077.
Why?
As of #62092, it is possible to set the
variations
field in a block'sblock.json
file to the name of a PHP file that generates the block's variations.How?
By extending the existing logic introduced for the
render
field (by #43917 and #51162) to encompassvariations
.Testing Instructions
block.json
file with avariations
field that points to a PHP file.cd example-variations/
../node_modules/.bin/wp-scripts build
.src/variations.php
has been copied to thebuild/
folder.variations
field insrc/block.json
to a variations array:"variations": [ { "name": "warning" } ]
.../node_modules/.bin/wp-scripts start
to begin development in watch mode.variations.php
file in thebuild/
folder.variations
field insrc/block.json
back to"variations": "file:./variations.php"
.variations.php
file should re-appear in thebuild/
folder.Patch