[RNMobile] Fetch VideoPress metadata for native block #29997

SiobhyB · 2023-04-08T18:33:13Z

Part of a series of PRs to address wordpress-mobile/gutenberg-mobile#5611.

Related PRs

The changes in this PR relies on the following:

gutenberg-mobile: VideoPress block: Fetch VideoPress metadata for native block wordpress-mobile/gutenberg-mobile#5652

Proposed changes:

06b3340: Skip an unnecessary isUserConnected check. As only simple WordPress.com sites are supported on native for the time being, we can safely assume that users will be connected to Jetpack. We can revisit this in the future when expanding support.
fe1ba4d: To avoid creating native-specific files with very few changes, the debug functionality (which isn't compatible with native by default) is conditionally changed to console.log.
aa4bf9b: We pass the endpoint as a path on native, to ensure the URL is correctly built in the native apps and to allows us more control over supported endpoints. Support for this endpoint is added in VideoPress block: Fetch VideoPress metadata for native block wordpress-mobile/gutenberg-mobile#5652.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

p9ugOq-3mV-p2

Does this pull request change what data or activity we track or use?

No, it does not.

Testing instructions:

Note
The changes in this PR make sure the VideoPress metadata is synced from the web → app only. The necessary changes to ensure the metadata is also synced from the app → web will come in a future PR.

As the useSyncMedia hook has not yet been implemented on native, it's necessary to first apply the following patch before testing these changes.

Patch here ⤵️

These changes will implement the useSyncMedia hook and log the results of useVideoData:

diff --git a/projects/packages/videopress/src/client/block-editor/blocks/video/edit.native.tsx b/projects/packages/videopress/src/client/block-editor/blocks/video/edit.native.tsx
index ff9906b1bf..ec61816006 100644
--- a/projects/packages/videopress/src/client/block-editor/blocks/video/edit.native.tsx
+++ b/projects/packages/videopress/src/client/block-editor/blocks/video/edit.native.tsx
@@ -13,6 +13,7 @@ import { useDispatch, useSelect } from '@wordpress/data';
 import { useState, useCallback, useEffect } from '@wordpress/element';
 import { __ } from '@wordpress/i18n';
 import { store as noticesStore } from '@wordpress/notices';
+import { useSyncMedia } from '../../hooks/use-sync-media';
 /**
  * External dependencies
  */
@@ -133,6 +134,8 @@ export default function VideoPressEdit( {
 		[ setAttributes ]
 	);
 
+	useSyncMedia( attributes, setAttributes );
+
 	const handleDoneUpload = useCallback(
 		newVideoData => {
 			setIsUploadingFile( false );
diff --git a/projects/packages/videopress/src/client/block-editor/hooks/use-sync-media/index.ts b/projects/packages/videopress/src/client/block-editor/hooks/use-sync-media/index.ts
index 66ec465133..5c1a39dc9d 100644
--- a/projects/packages/videopress/src/client/block-editor/hooks/use-sync-media/index.ts
+++ b/projects/packages/videopress/src/client/block-editor/hooks/use-sync-media/index.ts
@@ -279,7 +279,7 @@ export function useSyncMedia(
 	 * Store and compare the block attributes
 	 * in order to detect changes on them.
 	 */
-	const { isGeneratingPoster } = useVideoPosterData( attributes );
+	const isGeneratingPoster = false;
 
 	/*
 	 * Block attributes => Media data (sync)
diff --git a/projects/packages/videopress/src/client/block-editor/hooks/use-video-data/index.ts b/projects/packages/videopress/src/client/block-editor/hooks/use-video-data/index.ts
index f5b3b0816c..22c29c21b7 100644
--- a/projects/packages/videopress/src/client/block-editor/hooks/use-video-data/index.ts
+++ b/projects/packages/videopress/src/client/block-editor/hooks/use-video-data/index.ts
@@ -63,6 +63,10 @@ export default function useVideoData( {
 						skipRatingControl,
 					} );
 
+					const responseString = JSON.stringify( response );
+
+					console.log( 'video data: ' + responseString );
+
 					if ( response.duration ) {
 						debug(
 							`video duration available: ${ response.duration }, retried ${ retries } times`,

With the patch applied, first navigate to the post editor on the web.
Either add a new VideoPress block or use an existing one, and make sure to tweak its settings.
It's particularly important to tweak the settings in the Details and Privacy and rating panels, as these are the settings that are not currently being synced.
Save your change on the web and then navigate to the post in the native app.
Verify that the block's settings have been correctly fetched from the web.
Verify also that video data: {video data} has been correctly logged to the console.
Go through a few tries of editing the block on the web then returning the app to ensure syncing is working from the web → app.

github-actions · 2023-04-08T18:34:11Z

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run

bin/jetpack-downloader test jetpack rnmobile/methods-to-fetch-metadata

to get started. More details: p9dueE-5Nn-p2

github-actions · 2023-04-08T18:34:33Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
⚠️ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

SiobhyB · 2023-04-10T22:04:06Z

@jhnstn, @fluiddot, just noting that I've updated this PR based on feedback in the (now closed) Gutenberg PR. It's ready for review again.

fluiddot

@SiobhyB On iOS works as expected, I tried modifying the settings and confirmed they are fetched properly on the native version. However, on Android using an actual device, I'm getting the following error:

Possible Unhandled Promise Rejection (id: 0):
Error: undefined is not a function
Error: undefined is not a function

I tried to debug it via the Chrome debugger and it doesn't happen. This makes me think that there's a call somewhere within the useVideoData hook that is not supported in Hermes.

fluiddot · 2023-04-11T11:19:45Z

projects/packages/videopress/src/client/block-editor/hooks/use-video-data/index.ts

+const isNative = Platform.isNative;
+
+// eslint-disable-next-line no-console
+const debug = isNative ? console.log : debugFactory( 'videopress:video:use-video-data' );


I noticed that calls to debug are not being displayed in the native version because it's not enabled. If we add the following line all debug logs are printed:

debugFactory.enable( 'videopress:*' );

Since enable function should only be called once, I'm wondering if we could add this to the entry point:

import debugFactory from 'debug'; import registerVideoPressBlock from './blocks/video/index'; // eslint-disable-next-line no-undef if ( __DEV__ ) { debugFactory.enable( 'videopress:*' ); } registerVideoPressBlock();

WDYT?

Much nicer, thanks Carlos! Done in 08f9a92.

at() is not supported on some environments, including Hermes, which we rely on for Android.

SiobhyB · 2023-04-12T09:27:14Z

projects/packages/videopress/src/client/block-editor/hooks/use-video-data/index.ts

@@ -72,7 +77,8 @@ export default function useVideoData( {
 				setIsRequestingVideoData( false );

 				// pick filename from the source_url
-				const filename = response.original?.split( '/' )?.at( -1 );
+				const filename =


Debugging the reasons this code wasn't working with Android led me to this line. Although I couldn't find any open issues that specifically mention that Hermes doesn't work with the at() method, I did find it isn't supported by some older browsers, and replacing it fixes the issue with Android. I therefore strongly suspect the cause of the issues was Hermes not supporting the at() method.

Another way of rewriting this could be: const filename = response.original?.split('/').slice(-1)[0]; but I thought that the use of length may be more easily parsed, although it's more verbose. I'd be interested to hear other thoughts or opinions on this.

Debugging the reasons this code #29997 (review) led me to this line. Although I couldn't find any open issues that specifically mention that Hermes doesn't work with the at() method, I did find it isn't supported by some older browsers, and replacing it fixes the issue with Android. I therefore strongly suspect the cause of the issues was Hermes not supporting the at() method.

Great investigation! It's true that there's no reference to the at function in the Hermes language features, so as you mentioned might be an incompatibility with the WebView Android version (it's supported from 92 onwards). However, I checked my Android WebView version and it's 111, so not sure where's the incompatibility coming from 🤔.

Another way of rewriting this could be: const filename = response.original?.split('/').slice(-1)[0]; but I thought that the use of length may be more easily parsed, although it's more verbose. I'd be interested to hear other thoughts or opinions on this.

Another approach would be to import a polyfill for the function. I checked the at documentation and there's a polyfill in the core-js library that we could use. Technically, we could import it into Gutenberg by adding the following code to globals.js :

import 'core-js/proposals/relative-indexing-method';

WDYT?

I've just tested my above suggestion on an actual Android device and works 🎊.

@SiobhyB I took the liberty of opening a PR to include the polyfill in Gutenberg. Let me know if you could take a look and test this PR with the polyfill. Thanks!

Oh, brilliant, thanks for that! Approved the Gutenberg PR and reverted my change to the code in bfc9f37. I'll also go ahead to update the Gutenberg submodule in wordpress-mobile/gutenberg-mobile#5652 once it's merged.

Oh, brilliant, thanks for that! Approved the Gutenberg PR and reverted my change to the code in bfc9f37. I'll also go ahead to update the Gutenberg submodule in wordpress-mobile/gutenberg-mobile#5652 once it's merged.

Great, I've just merged the PR.

Thank you! The Gutenberg Mobile PR is up-to-date now :) It'd be great if we could get this PR merged now, if you think it's ready to be approved.

SiobhyB · 2023-04-12T09:29:25Z

@fluiddot, thank you so much for spotting that! After some digging, I believe the issue lies with Hermes not supporting the at() method. I've added my thoughts around the refactor I've done here, and am looking forward to hearing your thoughts. 🙇‍♀️ This PR's ready for review again.

…hod" This reverts commit 9a3ff4f.

fluiddot

LGTM 🎊 !

I confirm that changes made to the block on the web are reflected when opening the block in the app. I also checked that the block settings are updated even when the post is not refreshed in the app with the latest changes, e.g.:

Change block settings on the web.
Save the post.
Open the same post in the app without refreshing the post list.
Observe a log in the console notifying about some of the settings being out of sync (e.g. 'rating' is out of sync. Updating 'rating' attr from 'G' to 'PG-13').

Siobhan added 2 commits April 8, 2023 19:28

refactor: Enable debugging on native

fe1ba4d

refactor: Skip unnecessary isUserConnected check

06b3340

SiobhyB self-assigned this Apr 8, 2023

SiobhyB added the [Block] VideoPress label Apr 8, 2023

docs: Add details of changes to CHANGELOG

63e70e6

SiobhyB mentioned this pull request Apr 8, 2023

VideoPress block: POST support for wpcom/v2/videopress/meta endpoint wordpress-mobile/gutenberg-mobile#5649

Merged

2 tasks

github-actions bot added [Package] VideoPress [Status] In Progress labels Apr 8, 2023

This was referenced Apr 8, 2023

[RNMobile] Implement useSyncMedia hook on native #29996

Merged

VideoPress block: Fetch VideoPress metadata for native block wordpress-mobile/gutenberg-mobile#5652

Merged

Merge branch 'trunk' into rnmobile/methods-to-fetch-metadata

cedf5ec

SiobhyB marked this pull request as ready for review April 10, 2023 15:51

SiobhyB requested review from jhnstn and fluiddot April 10, 2023 15:58

Siobhan added 4 commits April 10, 2023 21:36

refactor: Replace 'isWeb' conditional w/'isNative'

83af214

refactor: Set up different endpoint for native

aa4bf9b

refactor: Replace isWeb conditionals with isNative

05496c6

refactor: Fix isNative checks

123350f

SiobhyB added [Status] Ready to Merge Go ahead, you can push that green button! [Status] Needs Team Review and removed [Status] In Progress [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 10, 2023

fluiddot reviewed Apr 11, 2023

View reviewed changes

Siobhan added 2 commits April 11, 2023 23:57

Merge branch 'trunk' into rnmobile/methods-to-fetch-metadata

6c80134

refactor: Replace at() with more supported method

9a3ff4f

at() is not supported on some environments, including Hermes, which we rely on for Android.

SiobhyB force-pushed the rnmobile/methods-to-fetch-metadata branch 2 times, most recently from 558671e to 08f9a92 Compare April 12, 2023 09:14

refactor: Enable use of debugFactory on native

08f9a92

SiobhyB commented Apr 12, 2023

View reviewed changes

SiobhyB requested a review from fluiddot April 12, 2023 09:29

fluiddot mentioned this pull request Apr 12, 2023

[RNMobile] Add core-js relative indexing method polyfills WordPress/gutenberg#49749

Merged

[not verified] Revert "refactor: Replace at() with more supported met…

bfc9f37

…hod" This reverts commit 9a3ff4f.

fluiddot approved these changes Apr 12, 2023

View reviewed changes

SiobhyB added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Team Review labels Apr 12, 2023

SiobhyB merged commit e1865ee into trunk Apr 12, 2023

SiobhyB deleted the rnmobile/methods-to-fetch-metadata branch April 12, 2023 15:39

SiobhyB mentioned this pull request Apr 17, 2023

[RNMobile] Sync VideoPress metadata when post is manually saved #30131

Merged

3 tasks

zinigor added this to the Jetpack/12.1 milestone Apr 24, 2023

kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Fetch VideoPress metadata for native block #29997

[RNMobile] Fetch VideoPress metadata for native block #29997

SiobhyB commented Apr 8, 2023 •

edited

Loading

github-actions bot commented Apr 8, 2023 •

edited

Loading

github-actions bot commented Apr 8, 2023 •

edited

Loading

SiobhyB commented Apr 10, 2023 •

edited

Loading

fluiddot left a comment

fluiddot Apr 11, 2023

SiobhyB Apr 12, 2023 •

edited

Loading

SiobhyB Apr 12, 2023

fluiddot Apr 12, 2023

fluiddot Apr 12, 2023

fluiddot Apr 12, 2023 •

edited

Loading

SiobhyB Apr 12, 2023 •

edited

Loading

fluiddot Apr 12, 2023

SiobhyB Apr 12, 2023 •

edited

Loading

SiobhyB commented Apr 12, 2023

fluiddot left a comment

[RNMobile] Fetch VideoPress metadata for native block #29997

[RNMobile] Fetch VideoPress metadata for native block #29997

Conversation

SiobhyB commented Apr 8, 2023 • edited Loading

Related PRs

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Apr 8, 2023 • edited Loading

github-actions bot commented Apr 8, 2023 • edited Loading

SiobhyB commented Apr 10, 2023 • edited Loading

fluiddot left a comment

Choose a reason for hiding this comment

fluiddot Apr 11, 2023

Choose a reason for hiding this comment

SiobhyB Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

SiobhyB Apr 12, 2023

Choose a reason for hiding this comment

fluiddot Apr 12, 2023

Choose a reason for hiding this comment

fluiddot Apr 12, 2023

Choose a reason for hiding this comment

fluiddot Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

SiobhyB Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

fluiddot Apr 12, 2023

Choose a reason for hiding this comment

SiobhyB Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

SiobhyB commented Apr 12, 2023

fluiddot left a comment

Choose a reason for hiding this comment

SiobhyB commented Apr 8, 2023 •

edited

Loading

github-actions bot commented Apr 8, 2023 •

edited

Loading

github-actions bot commented Apr 8, 2023 •

edited

Loading

SiobhyB commented Apr 10, 2023 •

edited

Loading

SiobhyB Apr 12, 2023 •

edited

Loading

fluiddot Apr 12, 2023 •

edited

Loading

SiobhyB Apr 12, 2023 •

edited

Loading

SiobhyB Apr 12, 2023 •

edited

Loading