Implement React Native bridge method for making api requests by mchowning · Pull Request #10779 · wordpress-mobile/WordPress-Android

mchowning · 2019-11-11T21:58:55Z

This PR implements a minimal api call to allow gutenberg to request from javascript the api call necessary to retrieve an image's available sizes.

Related PRs:

gutenberg-mobile: [try] Image Size with networking on native client gutenberg-mobile#1455
fluxc: Add React Native store for making calls to a given path WordPress-FluxC-Android#1424

To test:

Perform this test with both (1) a WP.com site and (2) a self-hosted site. It should work for both:

Run metro server on [try] Image Size with networking on native client gutenberg-mobile#1455 (git submodule update --init --recursive)
Add an image block to gutenberg.
Select an image from the WordPress media Library.
Select settings and choose a different image setting.
Observe the image adjust it's visual size if appropriate
Change to HTML mode.
Check that the image URL has changes according to the selected image size.

TODO before merge

update fluxc hash to tag (perhaps a beta release tag)
update gutenberg-mobe hash to merged gutenberg-mobile hash

PR submission checklist:

I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

peril-wordpress-mobile · 2019-11-11T22:08:20Z

You can test the changes on this Pull Request by downloading the APK here.

planarvoid · 2019-11-12T15:31:18Z

WordPress/src/main/java/org/wordpress/android/ui/posts/reactnative/ReactNativeRequestHandler.kt

+        onSuccess: Consumer<String>,
+        onError: Consumer<String>
+    ) {
+        GlobalScope.launch(bgDispatcher) {


GlobalScope should generally not be used. We should add the CoroutineScope interface to this class and add hooks to onCreate and onDestroy where the first one initializes the scope and the second one cancels it. When I'm thinking about it, it's probably enough to just hook to onDestroy to cancel the job since the ReactNativeRequestHandler is always recreated with the activity.

The disadvantage of the GlobalScope is that it creates a memory leaks (of the two passed Consumers). The operation is also not cancelled when the activity is destroyed.

That makes sense. Thanks for explaining. I've updated this. I did implement this in a slightly different way from what I've seen elsewhere in the app (instead of storing the job in a field, I just got it from the coroutineContext when I needed to cancel it), so let me know if you see any issues with that since I don't have a lot of experience with coroutines.

planarvoid · 2019-11-12T15:32:12Z

WordPress/src/main/java/org/wordpress/android/ui/posts/reactnative/ReactNativeUrlHandler.kt

+
+private const val WPCOM_ENDPOINT = "https://public-api.wordpress.com"
+
+internal fun parseUrlAndParamsForWPCom(pathWithParams: String, wpComSiteId: Long): Pair<String, Map<String, String>>? =


I'd change this file to an injected Utils class so we don't have to retest its functionality in all the places where it's called.

I wanted to avoid creating one of those util classes that doesn't have any state or any dependencies because those types of classes are one of the few cases where imho testing considerations lead to worse code design. Of course, I also didn't realize that we weren't using powermock or mockk in this project 😄 , and I certainly understand there are also good reasons for avoiding libraries that let you mock static methods.

Updated to be an injectable ReactNativeUrlUtil class. 👍

planarvoid · 2019-11-12T15:34:46Z

WordPress/src/main/java/org/wordpress/android/ui/posts/reactnative/ReactNativeRequestHandler.kt

+        response: ReactNativeFetchResponse,
+        onSuccess: (String) -> Unit,
+        onError: (String) -> Unit
+    ) {


I think this should have withContext(mainDispatcher) so you make sure the it's called on the main thread (it's injected with @Named(UI_THREAD) private val mainDispatcher: CoroutineDispatcher)

Because this class is specifically for use only with React Native I'm not sure I agree. Anything sent to React Native is sent asynchronously across the javascript bridge, so keeping it on a background thread on the native side makes sense to me. If we ever were to try to do some UI updates on the native side based on the result of this call (I can't imagine why we would, but never say never 😄 ), I assume the issue would immediately present itself pretty clearly (immediate crash with UI-update-from-background-thread type of exception), and we would be better equipped to determine the best way to handle that issue at that time.

With that said, there's really not any processing done between this point and the bridge, so I don't necessarily think it would do any harm to move it to the main thread here. I just don't want to make that change if this is only going to be used with React Native and there's no benefit for that (React Native) use case. But what do you think?

That makes sense. I just assumed that the callback is somehow affecting the UI and that's why I suggested the shift (basically the first launch jumps to the background thread so I assumed we want to go back to the original thread here).

mchowning · 2019-11-12T15:56:51Z

And now I know to run ktlint 😂

WordPress/src/test/java/org/wordpress/android/ui/posts/reactnative/ReactNativeUrlUtilTest.kt

mchowning · 2019-11-12T20:36:21Z

WordPress/src/main/java/org/wordpress/android/ui/posts/reactnative/ReactNativeRequestHandler.kt

I also toyed with the idea of making this something like:

coroutineContext[Job]?.apply { cancel() } ?: AppLog.e(EDITOR, "Scope could not be cancelled because it does not have a job. This should never happen.")

I decided against this approach since this class creates a CoroutineContext with a Job on initialization, and if this changes so that it starts failing a big, obvious, and immediate failure seems better than a silent memory leak. I do always feel a little weird using !! though, so let me know if anyone prefers this (or some other) approach.

I usually just keep the instance of the job and cancel it directly but I think this approach is fine 👍

Ref of fluxc develop once react native call changes were merged

…ry/gutenberg-image-size-networking-native

planarvoid · 2019-11-15T15:53:45Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

            ((AztecEditorFragment) mEditorFragment).disableContentLogOnCrashes();
        }
+
+        if (mReactNativeRequestHandler != null) mReactNativeRequestHandler.destroy();


NIT: I'd just use {} for consistency.

Updated. 👍

planarvoid · 2019-11-15T15:55:50Z

build.gradle

    daggerVersion = '2.22.1'
-    fluxCVersion = '1.5.1'
+    // FIXME update to FluxC release
+    fluxCVersion = 'd058757009410e19dfbf86cb0a6895d90b97cf43'


edit: you know about this

I just wanted to wait until this PR got some testing before tagging a fluxc release in case any issues were found that needed additional fluxc changes. But hopefully you're right, and I'll just be tagging this commit. 😄

planarvoid · 2019-11-15T15:57:08Z

WordPress/src/main/java/org/wordpress/android/ui/posts/reactnative/ReactNativeRequestHandler.kt

+import javax.inject.Inject
+import javax.inject.Named
+
+class ReactNativeRequestHandler @Inject constructor(


do you think you can write some tests for this class to?

hypest · 2019-11-19T08:27:27Z

Matt is a few hours away (different timezone) so I will re-target this PR to point to develop, and also update the gutenberg-mobile pointer to the latest of the respective PR.

…e-size-networking-native

hypest · 2019-11-19T16:25:38Z

Tried out against a WPCOM site and works 👍

Against a .org (Pressable, Jetpack installed but not activated) site of mine the size setting didn't seem to work and noticed the following network error in logcat:

BasicNetwork.performRequest: Unexpected response code 401 for https://hypestpressable.mystagingwebsite.com/wp-json/wp/v2/media/11?context=view&_locale=user
2019-11-19 18:18:55.155 23223-23223/org.wordpress.android.beta E/WordPress-API: Volley error on https://hypestpressable.mystagingwebsite.com/wp-json/wp/v2/media/11?context=view&_locale=user
    com.android.volley.AuthFailureError
        at com.android.volley.toolbox.BasicNetwork.performRequest(BasicNetwork.java:195)
        at com.android.volley.NetworkDispatcher.processRequest(NetworkDispatcher.java:131)
        at com.android.volley.NetworkDispatcher.processRequest(NetworkDispatcher.java:111)
        at com.android.volley.NetworkDispatcher.run(NetworkDispatcher.java:90)
2019-11-19 18:18:55.165 23223-24026/org.wordpress.android.beta W/WordPress-EDITOR: 'Network Error: ', { [Error: HTTP_AUTH_ERROR]
      framesToPop: 1,
      nativeStackAndroid: 
       [ { methodName: 'lambda$fetchRequest$0',
           lineNumber: 194,
           file: 'RNReactNativeGutenbergBridgeModule.java' },
         { methodName: 'accept', lineNumber: 4, file: null },
         { methodName: 'invoke',
           lineNumber: 34,
           file: 'ReactNativeRequestHandler.kt' },
         { methodName: 'invoke',
           lineNumber: 17,
           file: 'ReactNativeRequestHandler.kt' },
         { methodName: 'handleResponse',
           lineNumber: 80,
           file: 'ReactNativeRequestHandler.kt' },
         { methodName: 'performGetRequestForSelfHostedSite',
           lineNumber: 69,
           file: 'ReactNativeRequestHandler.kt' },
         { methodName: 'invokeSuspend', lineNumber: 16, file: null },
         { methodName: 'resumeWith',
           lineNumber: 33,
           file: 'ContinuationImpl.kt' },
         { methodName: 'resumeUninterceptedMode',
           lineNumber: 46,
           file: 'ResumeMode.kt' },
         { methodName: 'afterCompletionInternal',
           lineNumber: 31,
           file: 'Scopes.kt' } ],
      userInfo: null,
      code: 'EUNSPECIFIED',
      line: 11828,
      column: 26,
      sourceURL: 'http://localhost:8081/index.bundle?platform=android&dev=true&minify=false' }
2019-11-19 18:18:55.458 23223-24026/org.wordpress.android.beta W/WordPress-EDITOR: Warning: componentWillReceiveProps is deprecated and will be removed in the next major version. Use static getDerivedStateFromProps instead.
    
    Please update the following components: withAnimatable(View)
    
    Learn more about this warning here:
    https://fb.me/react-async-component-lifecycle-hooks

While investigating I saw that Gutenberg was outdated and not activated on the site. After updating and activating I got the following in logcat when opening the post-with-the-image I tried before:

2019-11-19 18:22:40.382 23223-23262/org.wordpress.android.beta E/Volley: [5233] BasicNetwork.performRequest: Unexpected response code 404 for https://public-api.wordpress.com/rest/v1.1/sites/0/post/46/diffs/?locale=en_US
2019-11-19 18:22:40.383 23223-23223/org.wordpress.android.beta E/WordPress-API: Volley error on https://public-api.wordpress.com/rest/v1.1/sites/0/post/46/diffs/?locale=en_US
    com.android.volley.ClientError
        at com.android.volley.toolbox.BasicNetwork.performRequest(BasicNetwork.java:199)
        at com.android.volley.NetworkDispatcher.processRequest(NetworkDispatcher.java:131)
        at com.android.volley.NetworkDispatcher.processRequest(NetworkDispatcher.java:111)
        at com.android.volley.NetworkDispatcher.run(NetworkDispatcher.java:90)

Then, tapping on the image got me the same 401 error (in logcat).

Not sure if some other configuration is weird on my .org site. I'll try another too.

hypest

Tried on a newly created Pressable site and the size setting works fine. I do get some 403s during the while process but, the setting works nevertheless.

I think this is good to merge and we can follow up with dedicated PR(s) for any issue.

mchowning · 2019-11-19T23:12:10Z

Thanks for the review @hypest !

Errors in Logs

I investigated the errors you noticed in the logs a bit and confirmed that they are not causing problems for this PR. I think the root of the issue is that in the native code we’re essentially using the local database key as the image’s id until we get an update from the web. Since we make a getMedia request every time the id changes we make a request with the initial id, which fails. But it works out in the end because the web responds to the upload with the proper id, and we then make another getMedia request with that id, which succeeds. That’s why the error we’re seeing in the logs isn’t causing a problem.

On Android, it looks like we’re using the database id (if there is not a media id from the wp api yet) here.
I also see this behavior on iOS, where I think we’re using a similar local id (NSManagedObjectId in particular) here.

api call missing images

I have noticed some intermittent behavior where an image that is showing in the media library on both web and on mobile is not appearing in the api call. During testing it appeared that this was happening on your testing site @hypesty since you were uploading images, but "https://hypestpressable.mystagingwebsite.com/wp-json/wp/v2/media/" continued to show only a single media file (which is an audio file).

As an example on my jurassic ninja site, if I hit the rest api I only get a single image with id 21:

▶ curl 'https://damp-ducks.jurassic.ninja/wp-json/wp/v2/media/' | jq '.[] | {id}'
{
  "id": 21
}

If I ssh in though and run wp media regenerate --only-missing, there is a second image with id 5 (which shows up in my media library):

Found 2 images to regenerate.
1/2 No thumbnail regeneration needed for "Screen Shot 2019-10-14 at 9.30.25 AM" (ID 21).
2/2 No thumbnail regeneration needed for "Screen Shot 2019-11-05 at 9.04.52 PM" (ID 5).

If I create a post on the web using this image (id 5), I can successfully resize the image with no issues. Even loading the post on mobile will display with the updated size (although I cannot change the size on mobile).

I can directly access image 21 using the rest api:

▶ curl 'https://damp-ducks.jurassic.ninja/wp-json/wp/v2/media/21' | jq
{
  "id": 21,
   ...
}

If I try and directly access image 5, I get a 401 forbidden error:

▶ curl 'https://damp-ducks.jurassic.ninja/wp-json/wp/v2/media/5' | jq
{
  "code": "rest_forbidden",
  "message": "Sorry, you are not allowed to do that.",
  "data": {
    "status": 401
  }
}

Interestingly, if I curl an actually non-existent image (i.e., an id other than 5 or 21) I do not get a 401 forbidden error like I get for image 5. Instead I now get a 404 invalid id error:

▶ 'https://damp-ducks.jurassic.ninja/wp-json/wp/v2/media/6'
{
  "code": "rest_post_invalid_id",
  "message": "Invalid post ID.",
  "data": {
    "status": 404
  }
}

At this point, I cannot find a way to consistently recreate this issue, but I think it may be worth adding to the issue tracking work for this feature, wdyt @etoledom ?

…e-size-networking-native

mchowning added the gutenberg-mobile label Nov 11, 2019

mchowning added this to the 13.7 milestone Nov 11, 2019

mchowning requested review from cameronvoell, hypest, marecar3, maxme and planarvoid November 11, 2019 21:58

mchowning self-assigned this Nov 11, 2019

mchowning mentioned this pull request Nov 11, 2019

Add React Native store for making calls to a given path wordpress-mobile/WordPress-FluxC-Android#1424

Merged

1 task

etoledom mentioned this pull request Nov 11, 2019

[try] Image Size with networking on native client wordpress-mobile/gutenberg-mobile#1455

Merged

1 task

mchowning force-pushed the try/gutenberg-image-size-networking-native branch from 54766bf to 8ef5ac9 Compare November 12, 2019 02:00

Implement React Native bridge method for making api requests

c026d8d

mchowning force-pushed the try/gutenberg-image-size-networking-native branch from 8ef5ac9 to c026d8d Compare November 12, 2019 14:16

planarvoid reviewed Nov 12, 2019

View reviewed changes

Remove GlobalScope from ReactNativeRequestHandler

5738591

wpmobilebot reviewed Nov 12, 2019

View reviewed changes

WordPress/src/test/java/org/wordpress/android/ui/posts/reactnative/ReactNativeUrlUtilTest.kt Outdated Show resolved Hide resolved

mchowning commented Nov 12, 2019

View reviewed changes

Move React Native url logic from top-level functions to util class

d23c1ee

mchowning force-pushed the try/gutenberg-image-size-networking-native branch from a0f2571 to d23c1ee Compare November 12, 2019 20:53

mchowning added 4 commits November 12, 2019 21:44

Update url parsing logic to handle urls without query params

99955a5

Use uri to parse url path

80be1ef

Update fluxc ref

07e5a09

Ref of fluxc develop once react native call changes were merged

Merge remote-tracking branch 'origin/gutenberg/release-1.17.0' into t…

cbb3360

…ry/gutenberg-image-size-networking-native

mchowning requested a review from planarvoid November 15, 2019 15:44

planarvoid reviewed Nov 15, 2019

View reviewed changes

mchowning added 4 commits November 15, 2019 12:35

Add tests for ReactNativeRequestHandler

25402e6

Make if-statement multiline

161d30a

Update gutenberg ref

010befd

Update gutenberg ref

5036494

mchowning modified the milestones: 13.7, 13.8 Nov 18, 2019

Update fluxc to tagged version

240a6ea

hypest changed the base branch from gutenberg/release-1.17.0 to develop November 19, 2019 08:27

hypest and others added 3 commits November 19, 2019 10:28

Update the gutenberg-mobile ref to latest of its PR

47c1d02

Merge branch 'develop' into try/gutenberg-image-size-networking-native

f8e316b

Merge remote-tracking branch 'origin/develop' into try/gutenberg-imag…

536635d

…e-size-networking-native

hypest approved these changes Nov 19, 2019

View reviewed changes

etoledom mentioned this pull request Nov 20, 2019

Image Sizes behavioral issues wordpress-mobile/gutenberg-mobile#1593

Closed

8 tasks

Update gutenberg ref

037189a

etoledom mentioned this pull request Nov 20, 2019

[RNMobile] Implement getMedia on Image block. WordPress/gutenberg#17979

Merged

Merge remote-tracking branch 'origin/develop' into try/gutenberg-imag…

526e20a

…e-size-networking-native

etoledom merged commit 8cad939 into develop Nov 20, 2019

etoledom deleted the try/gutenberg-image-size-networking-native branch November 20, 2019 09:40

mchowning mentioned this pull request Nov 22, 2019

Mobile gallery block - Upload options wordpress-mobile/gutenberg-mobile#1610

Closed


		private const val WPCOM_ENDPOINT = "https://public-api.wordpress.com"

		internal fun parseUrlAndParamsForWPCom(pathWithParams: String, wpComSiteId: Long): Pair<String, Map<String, String>>? =

Conversation

mchowning commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To test:

TODO before merge

Uh oh!

peril-wordpress-mobile bot commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchowning Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchowning commented Nov 12, 2019

Uh oh!

Uh oh!

mchowning Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

planarvoid Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hypest commented Nov 19, 2019

Uh oh!

hypest commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hypest left a comment

Choose a reason for hiding this comment

Uh oh!

mchowning commented Nov 19, 2019

Errors in Logs

api call missing images

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mchowning commented Nov 11, 2019 •

edited

Loading

peril-wordpress-mobile bot commented Nov 11, 2019 •

edited

Loading

mchowning Nov 12, 2019 •

edited

Loading

mchowning Nov 12, 2019 •

edited

Loading

planarvoid Nov 15, 2019 •

edited

Loading

hypest commented Nov 19, 2019 •

edited

Loading