Add new private upload-media package
#66290
Conversation
swissspidy
added
[Type] Build Tooling
swissspidy
force-pushed
the
add/61447-upload-media-pkg
branch
3 times, most recently
from
ad143dc
to
1851a20
Compare
|
Size Change: +3.89 kB (+0.21%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
swissspidy
force-pushed
the
add/61447-upload-media-pkg
branch
from
1851a20
to
b99c449
Compare
swissspidy
force-pushed
the
add/61447-upload-media-pkg
branch
from
b99c449
to
8a57140
Compare
package.json
Outdated
| @@ -310,7 +311,7 @@ | |||
| "lint:pkg-json": "wp-scripts lint-pkg-json . 'packages/*/package.json'", | |||
| "native": "npm run --prefix packages/react-native-editor", | |||
| "other:changelog": "node ./bin/plugin/cli.js changelog", | |||
| "other:check-licenses": "concurrently \"wp-scripts check-licenses --prod --gpl2 --ignore=@react-native-community/cli,@react-native-community/cli-platform-ios,@ampproject/remapping,human-signals,fb-watchman,walker,chrome-launcher,lighthouse-logger,chromium-edge-launcher\" \"wp-scripts check-licenses --dev\"", | |||
| "other:check-licenses": "concurrently \"wp-scripts check-licenses --prod --gpl2 --ignore=@react-native-community/cli,@react-native-community/cli-platform-ios,@ampproject/remapping,human-signals,fb-watchman,walker,chrome-launcher,lighthouse-logger,chromium-edge-launcher,webpack\" \"wp-scripts check-licenses --dev\"", | |||
There was a problem hiding this comment.
For some reason the Apache-licensed @webassemblyjs/leb128 and @xtuc/long packages, which are dependencies of webpack, were being flagged. No idea why this happened only when working on this PR.
|
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. |
An oversight. Added now. FWIW this is all coming from https://github.com/swissspidy/media-experiments if anyone wants to see the full context of how everything works together. |
|
I've been away and am a bit swamped, but I may be able to support the script-modules efforts if that would be helpful. |
|
I'll take any help I can get! 🙏 |
|
@youknowriad @sirreal @gziolo Would it be possible to move forward by merging this one for now and making any subsequent changes to the whole wasm and web worker thing in subsequent PRs? Feels like it would otherwise be stuck forever 😕 |
Co-authored-by: Jon Surrell <sirreal@users.noreply.github.com>
|
@youknowriad @gziolo @sirreal I've now reduced this PR for the media upload queue to the bare minimum, without any of the actual client-side processing/cropping/resizing logic whatsoever. Just hooking up this new package to the block-editor package—all behind the experimental flag of course. PTAL. |
|
How can this be tested in the editor? Could you add instructions? Could the PR description summarise why this queue is needed and how it can be used? |
Enable the client-side media processing experiment and upload media to the editor or so. Everything should still work as expected.
Sure, but I won't repeat everything. Please see #61447 for full context. https://github.com/swissspidy/media-experiments/blob/7b91599369f1d3ae412e3e18a4afe2f62257b4dc/docs/technical-overview.md is helpful as well. |
| @@ -290,6 +291,9 @@ function useBlockEditorSettings( settings, postType, postId, renderingMode ) { | |||
| isDistractionFree, | |||
| keepCaretInsideBlock, | |||
| mediaUpload: hasUploadPermissions ? mediaUpload : undefined, | |||
| __experimentalMediaSideload: hasUploadPermissions | |||
There was a problem hiding this comment.
If I'm understanding properly, this is defining a new "private" setting for the block editor.
We've been moving away from prefixed settings for these things, instead you can just use mediaSideload and add the key here
gutenberg/packages/block-editor/src/store/private-actions.js
Lines 26 to 29 in f7a4f3f
There was a problem hiding this comment.
Same for the two other settings.
| */ | ||
| import { unlock } from '../../lock-unlock'; | ||
|
|
||
| const { sideloadMedia: mediaSideload = () => {} } = unlock( privateApis ); |
There was a problem hiding this comment.
Is the default value needed?
| ? mediaSideload | ||
| : undefined, | ||
| __experimentalValidateFileSize: validateFileSize, | ||
| __experimentalValidateMimeType: validateMimeType, |
There was a problem hiding this comment.
Why are these two settings needed? Are they WP specific?
There was a problem hiding this comment.
No, they are not WordPress specific.
These separate functions are needed so that file size and mime type can be validated before a file is added to the queue.
Currently, this validation is only done in the uploadMedia() function from media-utils, which happens right before the file is sent to the server. But now there is additional processing before the uploading, so the two steps need to be separated.
Example 1 (file is too big):
- Drop a 10GB image into the editor
- File size is validated
- File is too big and is rejected, nothing is added to the queue
Example 2 (file is OK):
- Drop image into the editor
- File size is validated
- Mime type is validated
- File is added to the queue, where its size and mime type may change (compression, conversion)
- File is uploaded by calling
uploadMedia(), which does once again validate file size and mime type under the hood
Since the upload queue may change a file's type or size in the future (e.g. for converting HEIF images), eventually the validation will look more like this:
So the mime type would technically be disallowed, but since we know we can convert the image to an allowed mime type, it can be added to the queue nonetheless.
There was a problem hiding this comment.
Thanks for the details, it makes sense. Now, I do wonder why we need the settings though.
AFAIK the block-editor package already receives allowedMimeTypes and maxUploadFileSize settings. Why can't we bundle these within the packages directly instead of having these provided as extra settings?
There was a problem hiding this comment.
I think we should use private symbols for new experimental settings
Why not? I think it's useful if the relevant bits are repeated here.
I had looked there but I didn't see anything about the queue.
What should work as expected? This takes over all uploading if the experiment is enabled right? So that should be tested? |
packages/upload-media/src/components/provider/with-registry-provider.tsx
Show resolved
Hide resolved
Uploading media should still work as expected. |
|
I like that this package is an experiment. But why does the user experience need to be behind an experiment? Are we not confident that the queue will work for uploading media? It would make me more confident in adding an approval for this PR knowing that the e2e tests run fine at least. One small thing while testing: if you go to the GB demo page and upload one of the external images, the notification that the image has been uploaded now appears twice. I think we should make sure that all public setting keys are private too. I'm otherwise fine to add this. It shows that it can replace media uploading, which is great. I do wish it just immediately replaced rather than being behind a UI experiment (in addition to the package/API experiment). |
|
That's what experiments are for, no? :-) There's a ton of upcoming functionality in that upload queue (resizing, cropping, compression, conversion, thumbnail generation, etc.) that is best done behind a flag while that is being worked on. Especially with the WASM + web worker integration, which could potentially conflict with some plugins or hosting configurations. |
|
There's different layers to experiments. The package is an experiment so we don't add public APIs. The features built with this can all be experiments if you want. But why can't we use the queue for uploading so it's e2e tested? |
I think the question comes down to: Do we thinking the changes (upload queue) introduced in this PR are ready to ship to Core or not. |
Are other experiments e2e tested? Can we run all tests with all experiments enabled, or selectively enable experiments in certain tests?
No, not yet. |
We can probably run e2e tests on experiments (not sure if it has been done before), but we don't have good utils to enable experiments quickly AFAIK. |
What?
This is a new package for client-side media processing, see #61447 for full context.
It implements a queue-like system for processing media items prior to uploading them to the server. This will be used for things like resizing, cropping, compressing, thumbnail generation, poster generation, subtitle generation, etc.
See also https://github.com/swissspidy/media-experiments/blob/7b91599369f1d3ae412e3e18a4afe2f62257b4dc/docs/technical-overview.md.
This logic has been tested extensively as part of https://github.com/swissspidy/media-experiments, but for this initial PR several features have been removed. They will be reintroduced later in separate PRs to make reviews easier.
The package is private and shall not be published until later on when all of the subsequent PRs are merged.
Why?
Required for a new era of upload handling in Gutenberg
How?
Adds a new package to facilitate integration in the block-editor package
Testing Instructions
CI should be green.
Enable the client-side media processing experiment and upload media to the editor. Everything should still work as usual.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A