Editor: Move media upload from utils #7978

gziolo · 2018-07-16T13:41:50Z

Description

Part of #3955.

This is the last PR which will allow us to fully deprecate wp.utils module. We will have to keep a copy of deprecated methods for the next two releases to give some time plugin authors to migrate their code.

This PR schedules for removal 3 functions from wp.utils.* namespace:

wp.utils.getMimeTypesArray has been removed.
wp.utils.mediaUpload has been removed. Please use wp.editor.editorMediaUpload instead.
wp.utils.preloadImage has been removed.

That means mediaUpload will become an internal implementation of editorMediaUpload function.

In addition to that, we get rid of the global variable window._wpMediaSettings in favor of using editor settings, similar to what we do for other similar config values.

Testing

Make sure you can still use all deprecated methods in wp.utils.* namespace and they warn on the JS console about the upcoming deprecation.

Make sure you can upload all type of files using blocks like Audio, Video or Image.

aduth

Gave this one a rebase to resolve merge conflicts. There were a few updates from #7566 which needed to be ported to the new editor-media-upload/media-upload.js file.

I found it a bit difficult to test various workflows, as I encountered several unrelated bugs (#8022, #8023, #8025, #8026).

For workflows which do work on master, they also work here as well.

aduth · 2018-07-18T18:11:43Z

editor/store/defaults.js

@@ -81,4 +81,10 @@ export const EDITOR_SETTINGS_DEFAULTS = {

 	// Allowed block types for the editor, defaulting to true (all supported).
 	allowedBlockTypes: true,
+
+	// Maximum upload size in bytes allowed for the site.
+	maxUploadFileSize: 0,


I don't know that long-term we want these to be considered editor settings, since they apply to the site as a whole. Seems like something which should ideally live in @wordpress/core-data, though I do not believe they are yet available from the REST API.

aduth · 2018-07-18T18:13:16Z

docs/reference/deprecated.md

@@ -14,6 +14,9 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo
 - `wp.utils.decodeEntities` has been removed. Please use `wp.htmlEntities.decodeEntities` instead.
 - All references to a block's `uid` have been replaced with equivalent props and selectors for `clientId`.
 - The `MediaPlaceholder` component `onSelectUrl` prop has been renamed to `onSelectURL`.
+ - `wp.utils.getMimeTypesArray` has been removed.
+ - `wp.utils.mediaUpload` has been removed. Please use `wp.editor.editorMediaUpload` instead.


My first thought: Is this something which should be exposed on a public API? Then seeing we're using this in blocks, I'm left wondering: What does this have to do with the editor? It's just media uploads? Definitely seems like something for core-data.

aduth · 2018-07-19T03:45:48Z

Thinking more about naming / appropriateness in editor:

wp.editor.editorMediaUpload is pretty redundant; would wp.editor.uploadMedia be sufficient?
If it's something we're uncertain about as an API, maybe make it undocumented and exposed only as an unstable-prefixed function?

gziolo · 2018-07-19T09:35:37Z

wp.editor.editorMediaUpload is pretty redundant; would wp.editor.uploadMedia be sufficient?

Agreed, I updated PR to use shorter version.

If it's something we're uncertain about as an API, maybe make it undocumented and exposed only as an unstable-prefixed function?

It has been around for quite some time. We use with many core blocks.

aduth · 2018-07-19T12:19:02Z

It has been around for quite some time. We use with many core blocks.

It was more about the earlier point of whether this belongs as an editor function, vs. a core-data action. Prefixing it would serve the immediate need of requiring it in our current blocks.

aduth · 2018-07-19T15:29:06Z

editor/utils/media-upload/index.js

 	allowedType,
 	filesList,
 	maxUploadFileSize,
 	onError = noop,
 	onFileChange,
 } ) {
-	const postId = select( 'core/editor' ).getCurrentPostId();
+	const {
+		getCurrentPostId,


So, in retrospect, I think this is the one critical piece where having this be an editor-specific function could make sense, in effecting that the media be attached to the post being edited.

That said, with recent refactoring for store registries (#7527), exposing this as a simple function on the editor global does not seem tenable. If we want it, it seems like something which would be a higher-order component to inject the uploadMedia prop into the underlying component to take advantage of the contextualized store.

aduth · 2018-07-19T15:30:15Z

I think we should punt this to 3.4, so that we can offer an alternative solution, than to just remove the functionality or update it to a non-ideal alternative.

youknowriad · 2018-07-20T11:44:16Z

Do you think it's possible to land this as is and create an issue to come up with a better alternative? This is blocking the editor package.

aduth · 2018-07-20T12:41:09Z

@youknowriad My main concern would be the lack of an alternative to give in the release. I'd be fine with merging it immediately after, if that works?

gziolo · 2018-07-20T14:35:03Z

If we agree that initial implementation of HOC would only pass down mediaUpload function as prop, I can refactor it on Monday. It would be called withMediaUpload and located in editor package.

youknowriad · 2018-07-20T15:19:35Z

yep, let's at least wait for the release and then we can merge (before or after refactoring)

gziolo · 2018-07-23T07:46:07Z

There is one usage which makes it difficult to convert to HOC:
https://github.com/WordPress/gutenberg/blob/master/core-blocks/gallery/index.js#L134-L144

In this case, it's used inside the transform, so it doesn't have access to props at all because it is part of block settings. I'm trying to find a solution which would allow us to move it to the lifecycle of the GalleryEdit but it might be not the best idea.

youknowriad · 2018-07-23T09:15:55Z

I personally don't think mediaUpload should be a higher-order component. I think I'd rather add it as an async action once we have the API ready for @wordpress/data and see how we can improve its usage based on the action there. (The higher-order component is just withDispatch).

Let's move forward with just the mediaUpload removed from utils and discuss this further.

gziolo · 2018-07-23T09:21:27Z

a3795d0 shows that this is a more complex task which has some implications on the way we handle transforms. I will remove this commit, bump deprecations to be scheduled for 3.6 release and merge.

Let's continue the discussion and agree on the further steps.

youknowriad

LGTM 👍

aduth · 2018-08-14T19:03:52Z

lib/client-assets.php

@@ -1093,6 +1092,7 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
 	// https://github.com/WordPress/gutenberg/issues/5667.
 	add_filter( 'user_can_richedit', '__return_true' );

+	wp_enqueue_script( 'wp-utils' );


We should have commented to explain the context for this line. I'm in the course of removing the deprecated ./utils and it wasn't clear to me what relation this has.

Good call, when removing folder, you probably can remove all wp-utils occurrences in this file.

gziolo added [Status] In Progress Tracking issues with work in progress [Type] Breaking Change For PRs that introduce a change that will break existing functionality npm Packages Related to npm packages labels Jul 16, 2018

gziolo added this to the 3.3 milestone Jul 16, 2018

gziolo self-assigned this Jul 16, 2018

gziolo requested a review from youknowriad July 16, 2018 13:41

gziolo force-pushed the update/media-upload branch from ec234a1 to 4bbfbce Compare July 16, 2018 14:36

gziolo removed the [Status] In Progress Tracking issues with work in progress label Jul 16, 2018

gziolo changed the title ~~[WIP] Editor: Move media upload from utils~~ Editor: Move media upload from utils Jul 16, 2018

gziolo requested a review from a team July 16, 2018 14:54

Shelob9 mentioned this pull request Jul 16, 2018

Packages: Publish all modules as independent npm packages #3955

Closed

19 tasks

aduth self-requested a review July 18, 2018 13:38

aduth force-pushed the update/media-upload branch from 4bbfbce to cc763bf Compare July 18, 2018 15:01

aduth approved these changes Jul 18, 2018

View reviewed changes

aduth reviewed Jul 18, 2018

View reviewed changes

gziolo force-pushed the update/media-upload branch 2 times, most recently from 4cc3450 to 17cd99c Compare July 19, 2018 09:32

aduth reviewed Jul 19, 2018

View reviewed changes

aduth modified the milestones: 3.3, 3.4 Jul 19, 2018

gziolo mentioned this pull request Jul 20, 2018

Changed createObjectUrl to createBlobURL #7734

Closed

4 tasks

youknowriad mentioned this pull request Jul 20, 2018

Packages: Extract the editor package #8081

Merged

3 tasks

gziolo added 6 commits July 23, 2018 11:27

Testing: Simplify tests for media upload function

00f2494

Packages: Duplicate deprecated code for media-upload utils

33ff195

Editor: Move media upload from utils

5cc81ee

Editor: Rename editorMediaUpload to mediaUpload

0bcf43b

Editor: Update apiRequest occurrences with apiFetch

12d589b

Bump deprecation version for util methods

8cbaf6d

gziolo force-pushed the update/media-upload branch from a3795d0 to 8cbaf6d Compare July 23, 2018 09:29

youknowriad approved these changes Jul 23, 2018

View reviewed changes

gziolo merged commit b129f0e into master Jul 23, 2018

gziolo deleted the update/media-upload branch July 23, 2018 12:04

aduth reviewed Aug 14, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Move media upload from utils #7978

Editor: Move media upload from utils #7978

gziolo commented Jul 16, 2018 •

edited

Loading

aduth left a comment

aduth Jul 18, 2018

aduth Jul 18, 2018

aduth commented Jul 19, 2018

gziolo commented Jul 19, 2018

aduth commented Jul 19, 2018

aduth Jul 19, 2018

aduth commented Jul 19, 2018

youknowriad commented Jul 20, 2018

aduth commented Jul 20, 2018

gziolo commented Jul 20, 2018

youknowriad commented Jul 20, 2018

gziolo commented Jul 23, 2018 •

edited

Loading

youknowriad commented Jul 23, 2018 •

edited

Loading

gziolo commented Jul 23, 2018

youknowriad left a comment

aduth Aug 14, 2018

gziolo Aug 14, 2018

Editor: Move media upload from utils #7978

Editor: Move media upload from utils #7978

Conversation

gziolo commented Jul 16, 2018 • edited Loading

Description

Testing

aduth left a comment

Choose a reason for hiding this comment

aduth Jul 18, 2018

Choose a reason for hiding this comment

aduth Jul 18, 2018

Choose a reason for hiding this comment

aduth commented Jul 19, 2018

gziolo commented Jul 19, 2018

aduth commented Jul 19, 2018

aduth Jul 19, 2018

Choose a reason for hiding this comment

aduth commented Jul 19, 2018

youknowriad commented Jul 20, 2018

aduth commented Jul 20, 2018

gziolo commented Jul 20, 2018

youknowriad commented Jul 20, 2018

gziolo commented Jul 23, 2018 • edited Loading

youknowriad commented Jul 23, 2018 • edited Loading

gziolo commented Jul 23, 2018

youknowriad left a comment

Choose a reason for hiding this comment

aduth Aug 14, 2018

Choose a reason for hiding this comment

gziolo Aug 14, 2018

Choose a reason for hiding this comment

gziolo commented Jul 16, 2018 •

edited

Loading

gziolo commented Jul 23, 2018 •

edited

Loading

youknowriad commented Jul 23, 2018 •

edited

Loading