[RNMobile] Fix encoding of MediaInfo metadata property (iOS)

[fluiddot]

Related PRs:

• [RNMobile] Fix encoding of MediaInfo metadata property (iOS) wordpress-mobile/gutenberg-mobile#5584

What?

Fix encoding of MediaInfo metadata property passed when inserting media in a Media block. This fix only affects iOS.

NOTE: This bug is not affecting the editor's logic as metadata property is not being used yet.

Why?

The metadata property was introduced in #48994, however, the way it's encoded doesn't match the expectations:

Current	Expected
{ "metadata": [ { "param": "value" } ] }	{ metadata": { "param": "value" } }

Note that in the current output, it's returning an array but it should be a plain object.

How?

The main complexity of encoding metadata property is that we don't know its structure which is a requirement when encoding an Encodable class/struct. I tried using a generic type but still, it led to other issues. Finally, I decided to omit the property when encoding MediaInfo and encode add the metadata manually in encodeForJS function when we pass the data to React Native.

Additionally, I removed the encode function from MediaInfo. In theory, if we don't customize the encoding (i.e. use a different name or value) we can rely on the default implementation.

Testing Instructions

Preparation:

1. Build and open the demo app on iOS.
2. Connect the demo app with the local Metro server.
3. Apply the following patch file:

diff --git forkSrcPrefix/packages/block-library/src/video/edit.native.js forkDstPrefix/packages/block-library/src/video/edit.native.js
index 99225ba5d5c569d19a158f5b1a203c68aa353c33..c801aac0ececa419b8029d5b20355aa9a5f3ee59 100644
--- forkSrcPrefix/packages/block-library/src/video/edit.native.js
+++ forkDstPrefix/packages/block-library/src/video/edit.native.js
@@ -154,7 +154,9 @@ class VideoEdit extends Component {
 		this.setState( { isUploadInProgress: false } );
 	}
 
-	onSelectMediaUploadOption( { id, url } ) {
+	onSelectMediaUploadOption( media ) {
+		const { id, url } = media;
+		console.log( { media } );
 		const { setAttributes } = this.props;
 		setAttributes( { id, src: url } );
 	}
diff --git forkSrcPrefix/packages/block-library/src/image/edit.native.js forkDstPrefix/packages/block-library/src/image/edit.native.js
index 3ffd377005af9227a030b24019d05b0d92b45ccd..7cee99550c0c7b95d0e2e4645c39b9f01d900b5b 100644
--- forkSrcPrefix/packages/block-library/src/image/edit.native.js
+++ forkDstPrefix/packages/block-library/src/image/edit.native.js
@@ -429,6 +429,7 @@ export class ImageEdit extends Component {
 	onSelectMediaUploadOption( media ) {
 		const { imageDefaultSize } = this.props;
 		const { id, url, destination } = this.props.attributes;
+		console.log( { media } );
 		const mediaAttributes = {
 			id: media.id,
 			url: media.url,

Image block

1. Add a Image block.
2. Select "WordPress Media Library" option.
3. Check the logs and observe that the media object contains its properties, e.g.:

{
  "media": {
    "alt": "A snow-capped mountain top in a cloudy sky with red-leafed trees in the foreground",
    "caption": "Mountain",
    "id": 1,
    "metadata": {},
    "type": "image",
    "url": "https://cldup.com/cXyG__fTLN.jpg"
  }
}

Video block

1. Add a Video block.
2. Select "WordPress Media Library" option.
3. Check the logs and observe that the media object contains its properties, e.g.:

{
  "media": {
    "caption": "Cloudup",
    "id": 2,
    "metadata": { "extraID": "AbCdE" },
    "type": "video",
    "url": "https://i.cloudup.com/YtZFJbuQCE.mov"
  }
}

Testing Instructions for Keyboard

N/A

Screenshots or screencast

N/A

[fluiddot]

⚠️ Looks like there is a build failure on iOS due to these changes:

gutenberg/packages/react-native-bridge/ios/RNReactNativeGutenbergBridge.swift:472:37: error: protocol 'Encodable' as a type cannot conform to the protocol itself
            let metadataData = try? JSONEncoder().encode(self.metadata),
                                    ^
gutenberg/packages/react-native-bridge/ios/RNReactNativeGutenbergBridge.swift:472:37: note: only concrete types such as structs, enums and classes can conform to protocols
            let metadataData = try? JSONEncoder().encode(self.metadata),
                                    ^
gutenberg/packages/react-native-bridge/ios/RNReactNativeGutenbergBridge.swift:472:37: note: required by instance method 'encode' where 'T' = 'Encodable'
            let metadataData = try? JSONEncoder().encode(self.metadata),

It's interesting because I can't reproduce it when using Xcode 14+. In fact, I only get it when using Xcode 13 🤔. I'll check this deeper.

UPDATE: This is actually caused by the Swift version:

• Swift version 5.6 (Xcode 13): Fails ❌
• Swift version 5.7 (Xcode 14) Succeeds ✅

Specifically, seems related to a new feature introduced in 5.7: Implicitly Opened Existentials

[fluiddot]

⚠️ Looks like there is a build failure on iOS due to these changes:

gutenberg/packages/react-native-bridge/ios/RNReactNativeGutenbergBridge.swift:472:37: error: protocol 'Encodable' as a type cannot conform to the protocol itself
            let metadataData = try? JSONEncoder().encode(self.metadata),
                                    ^
gutenberg/packages/react-native-bridge/ios/RNReactNativeGutenbergBridge.swift:472:37: note: only concrete types such as structs, enums and classes can conform to protocols
            let metadataData = try? JSONEncoder().encode(self.metadata),
                                    ^
gutenberg/packages/react-native-bridge/ios/RNReactNativeGutenbergBridge.swift:472:37: note: required by instance method 'encode' where 'T' = 'Encodable'
            let metadataData = try? JSONEncoder().encode(self.metadata),

It's interesting because I can't reproduce it when using Xcode 14+. In fact, I only get it when using Xcode 13 🤔. I'll check this deeper.

Seems that using Xcode 14 in CI could do the trick but, as discussed internally with @geriux, we can't do this without bumping Appium to version 2. Since this is out of the scope of the fix, I'll drop this approach and work on a different one. Hence I'm changing the PR to draft.

[fluiddot]

Seems that using Xcode 14 in CI could do the trick but, as discussed internally with @geriux, we can't do this without bumping Appium to version 2. Since this is out of the scope of the fix, I'll drop this approach and work on a different one. Hence I'm changing the PR to draft.

I dropped using the type Encodable for metadata and simply use a Dictionary. This should be enough as metadata will be mainly used in React Native and Dictionaries are converted to JS objects when passing the bridge. In fact, MediaInfo is encoded to a Dictionary when sent to React Native 😅 .

The PR is ready for review again, thanks!

[github-actions]

Flaky tests detected in 04ee4cf.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4501782410
📝 Reported issues:

• [Flaky Test] allows tabbing to the inspector from the last block #48504 in /test/e2e/specs/site-editor/writing-flow.spec.js

[SiobhyB]

LGTM! I went through the testing steps and confirmed the output showed up as expected for both the image and video blocks:

Image	Video

Thanks Carlos 🙇‍♀️