Refactor to match Gutenberg architecture

[swissspidy]

I'm splitting this out from WordPress/gutenberg#61447 (comment) where @youknowriad has asked to basically split up the media handling/queue logic from the WP-specific upload logic so that the whole queue store can live in @wordpress/block-editor.

The uploadMedia() function itself from @wordpress/media-utils would be basically the same, just taking a file and uploading it to a server. This way, every block editor implementation can benefit from the image processing capabilities, and media-utils remains stateless.

So far, the code has been written with this architecture in mind:

• block-editor requires a mediaUpload setting, and without it does not support uploads by default. Is platform-agnostic.
• media-utils exposes a mediaUpload function that validates files, adds items to a queue, handles compression, etc. Is WordPress-specific.
• editor package wraps mediaUpload from media-utils to add the post ID and provides the function as a setting to block-editor. Is WordPress-specific.

But the goal is:

• block-editor should now gain all the capabilities for the upload queue and compression etc. The final file is passed to the mediaUpload function passed via settings. If the setting is missing, it still doesn't support uploads by default.
• media-utils does not change, it simply takes a file and uploads it to WordPress.
• editor also does not change, it still wraps media-utils to provide the post ID.

Things like sideloading will require either additional arguments to mediaUpload or a new mediaSideload setting or so.

I'm using this issue to keep track of all the work to make this happen, and write down ideas.

Thoughts / questions so far:

• @wordpress/editor calls wp.data.dispatch( 'core/block-editor').updateSettings() to pass the mediaUpload function for uploading to WordPress.
• Individual blocks etc. will call wp.data.select( 'core/block-editor' ).getSettings.mediaUpload() for uploading. But that one must first add items to a queue and then call the actual one passed from @wordpress/editor. So it must not actually expose the "raw" upload function.
• block-editor will want do basic mime type validation like uploadMedia does before adding to the queue. Require another setting for this?
  • Then the flow would be like: block-editor uses getSettings().validateMedia -> add to its queue -> once finished, call getSettings.uploadMedia()
• This also means block-editor (or the upload package it depends on) must expose some more functionality than just uploadMedia, like the various state selectors, etc.
  • Example: optimizeExistingItem for optimizing an existing image/video or isUploadingById for knowing whether a current image in the editor is already being processed.
• From the current upload-media package:
  • mediaSourceTerms will need to be handled at the editor level — or simply removed.
  • Things like looping through missing image sizes & adding them to the queue will now need to be handled by the caller — potentially causes lots of repetition — unless this could be handled by mediaUpload from media-utils? So move that post-upload logic there? 🤔

[swissspidy]

• block-editor will want do basic mime type validation like uploadMedia does before adding to the queue.

Also noting that there is a difference between the mime types supported by WordPress and the mime types supported by the media queue. For example, WordPress might not support HEIC by default and would block the file upload. However, the media queue happily takes that file and converts it to a JPEG.

[swissspidy]

@youknowriad @ntsekouras Pinging you two directly since you expressed interest in reviewing the code further. Feel free to ping others as well.

I did the main change now, which was to separate the processing & upload concerns. In a nutshell, it currently looks like this:

• media-utils - Like @wordpress/media-utils but more advanced.
• upload-media - Core upload logic with a queue-like system, implemented using a custom @wordpress/data store.
• editor - Main editor UI integration
  • This particular file is hooking media-utils and upload-media into @wordpress/block-editor.

It didn't feel right to move more out of upload-media. Things like client-side thumbnail generation are as much part of that package as the image compression itself. @wordpress/block-editor today already has some undocumented expectations about what mediaUpload's onFileChange function returns, now these are simply more clearly documented due to TypeScript (everything is 100% TS).

I would appreciate it if you could take some time to test the plugin, go through the code and provide some high level feedback about it.

Each package and each action, reducer and selector has extensive documentation and there's good e2e test coverage as well. That should make it possible to delve into the project and gain a better understanding.

[ntsekouras]

Also noting that there is a difference between the mime types supported by WordPress and the mime types supported by the media queue. For example, WordPress might not support HEIC by default and would block the file upload. However, the media queue happily takes that file and converts it to a JPEG.

Does this mean that we could apply this for suggesting WP allowed mime types, that a user would have to convert in order to upload? What other applications would this have in WP context?

[swissspidy]

That's how it works today in WP: if you drop an exotic file into the editor, you will get a notification that the mime type is not supported. Then you would need to convert it manually in order to upload. HEIC is such an example (depending on server support). This project now makes it possible to convert such files automatically in the browser before upload, so that you don't have to worry about it. Does that answer your question?

[ntsekouras]

Been looking at the codebase for a bit and it seems to me it's in the right direction. There's so much code though, that more eyes are needed.

I'd expect when we integrate this to GB, references and usage of coreStore (@wordpress/core-data) should be passed and have block-editor without that dependency.

Also noting that while testing by downloading the plugin, if I had GB trunk enabled it would break the editor. Not sure why..

[ntsekouras]

It would probably help if we start having some GB PRs for integrating the functionality. It's going to be easier to review specific change sets and will reveal any challenges and changes needed to follow the suggested architecture.

[swissspidy]

I'd expect when we integrate this to GB, references and usage of coreStore (@wordpress/core-data) should be passed and have block-editor without that dependency.

No. The only references to coreStore are for UI bits that would live in block-library, e.g. to retrieve the site logo from the site settings in the Site Logo block.

Also noting that while testing by downloading the plugin, if I had GB trunk enabled it would break the editor. Not sure why..

Hmm I will need to look into that. It's related to the cross-origin isolation and the iframed editor. useShouldIframe() forces the iframed editor if within the Gutenberg plugin.

Not sure when that changed, as I used to run GB trunk frequently in the past.

It would probably help if we start having some GB PRs for integrating the functionality. It's going to be easier to review specific change sets and will reveal any challenges and changes needed to follow the suggested architecture.

I would appreciate a high-level review first before I spend a significant time porting over the first chunks only to realize that XYZ is missing or wrong and I need to redo the whole thing. The useShouldIframe() case above is a good example of that :-)

After that, yes, the idea is to start with some smaller PRs step by step. You can see the proposed roadmap at WordPress/gutenberg#61447

[ntsekouras]

Personally I feel that the high level feedback was about the store and where parts of the code would fit in GB packages. The queue seems fine and could potentially be used for other parts in GB in the future and you've already made updates to decouple WP specific parts from media-utils, etc.. We can wait for more opinions though..

The UI will need design feedback of course, but that will be part of the GB PRs. With such PRs, possible integration issues will be easier to identify and some review questions about implementation details might be raised. In general by testing the plugin, everything seems to be working quite well and it seems to be in the right direction to integrate.

I guess something that I haven't checked and we need to know, is whether the libraries used here are compatible with GB licence. I'm not an expert in this domain though and if there is some uncertainty, we should ping some folks who know more about this.

[swissspidy]

I guess something that I haven't checked and we need to know, is whether the libraries used here are compatible with GB licence. I'm not an expert in this domain though and if there is some uncertainty, we should ping some folks who know more about this.

For the initial implementation as per the suggested roadmap there are no license concerns, all dependencies are fully GPL compatible. See #322.

The only question mark is about libheif-js, as previously discussed in this thread. See also #483.

As long as there is this question mark, it will not be added to GB. In the meantime, other folks are working on server-side HEIC conversion, where the license isn't an issue.