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

feat: Experimental editor assets REST API endpoint #64517

Open
wants to merge 13 commits into
base: trunk
Choose a base branch
from

Conversation

dcalhoun
Copy link
Member

@dcalhoun dcalhoun commented Aug 14, 2024

What?

Enable requesting all assets — styles and scripts — loaded for the block editor via the REST API.

Why?

The WordPress mobile team is exploring a WebView-based editor where a given site's editor assets power the editing experience. If deemed successful, the experimental approach might bring a few benefits to the iOS and Android mobile apps editing experience:

  • Expanded core block support.
  • Expanded third-party block support.
  • Improved offline editing support.
  • Closer overall alignment with the web editing experience.

How?

Add an experimental block editor assets REST API endpoint. The endpoint returns a collection of relevant assets utilizing an approach similar to WordPress core's iframe editor assets utility.

Testing Instructions

Without Permission

  1. Make an unauthenticated request to the new endpoint: curl -L <site_url>/?rest_route=/__experimental/wp-block-editor/v1/editor-assets
  2. Verify a 401 error response is returned.

With Permission

  1. Visit you site's WP Admin and navigate to UsersEdit (the admin user).
  2. Create and copy an application password.
  3. Create and copy a Base64 version of the combined username and application password: echo -n 'admin:<application_password>' | base64 | pbcopy
  4. Make an authenticated request to the new endpoint: curl --header "Authorization: Basic <base64_string>" -L <site_url>/?rest_route=/__experimental/wp-block-editor/v1/editor-assets
  5. Verify the response includes the expected editor style and script tags.

Testing Instructions for Keyboard

N/A, no user-facing changes.

Screenshots or screencast

N/A, no user-facing changes.

@dcalhoun dcalhoun added Core REST API Task Task for Core REST API efforts REST API Interaction Related to REST API [Type] New API New API to be used by plugin developers or package users. labels Aug 14, 2024
Disable unused variable warning as the `$request` parameter must be
referenced to satisfy inherited class requirements.

```
Fatal error: Declaration of WP_REST_Block_Editor_Assets_Controller::get_items_permissions_check() must be compatible with WP_REST_Controller::get_items_permissions_check($request) in /var/www/html/wp-content/plugins/gutenberg/lib/experimental/class-wp-rest-block-editor-assets-controller.php on line 144
```
@dcalhoun dcalhoun marked this pull request as ready for review August 14, 2024 17:13
Copy link

github-actions bot commented Aug 14, 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.

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: dcalhoun <dpcalhoun@git.wordpress.org>
Co-authored-by: spacedmonkey <spacedmonkey@git.wordpress.org>

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

@dcalhoun dcalhoun requested a review from geriux August 14, 2024 17:13
Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

Instead of this new PR, please can you review my PR - #21244.

@dcalhoun
Copy link
Member Author

Instead of this new PR, please can you review my PR - #21244.

Thank you for sharing this helpful context, @spacedmonkey. 🙇🏻

I reviewed the earlier PR — it is very cool and appears quite robust! I am concerned that its original goals may differ too much from the WordPress mobile team’s current goals. That divergence may complicate using the prior implementation for our particular needs.

More specifically, the following are a few challenges I encountered in my research. I’m unsure if/how your prior implementation might address them. I welcome your thoughts on these.

First, third-party plugins often fail to globally register conditionally utilized assets, instead the assets are only conditionally enqueued. This results in assets un-queryable at the global level. As quoted below, the WPGraphQL project referenced this same challenge in its own asset query feature as a “known issue.” In my proposed changes, I mitigate this issue by simulating an editor environment through applying relevant hooks, a very unique action for the particular use case of retrieving all editor assets.

Many plugins and themes, for example, don't register styles and scripts to the global registry, they just enqueue them. This can be problematic as the resource nodes for the assets won't be accessible via a node query using the asset ID, nor would they be accessible via a root { registeredScripts { nodes { ...fields } } query (because they're not actually registered).

The Storefront theme is an example of conditionally registering assets, which is also problematic. This means that a resource is only queryable via a connection and not available in a Root Query for all registered assets or via the node( id: $id ) query.

This will take some education around the ecosystem. . .Registering scripts and styles should be global, and enqueueing them should be conditional / contextual.

Second, the lower level nature of the prior implementation provides a robust foundation, but comes at the expense of requiring the client to reimplement dependency features WordPress already performs — e.g., sorting dependencies for correct loading order, deduplicating dependencies shared across assets, and building script/link tags. Rather than building this functionality in the mobile app, my proposed changes rely upon WordPress for these necessary actions.

Lastly, the prior implementation’s current form appears to only support querying a single asset by handle, rather than querying all assets for the editor; I presume we could adapt it to support querying all assets, though. My proposed changes focus on the specific need of loading all editor assets, rather than a single asset.

Again, thank you for sharing the past context. I’m interested to hear your thoughts on the challenges I share and the WordPress mobile team’s specific goal; it could be that both implementations provide unique value/capability for achieving the mobile team's goal. And, of course, please let me know if I misinterpreted anything in my review of your prior PR.

I look forward to exploring this together!

@spacedmonkey
Copy link
Member

@dcalhoun

To give you a little more context, I am a maintainer of the REST API in core and a core committer. I am the committer most likely to get this into core.

I agree that there are many issues with scripts being dynamically registered. It is likely that we will only be able to support scripts and styles that are registered by the register_block_type_from_metadata function. We may also need to manually trigger the register_core_block_style_handles function and possibly the enqueue_block_assets action as well.

I understand that my endpoint may not exactly match what you need, but it is a start. I strongly believe we can get it to a point where it could serve your purposes. However, I must emphasize that any endpoint that ends up in core needs to be generic and function as an interface that will remain in core indefinitely. Long after the mobile application codebase has changed, this endpoint will need to work the same way, as we promise backward compatibility. My endpoint is written to serve anyone requesting scripts and styles, not just a specific use case. For example, this endpoint could be used in a headless application.

For these reasons, I do not support the current format of this PR, as I don’t feel the endpoint is generic enough for other uses. In its current state, I would block it from being committed to core. However, I believe that my PR is a good base to build on. Could you take my PR and build on top of it, @dcalhoun? I am willing to pair on this, but I have limited time as I am about to become a father.

@dcalhoun
Copy link
Member Author

@spacedmonkey thank you for providing additional details for your perspective on these proposed changes. I understand and recognize the value in a more generic approach that might better serve additional use cases.

Currently, I am exploring an alternative approach inspired by the web editor's loadAssets utility used for on-demand loading of plugin assets when installing a block from the inserter.

I'll close this PR for the time being, as it seems a different approach  — either the aforementioned loadAssets or building upon #21244 — is the most likely path forward.

Congratulations on fatherhood, that is wonderful!

@dcalhoun dcalhoun closed this Aug 23, 2024
@dcalhoun dcalhoun deleted the feat/editor-assets-rest-api-endpoint branch August 23, 2024 12:44
@dcalhoun dcalhoun restored the feat/editor-assets-rest-api-endpoint branch September 18, 2024 19:56
This cleared out previously registered dependencies by core and plugins.
It appears third-party plugins often rely upon this hook for enqueuing
assets.
These are necessary for the post editor to function.
@dcalhoun dcalhoun reopened this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core REST API Task Task for Core REST API efforts REST API Interaction Related to REST API [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants