Try canonical block plugins using the Time to Read block #61504
Conversation
creativecoder
added
[Type] Enhancement
creativecoder
changed the title
|
Size Change: +389 B (+0.02%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
This is a great start. Do you want it to be more tightly integrated with the monorepo? I could offer a few hints on moving some of the configurations to the root of the projects. In the long run, we could also explore improving linting overrides related to textdinaub so plugin authors can benefit from it and avoid the need to maintain their own customizations. |
@gziolo , thanks for taking a look! Yes, that would be great--any hints you have would be appreciated. I'd like to use this PR to experiment with monorepo integration, CI configuration, and the like. We can always break it out into more granular changes before merging, if needed. |
There was a problem hiding this comment.
I left a bunch of suggestions on how to move some control to the top of the repository so that whenever we add a new block, we will be updating existing files. I think, the most impactful thing would be getting rid of maintaining the local package lock file since all the packages in most of the cases will be installed in the root node_modules folder. By the way, to keep the lock file from the project, you can use the following setting in the .npmrc file:
I'm also sure we can figure out a way to be able to build these block from the root of the project, similar to how we run additional build steps for some WP packages using Learn:
Line 272 in 4d1c083
We shouldn't reuse Lerna here for a new script, because we don't need to publish anything to npm, but we could start using npm workspaces to achieve the same goal.
| "start": "wp-scripts start" | ||
| }, | ||
| "devDependencies": { | ||
| "@wordpress/scripts": "27.7.0" |
There was a problem hiding this comment.
Can we drop it from here? We can use the same script installed in the root of the project. The same applies to some of the script like linting and formatting that are handled for the entire repository already.
| "@wordpress/scripts": "27.7.0" | ||
| }, | ||
| "dependencies": { | ||
| "@wordpress/block-editor": "^12.24.0", |
There was a problem hiding this comment.
I think we should reference the local packages that are present in the project, like in other places, example:
gutenberg/packages/block-library/package.json
Lines 33 to 66 in 4d1c083
Here, the path will be a bit different:
| "@wordpress/block-editor": "^12.24.0", | |
| "@wordpress/block-editor": "file:../../packages/block-editor", |
When using explicit version, these quickly will get outdated. By referencing the same packages from the repository, we will always use the latest version. I'm not entirely sure if that is necessary outside of unit tests, but at least we won't have to install these packages from npm.
There was a problem hiding this comment.
Yes, that seemed to work as expected. Also done in 30cab7a.
There was a problem hiding this comment.
@gziolo Thinking through this a bit more, I think we would need to specify the latest version of packages shipped in WordPress, to ensure compatibility.
Block plugins should work without Gutenberg active. Since @wordpress packages are transformed to reference window.wp as part of the js build process, I believe that production block code could end up using older package versions and unexpectedly break if newer APIs were not available in those older versions.
Does that seem accurate? What do you think?
There was a problem hiding this comment.
Then again, it doesn't really seem to matter what package versions are specified here, as either way the actual code in the browser is going to use whatever window.wp provides, independent of the packages here in the repo.
The important thing will be running e2e tests without Gutenberg active to ensure the block plugin works with the latest WordPress release even when the Gutenberg plugin is disabled.
| @@ -0,0 +1,13 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
Here, I hope we could also should be able to update the shared config by setting override for the entire folder:
https://github.com/WordPress/gutenberg/blob/trunk/phpcs.xml.dist
Although, I'm not sure how to do it myself. @anton-vlasenko might be able to help.
There was a problem hiding this comment.
I looked through the phpcs configuration docs and can't see a way to apply different rule attributes to different paths in a single command (to use different translation domains for block plugins and the main plugin in the same phpcs check under the WordPress.WP.I18n rule). It seems that whenever you redeclare the same ruleset, it overrides any previous instances of the same rule. I believe we could add extra domains to the list, but that doesn't seem like enough, as it wouldn't catch 'gutenberg' being used in block plugins.
So the best I could come up with so far
- Move most of the rules to a
phpcs.xml.sharedfile - Import the shared rules into the main
phpcs.xml.dist, and into a separate file for each block plugin - Use a script when running the
lint:phpcommand that does a separate run of phpcs for eachphpcs.xml.distconfig
But let me know if I'm missing something or there's a better way to organize this.
|
I've updated this PR so that all plugin commands (like
|
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core GitHub repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ phpunit/plugins/time-to-read-block/renderBlock.php ❔ phpunit/plugins/time-to-read-block/wordCount.php ❔ phpunit/bootstrap.php |
|
Flaky tests detected in 7d01afd. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9557388463
|
- Rename and move plugin as time-to-read-block - Update readme.txt and plugin headers with appropriate values - Add screenshots - Add phpcs and eslint configuration overrides - Update PHP function names to use the gutenberg_ namespace - Resolve eslint error by installing the clsx package
- Add custom errors for assertions - Activate/deactivate plugins in sequence, rather that in parallel, since parallel activation seems to cause block not found errors
- Sets up a phpunit/plugins directory for canonical block plugin tests - Copies existing time to read block tests into plugins test dir
creativecoder
force-pushed
the
try/canonical-block-plugins
branch
from
5f9d0d8
to
b8db311
Compare
…s.org as the author
creativecoder
added
the
No Core Sync Required
|
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've marked this PR ready for review, as I believe all of the technical work needed to try out releasing the Time to Read block in a separate plugin is in place, and CI is all finally passing. (Though the canonical plugins concept likely needs more discussion and a core proposal is still planned.) cc @gziolo @youknowriad @anton-vlasenko specifically for any more feedback on the monorepo integration, like how npm scripts and tests are set up. |
| @@ -268,12 +271,14 @@ | |||
| "jsdom": "22.1.0" | |||
| }, | |||
| "scripts": { | |||
| "build": "npm run build:packages && wp-scripts build", | |||
| "build": "npm run build:packages && concurrently 'npm run build:plugins' 'wp-scripts build'", | |||
There was a problem hiding this comment.
The question to be asked is whether we want to build plugins when we're working on Gutenberg. Building plugins for now is probably very fast but I expect that as we increase the number of canonical blocks, it might become a burden and developers would want to focus on one thing in particular: Gutenberg, a specific block... An option to build everything, would be handy too but I wonder if it should be the default or not.
There was a problem hiding this comment.
My personal expectation is that the primary build command will build the entire project, and that I shouldn't need to run any other commands for a complete build. That said, having a build:gutenberg or similar that only builds the main Gutenberg plugin (and packages) seems like a good idea.
| } | ||
| tests_add_filter( 'muplugins_loaded', '_manually_load_plugin' ); | ||
| tests_add_filter( 'muplugins_loaded', '_manually_load_plugins' ); |
There was a problem hiding this comment.
Do we want to run all the tests of Gutenberg in the same CI job or we want to separate the CI jobs for plugins?
There was a problem hiding this comment.
Same for JS and e2e tests.
There was a problem hiding this comment.
Good question: it would probably be better to have separate CI jobs, to help ensure all plugins work independently from Gutenberg (and from one another). Though I think all jobs should be run on all PRs, to catch any Gutenberg changes that accidentally break unrelated things in plugins.
| @@ -0,0 +1,184 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Interesting choice, should it be phpunit/plugins/time-to-read-block or plugins/time-to-read-block/phpunit?
There was a problem hiding this comment.
I guess, similar question for e2e tests.
There was a problem hiding this comment.
Interesting choice, should it be phpunit/plugins/time-to-read-block or plugins/time-to-read-block/phpunit?
That seems reasonable and makes the tests easier to find when they're colocated with the plugins.
That said, I think it's important to be able to run all tests of a particular type using the same command, from the root of the repo, rather than needing to know about separate commands or switch directories to run a complete set of tests. Additionally I think we should avoid duplicating testing configuration as much as possible.
It would also be good to define a separate plugins suite for phpunit, and something similar for other test types, so it's trivial to run plugin tests separately, when desired.
There was a problem hiding this comment.
Additionally I think we should avoid duplicating testing configuration as much as possible.
💯
In my mind the root level is basically plugins/gutenberg but for historic reasons it's in the root. In other words, I like when tests are collocated with the plugin I think, not sure how complex would it be to do that.
- Leave the configuration in a root folder.
- Collocate the plugin tests.
- For Gutenberg it's a special case, maybe keep it root level for now.
| </ruleset> | ||
| ``` | ||
|
|
||
| #### ESLint |
There was a problem hiding this comment.
Can this override be done in an .eslintrc.js within the plugin folder?
There was a problem hiding this comment.
I can be, and that's what I had at first, though I changed it on based on @gziolo 's feedback (#61504 (comment))
There was a problem hiding this comment.
Personally I feel collocating is better as most people will just copy/paste a plugin folder and adapt it. Simpler than having to change files elsewhere.
That said, it's not a big deal either if it stays as is.
|
In terms of dev workflow and everything, this seems very good to me already. I'd prefer if we collocate as much as we can (tests, configs...) but it's a small thing. Other than that, I think we need to think about releases, timing, workflows... This is a tangent to the PR though and should happen separately. |
|
Given the new plan described here, I'm going to close this PR. But it's in a good state to serve as a reference if and when we return to wanting to use the Gutenberg monorepo for additional plugins. |
What?
Experiment to try creating a canonical block plugin, using the existing Time to Read block.
Part of #58773
Why?
Canonical Block Plugin?
From the #58773 description:
Time to Read Block
The block already exists in Gutenberg, but has been punted from WP releases twice, over concerns it's not a good fit for the default block library (#53776). But it still seems to be a popular block, so would be useful to have available in a stand-alone block plugin for easy installation from the editor (for those who don't run the Gutenberg plugin on their site).
How?
Time to Read block
create-block-pluginscaffolding.Canonical plugins infrastructure
plugins/folder in the repo.npm run build -w plugins/time-to-read-block).See the
plugins/README.mdfile in this PR for more details and documentation.Testing Instructions
npm run buildwp-content/plugins/time-to-read-blockin your development environmentFuture work
This PR does not include scripts and workflows for the following:
Requires at least:andRequires PHP:plugin headers when new versions of WordPress are released