Build: Include Core blocks' render and variations files
#63311
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ockham
added
[Type] Build Tooling
|
Size Change: 0 B Total Size: 1.78 MB ℹ️ View Unchanged
|
ockham
force-pushed
the
add/render-files-to-core-blocks-build
branch
from
88caa11 to
445291f
Compare
ockham
commented
Jul 16, 2024
| * | ||
| * @type {string[]} | ||
| */ | ||
| static paths; |
There was a problem hiding this comment.
Probably need to change this so there won't be any collisions between class instances.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤔
ockham
commented
Jul 16, 2024
| @@ -90,6 +92,10 @@ module.exports = [ | |||
| plugins: [ | |||
| ...plugins, | |||
| new DependencyExtractionWebpackPlugin( { injectPolyfill: false } ), | |||
| new PhpFilePathsPlugin( { | |||
| context: './packages/block-library/src/', | |||
There was a problem hiding this comment.
Should probably add the other paths used as from arg to the CopyWebpackPlugin instance as well.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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/scriptsshould 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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll try to give it a round of testing this week before approving it.
ockham
changed the title
render files
ockham
force-pushed
the
add/render-files-to-core-blocks-build
branch
from
2714ff7 to
a4c7c06
Compare
ockham
changed the title
render filesrender and variations files
|
What's missing to have it ready for review? |
ockham
force-pushed
the
add/render-files-to-core-blocks-build
branch
from
20fe170 to
aa2ef5e
Compare
Really just the two things I've left notes for. Maybe you can weigh in on the first one; I can work on the second one. |
ockham
requested review from
ntwb,
nerrad,
ajitbohra and
ryanwelcher
as code owners
|
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. |
|
I can confirm that both 
|
gziolo
approved these changes
Sep 6, 2024
|
Looking at the |
ockham
commented
Sep 9, 2024
ockham
force-pushed
the
add/render-files-to-core-blocks-build
branch
from
e833e1e to
916ab9b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
If a Core block's
block.jsoncontains arenderand/orvariationsfield that points to a PHP file name, copy those files to the build directory.Why?
It's an attempt at fixing #63077.
How?
By reusing logic from the
@wordpress/scriptspackage.Testing Instructions
rendermethod fromindex.phpto a newly createdrender.php, and add arenderfield toblock.jsonthat points to thatrender.phpfile.)npm run start.packages/block-library/src/template-part/render.phphas been copied tobuild/block-library/blocks/template-part/render.php.Patch