Amp-wp: Uploading videos broken

[swissspidy]

Bug Description

Video upload seems to be broken because there doesn't seem to be a mediaUpload prop being passed anymore. Perhaps something changed upstream causing this to be missing.

Expected Behaviour

Uploading videos should work

Steps to reproduce

1. Insert video block
2. Upload video
3. See block being broken

Screenshots

Additional context

• WordPress version: 5.3 Beta
• Plugin version: current develop
• Gutenberg plugin version (if applicable): 6.6
• Browser: Chrome on macOS

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• AC1: Enable users to successfully upload videos.

Implementation brief

• Add conditional check to usage of media upload function (as per comments below)

QA testing instructions

1. Insert video block
2. Upload video
3. See video successfully being inserted and working as expected also on the frontend

Demo

• https://youtu.be/dn6U3UJoR-4

Changelog entry

N/A as it addresses a recently introduced regression

[spacedmonkey]

When researching this, the issue was created in https://github.com/ampproject/amp-wp/pull/3412 . The refector to hooks broke the upload. The reason it breaks is the odd setup of the media uploader in gutenberg. The media uploader is setup using a filter here. As this filter has no fired in the callback, then mediaUpload fails.

When testing, I reverted the changes to the custom-video-block-edit.js and the uploader works. As much as I would personally like everything to use hooks. I think reverting the hook changes in favour is acceptable workaround to the limitation of gutenberg.

Thoughts @swissspidy @miina ?

[miina]

@spacedmonkey Thanks for looking into this bug! To understand better -- how is the filter behaving differently when not using the hooks / how does using hooks influence the filter? (In case you found that out while debugging.)

[miina]

Adding if ( file && 'function' === typeof mediaUpload ) { here seems to fix the issue, too:
https://github.com/ampproject/amp-wp/blob/6c946dc0351cb60aa5c61ef1eb756b6660903753/assets/src/stories-editor/components/custom-video-block-edit.js#L109

Basically, it looks like in some cases mediaUpload hasn't been defined yet, however, once it is, then the useEffect takes place again and it works.

[swissspidy]

That sounds like a good fix to me

[miina]

Moving this to "Estimate" column for clarity since it seems to have a fix to implement.

[spacedmonkey]

Adding if ( file && 'function' === typeof mediaUpload ) { here seems to fix the issue, too:

I looked at a solution like this, but I felt like it just hid the problem and didn't really solve the problem. But I think we know what the problem is here, it is an code order issue. I find the above solution a little dirty, but if it works :man_shrugging:

[spacedmonkey]

After working with @miina I found a much better solution to this issue. I have created a PR at #3487 for review.

[csossi]

Getting an error on uploading.

Selected upload from the video block (as opposed to clicking Media Library button). Video appears fine at first, but then changes to this:

Console:
index.js:1 wp.blockEditor.RichText wrapperClassName prop is deprecated. Please use className prop or create your own wrapper div instead.
c @ index.js:1
2/wp-admin/undefined:1 Failed to load resource: the server responded with a status of 404 ()
react-dom.min.b694e242.js:212 Uncaught (in promise) Error: Minified React error #185; visit https://reactjs.org/docs/error-decoder.html?invariant=185 for the full message or use the non-minified dev environment for full errors and additional helpful warnings. 
    at Sb (react-dom.min.b694e242.js:212)
    at Mg (react-dom.min.b694e242.js:85)
    at onFileChange (amp-stories-editor.js:16)
    at index.js:1
    at tryCatch (wp-polyfill.min.js:1)
    at Generator.invoke [as _invoke] (wp-polyfill.min.js:1)
    at Generator.t.<computed> [as next] (wp-polyfill.min.js:1)
    at r (index.js:1)
    at u (index.js:1)
    at index.js:1
media:1 Failed to load resource: the server responded with a status of 504 ()
2/wp-admin/undefined:1 Failed to load resource: the server responded with a status of 404 ()

[swissspidy]

Looks like there are still some issues with hooks now, causing infinite loops 🤔

[swissspidy]

The 404 seems easy to fix in assets/src/stories-editor/components/custom-video-block-edit.js:

useEffect( () => {
-       if ( ! isBlobURL( src ) ) {
+       if ( src && ! isBlobURL( src ) ) {
            getContentLengthFromUrl( src ).then( setVideoSize );
        }
    }, [ src ] );

The other issue seems to be in the useEffect hook that does the mediaUpload. src seems to be constantly changing to a different blob, so the hook is fired gazillions of times.

[swissspidy]

This might work for fixing the infinite loop:

useEffect(
        () => {
            if ( ! id && isBlobURL( src ) ) {
                const file = getBlobByURL( src );
                if ( file ) {
                    mediaUpload( {
                        filesList: [ file ],
                        onFileChange: ( [ { id: mediaId, url } ] ) => {
                            setDuration( null );
                            setVideoSize( null );
                            setAttributes( { id: mediaId, src: url } );
                        },
                        onError: ( message ) => {
                            setIsEditing( true );
                            noticeOperations.createErrorNotice( message );
                        },
                        allowedTypes: allowedVideoMimeTypes,
                    } );
                }
            }

            if ( src && ! isBlobURL( src ) ) {
                getContentLengthFromUrl( src ).then( setVideoSize );
            }
        },
        // Disable reason: src is constantly changing, but we want this hook to fire only once (like componentDidMount).
        // eslint-disable-next-line react-hooks/exhaustive-deps
        []
    );

But perhaps someone has a better idea 🤔

[spacedmonkey]

I will take a look

[spacedmonkey]

I am not seeing this error when I upload. I am seeing this error.

Uncaught (in promise) Invariant Violation: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
    at http://localhost:8890/wp-content/plugins/gutenberg/vendor/react-dom.8b3dda97.js:23253:28
    at checkForNestedUpdates (http://localhost:8890/wp-content/plugins/gutenberg/vendor/react-dom.8b3dda97.js:23256:7)
    at scheduleUpdateOnFiber (http://localhost:8890/wp-content/plugins/gutenberg/vendor/react-dom.8b3dda97.js:21527:3)
    at dispatchAction (http://localhost:8890/wp-content/plugins/gutenberg/vendor/react-dom.8b3dda97.js:15954:5)
    at onFileChange (http://localhost:8890/wp-content/plugins/amp/assets/js/amp-stories-editor.js?ver=13bc494…:7941:13)
    at http://localhost:8890/wp-content/plugins/gutenberg/build/media-utils/index.js?ver=ae8f150…:1:10442
    at tryCatch (http://localhost:8890/wp-includes/js/dist/vendor/wp-polyfill.js?ver=7.0.0:6292:40)
    at Generator.invoke [as _invoke] (http://localhost:8890/wp-includes/js/dist/vendor/wp-polyfill.js?ver=7.0.0:6518:22)
    at Generator.prototype.<computed> [as next] (http://localhost:8890/wp-includes/js/dist/vendor/wp-polyfill.js?ver=7.0.0:6344:21)
    at r (http://localhost:8890/wp-content/plugins/gutenberg/build/media-utils/index.js?ver=ae8f150…:1:12087)

[swissspidy]

That's the same, just in the development build. See https://reactjs.org/docs/error-decoder.html/?invariant=185 from the production build error message.

[spacedmonkey]

I have created a fix at #3553 .

It is worth noting, that this is another bug, unrelated to the original one. But for the sake of speed I have fixed in this issue.

[swissspidy]

@csossi Could you please try again now? Thanks in advance.

[csossi]

Verified in QA