react-native-webview / react-native-webview

React Native Cross-Platform WebView
https://github.com/react-native-community/discussions-and-proposals/pull/3
MIT License
6.58k stars 3.01k forks source link

[DISCUSS] Cross platform API for RN<>WebView 2-way RPC/message passing #66

Closed fungilation closed 5 years ago

fungilation commented 6 years ago

Referencing (my) prior discussions regarding this, for context and so we don't repeat here. In reverse chronological order:

This consistent, robust, cross platform API is important for any app that blends native and webviews. I list mine as one. A requirement of this API is that an injected postMessage for purpose of RN message passing, not be overridden by a website's JS in the WebView. Which is currently the case with RN core's WebView, and presumably this forked react-native-webview. It could be achieved (and have been attempted in prior discussions and in RN core's WebView) by loading RN's injected postMessage last, but that's unreliable should the website create after onLoad a function of the same name, alongside iframe issues. This is not hypothetical as it does get overridden for a fact, when loading google.com in WebView for example.

A simple function rename that doesn't use the web standard of postMessage would sidestep what is effectively a name collision issue. Call it rnPostMessage or anything else. Open to suggestions on a better alternative to renaming that actually has a guarantee for not being overridden by non-locally loaded JS within WebView.

This new API should consider/port code (if tested as reliable) from react-native-wkwebview, which is iOS only. And implement the same API for other platforms. There is also working RN<>WebView bridge API at react-native-webview-bridge that's well established (old but fully functional, with google.com loaded in webview, and for Android and iOS), but that's based on UIWebView and not WKWebView on the iOS side.

jamonholmgren commented 6 years ago

Thanks @fungilation !

One other interesting API I found when perusing Github was this one:

https://github.com/pinqy520/react-native-webview-invoke

It exposes an API where you can call an async function directly, as long as you control both the web and mobile side of the connection. Under the hood, it still uses postMessage, but I thought I'd bring this up in case it was interesting as a potential API.

fungilation commented 6 years ago

Interesting. Although requiring below on the Web side is no good. New API should support any WebView src, not only local js:

import invoke from 'react-native-webview-invoke/browser'
Titozzz commented 6 years ago

@fungilation So I was using a fork of alinz/react-native-webview-bridge until I started using this package. I wrote a simple piece of code that allowed me to be 100% non breaking with what was there before. This does not mean it's safe regarding postMessage naming and such but I'll just post it here in case anyone needs it.

const injectedJavascript = `(function() {
  window.WebViewBridge = {
    onMessage: function() {
      return null;
    },
    send: function(data) {
      window.postMessage(data, '*');
    },
  };
  var event = new Event('WebViewBridge');
  window.dispatchEvent(event);
})()`;

const generateOnMessageFunction = data =>
  `(function() {
    window.WebViewBridge.onMessage(${JSON.stringify(data)});
  })()`;

sendMessageToWebView = (message) => {
    this.webView.current.injectJavaScript(generateOnMessageFunction(message));
  };

Then you just need to use onMessage prop to listen and sendMessageToWebView to send. From the website the api is unchanged. You can use webviewBrigde send and onMessage as before :)

KoenLav commented 6 years ago

I totally agree this is a requirement for even considering React Native, with a WebView, as a suitable replacement for Cordova (without using the native components, rather than their web counterparts).

I created the following schematic and we will be investing some time in working up an example, any advice on improving this implementation strategy would be appreciated!

Also some idea of whether we would want to move this functionality into this package (I believe it should be included) or maintain it as a separate package would be nice.

img_20181002_130735

Titozzz commented 6 years ago

Well that is what I'm doing already (excepted for the callback array, as I'm more in the just dispatching actions without waiting for answers mind)

KoenLav commented 6 years ago

@Titozzz my end goal is to allow calling any react-native package directly from the WebView as if we didn't even have to cross the boundary between the two.

I have a simple (in a good way) working example of a Bridge which does the following; ReactNativeBridge.call('functionName', arg1, arg2, arg..., callback) where you can pass as many arguments as you want and callback is of the form (err, res). On the native side you define what functionName does (and whether it is sync/async), quite similar to the invoke package.

@fungilation I also forked this package and modified the Android implementation of postMessage (in the webview) to no longer overwrite the window.postMessage (this caused issues for our application).

I didn't implement this using injectedJavascript yet, as I actually don't mind importing a package on both sides (native & web), but will work on that soon.

Are there any other use cases I should take into account?

It is my intention to create a PR for this feature which could be enabled by passing bridge={true} to the Webview component and add documentation on how to use it in the WebView.

One limitation of my approach, which could be easily resolved, is that it currently only allows calling from WebView to Native, not the other way around.

Happy to hear your thoughts!

Titozzz commented 6 years ago

Okay I've updated all the repository issues regarding postMessage πŸ˜„ .

Let's keep the discussion focused in this thread only, or we will get lost! 😒

I think everyone agrees that using postMessage I not a good idea for several reasons!

Listing what we need:

Nice to have ..?

KoenLav commented 6 years ago

@Titozzz for our use-case async/callback return values are a must-have.

The BridgedWebview repository (https://github.com/KoenLav/bridged-webview) I created already fulfills the first two checks on your list and covers async/callback returns values.

I'm not sure however how you interpret two-way communication? The approach I took is pretty much centered around the WebView having the 'lead' (dictating what is happening), but I can imagine use-cases where one would want the native side of things to have the lead, and dictate what happens in the WebView. What's your take on that?

I could quite easily extend the classes to also cover this use-case, but if we don't expect anyone to use it like that it could just make things confusing.

Titozzz commented 6 years ago

Yeah, I actually have a use-case were both side are reacting to each others, so I don't really have one master but both are emmiting events that the other is free to listen to, which is why I was not using return values, as I'm not waiting for anything. Would not hurt having it tho πŸ‘

Titozzz commented 6 years ago

So I've read your code (super fast, sorry it's late here 😴). I'm however not sure that we want to implement something that is THAT specific. What I mean is that you defined a 'RFC' to communicate and do function calls and all that, but in the end what I think this repo should provid is a solid communication (eg: just posting JSON payload) both ways and then leave the implementation back to the users, as you did in your repository. I don't know if i'm clear. I'll try to explain better tomorrow !

KoenLav commented 6 years ago

I see (with regards to the use-case).

The reason I didn't invest any time into Native => WebView yet is also because it's pretty easy to solve once WebView => Native works. For example: injectJavascript(const result = doSomethingInWebview(); ReactNativeBridge.call('anyNativeFunction', result, someOtherVariables...);

=====

Given the use-case you are describing (just sending data from one side and having it picked up on the other side) I can imagine this might seem specific. What do you mean by solid communication? I think this repo already does that just fine (on Android, at least, not sure of the status of the WKWebView included at the moment). If there is still work to do in making sure that postMessage/injectJavascript work fine on all supported platforms then that definitely needs to happen first!

In our use-case we actually want to port a (Meteor) web-app from Cordova to React Native, but for some functionality we rely on native code (getting the unique device ID, communicating via TCP sockets, etc.). What I was going for is providing a way to do anything from the WebView about as easily as you could from the native side of things.

I don't think this is THAT specific; it can also be used to simply pass JSON from one side to the other, as it doesn't require setting a callback and the underlying postMessage function is still available as well, for users wanting more fine-grained control.

I simply defined a standardized way of instructing the native side how to handle specific messages, which I think is much easier to understand for a new user than what is currently out there (react-native-webview-bridge, react-native-webview-messaging, react-native-webview-invoke) AND supports all use-cases the packages I just mentioned support.

Looking through the code sample you posted earlier I did however notice that I didn't implement the onMessage function within the WebView yet, if I add that then the statement above (supporting all use-cases) is actually true, as far as I can tell.

Happy to hear your thoughts!

fungilation commented 6 years ago

Meteor/Cordova! I tried and failed with my last mobile app project using it, good to see another migrant to RN.

I'm copying over my comment from the other issue as I think it's useful for discussion of what 2-way communication means to me, and what I think a good API looks like based on the tried and true in existing libraries:


I'm for a breaking change in API. Especially if that means sidestepping entirely security issues with origin. As a user, it doesn't matter as long as it's documented and works. I've advocated for renaming so postMessage / onMessage equivalent APIs can work independent of what the WebView's original site javascript does. Sticking with web-centric APIs for RN<>webview communication has never been a wanted feature (for me).

For starter, instead of proposing completely new API, what about just borrow one from existing, working library? react-native-wkwebview's API looks comprehensive, but they are using the same window.postMessage and onMessage API. Do they guarantee against function overwriting from websites' javascript and security issues with origin?

react-native-webview-bridge I use actively with my own fork for my own app. It's not the best API but it works, and it works for both iOS and Android. We could start with that as far as API definition? With one exception, on calling from RN to WebView:

react-native-webview-bridge uses this to send string to WebView:

webviewbridge.sendToBridge("hello from react-native")

but react-native-wkwebview's way is better, since users can directly evaluate JS, and define separate functions for different purposes injected in webview. String sending in react-native-webview-bridge means a nest of conditionals to handle a list of strings sent from RN, and it gets messy.

this.webview.evaluateJavaScript('receivedMessageFromReactNative("Hello from the other side.")')
KoenLav commented 6 years ago

@fungilation I read your comments to most issues before I started working on this and I think the package linked in my latest PR, and the API it suggests, will suit your use case.

Any comments are much appreciated!

Titozzz commented 6 years ago

Given the use-case you are describing (just sending data from one side and having it picked up on the other side) I can imagine this might seem specific. What do you mean by solid communication? I think this repo already does that just fine (on Android, at least, not sure of the status of the WKWebView included at the moment). If there is still work to do in making sure that postMessage/injectJavascript work fine on all supported platforms then that definitely needs to happen first!

So about that. This is the part I mean to address in that repository and that really belongs here imho, the current bridge is not OK, as it is using postMessage and this has major drawbacks. We will introduce a breaking change to implement something like react-nativew-webview-bridge (more or less), that will allow for a safer, more reliable communication.


In our use-case we actually want to port a (Meteor) web-app from Cordova to React Native, but for some functionality we rely on native code (getting the unique device ID, communicating via TCP sockets, etc.). What I was going for is providing a way to do anything from the WebView about as easily as you could from the native side of things.

I don't think this is THAT specific; it can also be used to simply pass JSON from one side to the other, as it doesn't require setting a callback and the underlying postMessage function is still available as well, for users wanting more fine-grained control.

I simply defined a standardized way of instructing the native side how to handle specific messages, which I think is much easier to understand for a new user than what is currently out there (react-native-webview-bridge, react-native-webview-messaging, react-native-webview-invoke) AND supports all use-cases the packages I just mentioned support.

Regarding this, I'm not sure that this should belong directly inside the repository and be loaded for everyone. As I said once the bridge is reliable, I feel like (and it's my opinion) that most webview users have very specific use cases, and I think we should not decide how to implement this for them.

As shown above, once the bridge is strong, you can easily with a few lines implement whatever API you need.

One thing we could however do it add some example in the docs (something like your code) to give a more beginner user a template to follow / understand to do whatever he needs.

KoenLav commented 6 years ago

@Titozzz I looked into the implementation of react-native-webview-bridge and I don't think it is much/any safer than the currently implemented solution here.

Can you identify where their implementation is better and/or what is not 'strong' about the current implementation in this repository?

Titozzz commented 6 years ago

Sure I gave the example only because they are not using window.postMessage.

Issue with the current implem as stated in the docs:

Security Warning: Currently, onMessage and postMessage do not allow specifying an origin. This can lead to cross-site scripting attacks if an unexpected document is loaded within a WebView instance. Please refer to the MDN documentation for Window.postMessage() for more details on the security implications of this.

Plus when you instantiate the webview it messes with window.postMessage that was previously defined.

This is why we will probably define something else than postMessage πŸ˜„

KoenLav commented 6 years ago

@Titozzz my PR already resolves the second issue.

The cross site scripting one I think is a more difficult one, but I could also take a look at that.

Do you have any suggestions?

Oh wait, I don't think it's that difficult... We could simply define which origins are accepted on the React Native side.

Titozzz commented 6 years ago

Yeah, we need to stop using postMessage behind the scenes !

KoenLav commented 6 years ago

I'm not sure I understand you correctly: in my opinion we need to stop using window.postMessage, but a AnyGlobal.postMessage function would be fine, agreed?

The AnyGlobal.postMessage function is simply a function which is inserted into the window and enables us to send messages to the React Native side.

We could name the function anyhow we wanted as it simply makes use of a JavascriptInterface on Android: https://github.com/react-native-community/react-native-webview/blob/master/android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java#L261

https://developer.android.com/reference/android/webkit/JavascriptInterface

Or a WKScriptMessageHandler on iOS WKWebView:

https://github.com/react-native-community/react-native-webview/blob/master/ios/RNCWKWebView.m#L146

https://spin.atomicobject.com/2016/09/01/sharing-web-data-wkwebview/

It does appear however, when reading the iOS example, that in iOS it is only possible to implement a variable in window.webkit.messageHandlers.SomeName.postMessage.

So I guess, for consistency, we should do the same for Android (which lets us insert the bridge in any place).

Titozzz commented 6 years ago

yeah sure when I say stop using post message I'm talking about the one inside the window πŸ˜› .

KoenLav commented 6 years ago

Which requires a breaking change, which is the only thing holding my PR back in your eyes?

What do you think of my suggestion of implementing a switch called something like "overWriteWindowPostmessage" to re-enable the old behavior?

Titozzz commented 6 years ago

oh, I need to re-read that PR. Will do soon !

satya164 commented 6 years ago

A requirement of this API is that an injected postMessage for purpose of RN message passing, not be overridden by a website's JS in the WebView. Which is currently the case with RN core's WebView, and presumably this forked react-native-webview. It could be achieved (and have been attempted in prior discussions and in RN core's WebView) by loading RN's injected postMessage

One thing I didn't understand is why is this necessary? You have control over the website's code, so why can't you prevent the JS in the website from overriding postMessage?

jamonholmgren commented 6 years ago

@satya164 I think the problem comes in when someone accidentally (or otherwise) navigates to an untrusted website, which can also use the window.postMessage protocol to send messages to native. By injecting the custom MyModule.postMessage, you control which web pages have a pathway to communicating back to React Native. Does that help?

satya164 commented 6 years ago

@jamonholmgren so it's when you're injecting JS to a website you don't control?

I guess in that case, using __REACT_WEB_VIEW_BRIDGE.postMessage should work as expected. Though __REACT_WEB_VIEW_BRIDGE doesn't look like a public API, so probably the simplest change we have to make it rename __REACT_WEB_VIEW_BRIDGE to something like ReactNativeBridge or REACT_NATIVE_BRIDGE and document it. Then people can use ReactNativeBridge.postMessage when they can't control the website.

We don't have to change the current code which polyfills window.postMessage, so it's backward compatible.

KoenLav commented 6 years ago

@satya164 window.postMessage isn't polyfilled, it's simply replaced.

This produces errors on various websites which make use of window.postMessage.

This breaking behavior should, in my opinion, be disabled by default and allowed to be re-enable with a flag.

Aside from that you are only free to expose a random global variable to the webview in Android, in iOS it needs to go in window.webkit.messageHandlers.

Please take a look at my PR for a suggested implementation.

fungilation commented 6 years ago

@satya164, Yes it's for injecting JS into ANY website, not just local JS. And backward compat with window.postMessage is not what any RN user should want. Let websites' JS not interfere with the RN<>WebView bridge or vice versa.

window.postMessage is for the Web and websites in the wild. Let's not touch it anymore, as far as RN's API is concerned.

satya164 commented 6 years ago

@fungilation And backward compat with window.postMessage is not what any RN user should want

I'm not talking about compatibility with window.postMessage, I'm talking about backward compatibility with previous versions. Breaking existing code which relies on this API without any warning and deprecation cycle is very irresponsible, especially when a lot of people could be using this.

@KoenLav window.postMessage isn't polyfilled, it's simply replaced

Yeah, the current implementation isn't very nice. It should've been something like this to not break websites:

window.postMessage = function(data, origin) {
  window.originalPostMessage(data, origin);

  if (!origin) {
    __REACT_WEB_VIEW_BRIDGE.postMessage(String(data));
  }
}

However changing the behavior now would be a breaking change.

I saw your PR, I think the API is good. But it's a breaking change without a deprecation cycle. I think we should add the new API from your PR, but keep the code around for few versions, maybe with some kind of warning, and remove it later.

fungilation commented 6 years ago

I thought the point of spinning WebView out of RN core into this repo, is so there can be clean improvements that break backward compat. I'm not sure you know how many times I've argued against using window.postMessage in RN core, at least renaming postMessage to something else. And I've repeatedly been ignored in the name of compatibility.

People who depends on old behavior can easily stick with RN core's WebView. Breaking changes in this repo at this stage can have warning of change, in console and in changelog, but require no deprecation cycle.

And I still fail to see the point of preserving window.postMessage for RN communication. If it's injected JS making such call, it can call whatever function the new API is to be named. Website JS wouldn't be RN-aware and would continue using window.postMessage, unchanged and undisturbed.

KoenLav commented 6 years ago

@fungilation totally agreed.

Adding a property which would reinstate the old, deprecated behavior (overwriting window.postMessage) would be nice though, I'll work on that tomorrow.

I think if/when we settle that issue we can start to think about what more the bridge should do; because IMO simply providing the developer with an API where strings can be passed from WebView to RN seems like doing half the work.

fungilation commented 6 years ago

IMO simply providing the developer with an API where strings can be passed from WebView to RN seems like doing half the work.

this.webview.evaluateJavaScript() like I mentioned, borrowing from react-native-wkwebview?

KoenLav commented 6 years ago

@fungilation I'm talking about Web => RN, you're talking about RN => Web, I think?

For me injectJavascript does the job just fine there, why would you want to add evaluateJavascript and what would be the difference?

fungilation commented 6 years ago

I'm always talking about both ways. "RN<>WebView 2-way RPC/message passing".

injectJavascript is only half of the story (a quarter?). Message passing from RN to WebView in order to get WebView to do something, or evaluateJavascript would be a direct RPC.

KoenLav commented 6 years ago

I understand, but as far as my requirements go I think everything is possible already, so I'm trying to find what you are missing.

If you want to call someFunction within the webview from RN simply injectJavascript("someFunction()").

If you want to call someNativeFunction from the webview you can use the API presented in the package I created (BridgedWebview), using ReactNativeBridge.call('someNativeFunction'), which I think should be merged into this repository.

fungilation commented 6 years ago

Ok we are not talking about the same kind of injectJavascript. I'm referring to one that's a prop that takes a static string as JS to inject, once on WebView component init, as in react-native-webview-bridge. Yours would be effectively the same as evaluateJavascript.

Titozzz commented 6 years ago

@satya164 This repository is using semver versioning, so would it be that bad to do breaking changes πŸ€” ? To me as long as you do a major release and document how to upgrade it should be fine, right ?

Titozzz commented 6 years ago

If we want a smoother transition, we can add the new interface and let the old one for ~one month. However I don't like maintaining old code for too long as it hurts more than it helps most of the time. Same reason I'll probably get rid of the UIWebview soon-ish.

KoenLav commented 6 years ago

@Titozzz totally agree. I don't think it is necessary to keep the old interface by default, but for those who aren't willing to migrate yet we could provide it in an optional form.

I've updated my PR to reflect this: when setting the overwriteWindowPostMessage prop to true, window.postMessage will still be overwritten. However it won't break existing window.postMessage functionality (as it did previously) because inside the new window.postMessage we still call window.originalPostMessage.

https://github.com/react-native-community/react-native-webview/pull/104

Please review and/or share your thoughts!

satya164 commented 6 years ago

@Titozzz You're the maintainer, so it's up to you. I always add a deprecation warning first and remove it after few releases. Because even if you follow semver, not everyone understands it and many people update their deps blindly. When someone updates to a new version, you can suddenly break their code or provide a helpful warning to migrate to the new API. React and React Native do the same when deprecating things. I don't like keeping around old code, but ultimately it provides a better UX without any actual impact on maintenance since it's going to be removed anyway, there's no reason to touch it until it's removed.

I don't think it is necessary to keep the old interface by default

It doesn't have to be default. The goals is to provide a helpful warning to migrate to new API. Adding a prop for enabling the deprecated feature is ok, but the important thing is to provide a deprecation warning telling them to use the new APIs/use the prop in the meantime. Breaking the code without a warning is not helpful even if we provide a prop. People can read the release notes, but not everyone does.

fungilation commented 6 years ago

This repo is a spinoff, experimental and brand new.

satya164 commented 6 years ago

This repo is a spinoff, experimental and brand new.

Where does it say it's experimental? My coworkers are already using it in production. Why are you so against a deprecation warning anyway?

fungilation commented 6 years ago

Why do I have to keep repeating myself as to why? Because you can go back to using RN core WebView in the meantime while it's bleeding edge, and the point of this repo is to implement the breaking changes that core wouldn't. And while it's bleeding edge, this should be about abandoning baggage and move at maximum speed.

I'm done talking to you on this point since this is getting toxic.

KoenLav commented 6 years ago

@fungilation disagreements generally lead to new insights. Everyone here has the best intentions.

@satya164 while I understand your point I totally agree with @fungilation; there should be some breaking changes to fix the mistakes which were made before.

Providing a prop to restore the old behavior, in my opinion, is sufficient.

People should understand that this repository, at this point, isn't about stability but about moving things forward in a way which could not be achieved within core.

If this means that some of them, those who (1) already moved to this repository and (2) bump major versions without thinking twice, will break their application, so be it.

This is not our responsibility at this point.

Let's quit the endless discussion (we are not going to agree, apparently) and move things forward!

jamonholmgren commented 6 years ago

Hey all, thank you for your thoughts! I really appreciate it.

I think in this case that there are very valid points on different sides of this discussion. I typically like to move quickly and remove deprecated code. However, I want it to be a good developer experience, of course.

I'll chat with @Titozzz in the meantime and see what might be the best way to handle this. I'll open up the conversation in the meantime now that it's been a couple days -- feel free to add other (friendly!) thoughts and perspectives. πŸ˜€

KoenLav commented 6 years ago

@fungilation @satya164 I think we covered the most significant points of view in this discussion.

Let's work towards implementing something?

Could you review my PR here and/or provide an alternative implementation?

https://github.com/react-native-community/react-native-webview/pull/104

fungilation commented 6 years ago

I'm not sure I'm qualified to review this as my knowledge of native iOS/Android code is quite limited. In #104, @Titozzz last commented that it cannot be merged as is, so it's his call?

My 2c is if #104 only offers 1 way call from WebView to Native, without building a foundation for calling from the other way around as well, then it's not a good path forward.

Also, I'm not sure what you meant when you said "I didn't implement this using injectedJavascript yet" before? injectedJavascript or equivalent like eval is key in getting RN and WebView to talk to each other.

KoenLav commented 6 years ago

Please take a look at the code, you'll understand what's done from the diff :)

This PR does not (yet) offer a way to call functions from WebView to ReactNative. The package I am working on here does: https://github.com/KoenLav/bridged-webview

Titozzz mentioned he doesn't feel like this should be included in this repository (you and me disagree with him, and think it should), so I left it out of the PR for now.

This PR solely focuses on un-breaking things (window.postMessage) and making sure the behavior between iOS and Android is consistent, while maintaining backwards compatibility under a flag.

In a later PR I want to work on creating a property on the WebView (injectBridge={true}) which will make the following possible, from the WebView:

ReactNativeBridge.call('nativeFunctionName', arg1, arg2, function(err, res) { console.log(err, res) });

As you can see in the BridgedWebview repository.

ps injectJavascript remains implemented in my changes, please take a look at the code!

iddan commented 6 years ago

Two questions:

satya164 commented 6 years ago

@KoenLav will review!

@iddan which API should be promise based?

iddan commented 6 years ago
ReactNativeBridge.call('nativeFunctionName', arg1, arg2, function(err, res) { console.log(err, res) });

Should actually be:

ReactNativeBridge.call('nativeFunctionName', [arg1, arg2]).then(console.log).catch(console.error);