Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: android should intercept request #3422

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

AbrahamOsmondE
Copy link

@AbrahamOsmondE AbrahamOsmondE commented Apr 30, 2024

Initial attempts at implement the shouldInterceptRequest method of Android Webview Client.

Results:

Screenshot 2024-04-30 at 11 44 39 PM

@AbrahamOsmondE
Copy link
Author

AbrahamOsmondE commented May 1, 2024

Current implementation only supports returning a string from React Native to Android, where it will be converted to a InputStream in the form of a byte array

Areas of improvement:

  • Refactor the Java Android method that react native calls to include status code, mime type, reason, headers, and other relevant WebResourceResponse parameters

  • Refactor the native event being passed to the onShouldInterceptRequest to include values from WebResourceRequest

Concerns

  • Should there be a size limit on what React Native can pass through JSI? Would the 1 second timeout be enough?

  • What are the limitations of JSI? What can be passed through it? Is it only primitive data types?

  • Is there a way to implement other forms of InputStream? Below is a list of InputStream subclasses

  • AssetInputStream

  • BackupDataInputStream

  • ByteArrayInputStream

  • FileInputStream

  • ObjectInputStream

  • PipedInputStream

  • SequenceInputStream

  • StringBufferInputStream

@@ -312,6 +380,19 @@ protected WritableMap createWebViewEvent(WebView webView, String url) {
return event;
}

protected WritableMap createWebViewEventTest(WebView webView, String url) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was created because createWebviewEvent returns an error related to threads when setting one of the properties, will refactor this function

@Titozzz
Copy link
Collaborator

Titozzz commented May 2, 2024

Gave a quick read to the PR:

  • it seems that there is a lot of thing that are copy/pasted from shouldOverrideUrlLoading, could we imagine sharing most of the code?
  • 1000 ms seems like forever to block the main thread, did you encounter cases that were that slow during your testing ?
  • You are talking about JSI, but (at least for now) this module is also compatible with the old arch so we cannot go that way I think.

I'm not an expert regarding all the points you made within your concerns (that are very much valid)

@AbrahamOsmondE
Copy link
Author

Gave a quick read to the PR:

  • it seems that there is a lot of thing that are copy/pasted from shouldOverrideUrlLoading, could we imagine sharing most of the code?
  • 1000 ms seems like forever to block the main thread, did you encounter cases that were that slow during your testing ?
  • You are talking about JSI, but (at least for now) this module is also compatible with the old arch so we cannot go that way I think.

I'm not an expert regarding all the points you made within your concerns (that are very much valid)

Hey Titozzz, thanks for your reply. I'll look to refactor the code for reusability with shouldOverrideUrlLoading and will do some testing on the timeouts

Do you know who I should ask for more information on JSI, or Native to React native communication? I have taken a look at this page https://reactnative.dev/docs/communication-ios, but it doesn't go in depth on the limitations

@Titozzz
Copy link
Collaborator

Titozzz commented May 10, 2024

The part you linked is using the old arch. So it is mainly outdated

@AbrahamOEd
Copy link

AbrahamOEd commented May 20, 2024

Several observations during testing:

  • shouldInterceptRequest is never called on the main thread (checked using Looper.myLooper() == Looper.getMainLooper())
  • This would mean we will not be able to utilize createWebViewEvent (as WebView methods must be called within the main thread), as it populates the WebviewEvent object with the following:
event.putBoolean("loading", !mLastLoadFailed && webView.getProgress() != 100);
        event.putString("title", webView.getTitle());
        event.putBoolean("canGoBack", webView.canGoBack());
        event.putBoolean("canGoForward", webView.canGoForward());

Thus, shouldInterceptRequest should have its own event object which will be a subset of WebviewEvent

  • Since shouldInterceptRequest is never called on the main thread, the timeout imposed for this function does not refer to how long the main thread is blocked, rather how long should the function wait before resorting to the default request
  • The current implementation only allows for a static intercepting of a request. This can be changed to allow dynamic page interception by changing the callback to be asynchronous
  • Using the infinitered website, we perform the following test and notice that it would usually take 80 - 300ms to intercept a request (with the bottleneck in downloading the url)
onShouldInterceptRequest={async (event) => {
            if (event.url === 'https://infinite.red/') {
              const start = new Date().getTime();
              const response = await fetch('https://infinite.red/');
              const html = await response.text();
              const modifiedHtml = html.replace('<body>', '<body><p>Request succesfully intercepted!</p>');
              console.log("onShouldInterceptRequest end", new Date().getTime() - start)
              return modifiedHtml
            }
          }}
  • One URL could trigger shouldInterceptRequest 50 - 150 times

AbrahamOsmondE and others added 2 commits May 20, 2024 17:01
- refactor common code between shouldInterceptRequest and shouldOverrideUrlLoading
- allow async/await on the onShouldInterceptRequest callback
@AbrahamOEd AbrahamOEd force-pushed the feat/should-intercept-request branch from 402e83d to 13a73cb Compare May 20, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants