Declare blocks as __experimental in block.json to automate syncing Gutenberg packages to WordPress #40655
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
adamziel
requested review from
gziolo,
ntwb,
nerrad and
ajitbohra
as code owners
gziolo
added
[Package] Block library
|
Size Change: +148 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
adamziel
changed the title
adamziel
changed the title
adamziel
force-pushed
the
try/block-library-babel-plugin
branch
from
db90945
to
9cb8fb8
Compare
gziolo
reviewed
Apr 29, 2022
gziolo
reviewed
Apr 29, 2022
There was a problem hiding this comment.
This looks great. We are almost there. Excellent work. I left a couple of nitpicks to process. I still need to look at the Babel plugin part which is the most complex one (or seems so when you don't write plugins too often).
By the way, the test plan for the production build where IS_GUTENBERG_PLUGIN is set to false is only a test for the WordPress core because it won't be ever a case in the plugin. Just noting in case it confused anyone. We could update webpack now to read block.json and optimize the output but it would never be useful in the plugin. However, it's exactly what we plan to do in the WordPress core next 🎉
|
@gziolo I addressed all your comments, this one is good for a re-review now. |
gziolo
reviewed
Apr 29, 2022
4 tasks
jsnajdr
reviewed
May 5, 2022
There was a problem hiding this comment.
One downside of adding new Babel plugins is that it makes it harder to migrate to another transpiler like swc.
Maybe a webpack plugin could do the same job. We are inspecting import statements, and that's close to webpack competencies. If a block import is experimental, replace the import with null, and filter the getAllBlocks array with _.compact or .filter( Boolean )? Sounds doable to me.
Isn't it the same issue if we use webpack like when using Babel but for a different part of the build process? We would have to run the plugin that removes code with experimental blocks in the WordPress core instead. I guess the other difference would be that we would publish to npm all blocks without the feature flag guard. By the way, |
Yes, it's just that short-term, replacing Babel with swc is more likely than replacing webpack with another bundler.
That's true, I forgot that the files we publish in NPM are not processed by webpack at all. That makes the webpack option much less attractive. My thinking was that the Babel plugin looks at |
Agreed 😄
That's a fair point. It's also the question whether we find an alternative way for exposing blocks from the library for 3rd party projects. It's a separate issue though. I don't mind we treat the Babel approach a temporary measure until we come up with a more flexible approach where you can import individual blocks (or block presets) and they get registered without too much boilerplate. |
This sounds nice as a next step. There's a bunch of conceptual problems to consider, such as how to treat the statements and expressions where these imports are used. Perhaps that wouldn't be too different from what this PR is doing, but I can see it turning into a larger discussion. Which is to say – let's start with the Babel plugin for now. |
adamziel
force-pushed
the
try/block-library-babel-plugin
branch
from
0581f21
to
bae9eb4
Compare
gziolo
reviewed
May 5, 2022
My hypothetical webpack plugin would resolve the imports as The Babel plugin modifies the code where the imported identifier is used, i.e., modifies syntax, which I think would be over the line for a webpack plugin.
Agreed, I'm totally not demanding that you rewrite the entire PR, just thinking about it loud 🙂 |
Good thinking. I appreciate your feedback here. It only assures me that we need to seek for more granular structure for consuming core blocks from the package so we can't get rid of the Babel plugin just introduced 👍🏻 |
|
Tested with:
I executed and later |
@jsnajdr – thank you for that, you are on to something here. I like the idea of resolving all such imports to @gziolo it looks like it completely removed what's between the parentheses here: etc That's weird and completely unexpected. Also, it doesn't happen on my end! Just to confirm, do you have the latest npm dependencies installed? |
|
I've just implemented the |
gziolo
added
[Type] Code Quality
|
With "config": {
"IS_GUTENBERG_PLUGIN": false
},
With "config": {
"IS_GUTENBERG_PLUGIN": true
},
Everything works as expected after I removed |
gziolo
approved these changes
May 6, 2022
gziolo
reviewed
May 6, 2022
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
…/gutenberg into try/block-library-babel-plugin
|
All green 🎉 |
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 problem is this PR trying to solve?
I'd like to automate syncing Gutenberg changes to the WordPress repository.
Currently it requires manually checking which blocks were:
There is no easy way to extract the information about stable/experimental blocks from the block library in the CLI environment.
How does this PR proposes to solve it?
There are two parts:
A new
__experimentalflag in the block.json:block.jsonflag is clearer than moving variable names between the lists.For example:
post-author-name/block.json:
post-comment/block.json:
A babel plugin:
One downside of introducing
getAllBlocks()is that it breaks dead code elimination.Through static analysis, Webpack knows to remove blocks like
if( process.env.IS_GUTENBERG_PLUGIN === true) {whenIS_GUTENBERG_PLUGINis known to be false. This PR, however, moves the experimental blocks from inside of thatifstatement to the same list where stable blocks reside. As a result, the unused experimental blocks are bundled in the built assets.This can be fixed via a babel plugin as follows (thanks @gziolo @jsnajdr!):
block.jsonmetadata.So that on build, Webpack can remove the post-author-name block from the bundle.
Other notes
I considered updating the block.json API reference, but it doesn't seem right – this is a private property that makes sense only for core blocks in the Gutenberg repository. I documented it inside of
block-library/README.mdinstead.Test plan
The development build:
npm run devand confirm the site editor works in the browser and the experimental blocks are available in the inserter.npm run build iand confirm thatbuild/block-library/index.jsstill contains the experimental blocks likepostAuthorName.The production build:
(for the WordPress core merge scenario, it won't be ever a case in the plugin)
IS_GUTENBERG_PLUGINtofalsenpm run build iand confirm thatbuild/block-library/index.jsdoes not contain the experimental blocks likepostAuthorName. Note that thebuild/block-library/blocks/post-author-namestill exists, this is expected and how it works even without this PR.build:plugin-zip, install the builtgutenberg.zipplugin on a clean WordPress install, confirm it works and does not offer any experimental blocks such aslist-itemorpost-author-namein the site editor.cc @gziolo