Skip to content
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

Add viewScriptModule handling to block.json metadata #5860

Closed
wants to merge 27 commits into from

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Jan 11, 2024

Add handling for viewScriptModule block.json metadata when registering blocks.

This can be tested with this demo plugin.

The block can be added in the editor. On the frontend, the module is enqueued when viewing a page with the block:

document.querySelectorAll('[type=module]')
// NodeList [script#jon-the-block-view-script-module-js-module]

Trac ticket: https://core.trac.wordpress.org/ticket/60233

This is a port from Gutenberg: WordPress/gutenberg#57959
It was implemented with viewModule in WordPress/gutenberg#57437
Gutenberg PR to switch to viewScriptModule: WordPress/gutenberg#58731


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@sirreal
Copy link
Member Author

sirreal commented Jan 15, 2024

We'll want to wait until naming questions are settled:

#5869

@youknowriad
Copy link
Contributor

Is the naming settled, what's blocking here?

@gziolo
Copy link
Member

gziolo commented Jan 23, 2024

Is the naming settled, what's blocking here?

It needs a rebase after #5869 lands. The name of the helper functions for modules will follow the following pattern: wp_(register|enqueue)_script_module, so that probably should get reflected in block.json and other places, viewScriptModule? It's a bit unfortunate that we can't extend viewScript in a backward-compatible way to support both ES Modules and legacy scripts.

@sirreal
Copy link
Member Author

sirreal commented Jan 23, 2024

#5869 landed recently so I'll finish this and get it ready for review.

@sirreal sirreal force-pushed the add-view-module-handling branch from 440b5eb to c047353 Compare January 23, 2024 14:00
@sirreal sirreal changed the title Add viewModule handling to block.json metadata Add viewScriptModule handling to block.json metadata Jan 23, 2024
@sirreal sirreal force-pushed the add-view-module-handling branch from 5d829fa to 4a8a7f3 Compare January 24, 2024 12:58
@sirreal
Copy link
Member Author

sirreal commented Jan 24, 2024

The Modules API ended up with names like "script_module." To better align, this has been updated to use viewScriptModule instead of viewModule.

@sirreal sirreal marked this pull request as ready for review January 24, 2024 15:00
@sirreal
Copy link
Member Author

sirreal commented Jan 24, 2024

@luisherranz @gziolo @youknowriad This is ready for review.

src/wp-includes/blocks.php Outdated Show resolved Hide resolved
@sirreal
Copy link
Member Author

sirreal commented Jan 24, 2024

There was 1 test failure on a single PHP version that seems to be a flakey test.

@sirreal
Copy link
Member Author

sirreal commented Jan 24, 2024

Adding support to the @wordpress/scripts for building blocks with viewScriptModule: WordPress/gutenberg#58203

Copy link

github-actions bot commented Feb 5, 2024

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 props-bot label.

Core SVN

Core Committers: Use this line as a base for the props when committing in SVN:

Props jonsurrell, gziolo, cbravobernal, luisherranz, youknowriad.

GitHub Merge commits

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: sirreal <[email protected]>
Co-authored-by: gziolo <[email protected]>
Co-authored-by: c4rl0sbr4v0 <[email protected]>
Co-authored-by: luisherranz <[email protected]>
Co-authored-by: youknowriad <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@sirreal
Copy link
Member Author

sirreal commented Feb 5, 2024

Tests were failing because the number of REST fields returned in the response has changed again. They should be fixed now.

@sirreal
Copy link
Member Author

sirreal commented Feb 6, 2024

The asset file can be optional: #5799

@@ -471,6 +471,12 @@ public function render( $options = array() ) {
}
}

if ( ! empty( $this->block_type->view_script_module_ids ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering whether it would fit better after all filters get applied like render_block. This way, plugin authors still would have the opportunity to change the list of module IDs on the block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that authors could change the block or module dependencies in the render_block filter function?

I don't have strong feelings on whether or not it should be moved after the render filters. I do know that changing the module graph is error prone and not something that should be done in most cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extenders can add/remove/replace what is inside $this->block_type->view_script_module_ids with filters like render_block, so maybe it's worth accounting for it. Anyway, the same issue exists today with view scripts, so maybe we need to live with it for conistency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to defer to your judgment. My instinct was to align with how scripts work but we do not strictly need to do if we can improve things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can check if differs after filters and do a _doing_it_wrong() with the message of Changing the module graph is error prone

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion about this but from my experience trying to change supports.interactivity dynamically, I don't think mutating the Block Type at render time should be encouraged. The way I see it now is that the Block Type is meant to represent the properties of all the blocks of that type, not just the one being rendered. Changing it on the fly to accommodate the block that is being rendered is confusing and leads to complexity. So, I think that the differences between the rendered blocks of the same type should be reflected by other means.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you anticipate introducing any conditional loading for view script modules depending on the HTML rendered for the block? Let's say enqueueing only these registered modules for the block that has corresponding directives in the final HTML. That would be a more compelling reason to move the logic after block render filters.

@sirreal sirreal force-pushed the add-view-module-handling branch from d12e711 to a225da1 Compare February 8, 2024 10:00
@sirreal
Copy link
Member Author

sirreal commented Feb 8, 2024

I've finished rebasing (it was a bit tricky) and tweaking some documentation, I think this is ready now.

@gziolo
Copy link
Member

gziolo commented Feb 8, 2024

Committed with https://core.trac.wordpress.org/changeset/57565.

@gziolo gziolo closed this Feb 8, 2024
@sirreal sirreal deleted the add-view-module-handling branch February 8, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants