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

Remove errors for detecting native postMessage #10941

Closed
wants to merge 4 commits into from
Closed

Remove errors for detecting native postMessage #10941

wants to merge 4 commits into from

Conversation

jacobp100
Copy link
Contributor

A lot of people are complaining that they are getting errors when using postMessage in WebViews. This is because pages are replacing this value. There isn’t a way that we can say they want to ignore the error, but it seems that in every case, they did want to ignore the error, and did not find it useful.

This PR removes the checks, and documents window.originalPostMessage. If the user does care about this, they can inject JS into the webview to change it back (i.e. window.postMessage = window.originalPostMessage).

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @caabernathy and @spicyj to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Nov 15, 2016
@fungilation
Copy link

fungilation commented Nov 19, 2016

This is better. Though why overlap and break pages that already use window.postMessage, when RN can use a different name? Say, window.rnPostMessage. Then this issue of namespace collision is sidestepped, and original webpages that uses window.postMessage can load on page without breakage.

Otherwise, we could inject JS to run at precise the right moment (right after postMessage got moved to originalPostMessage), just to undo this RN behaviour by:

window.rnPostMessage = window.postMessage
window.postMessage = window.originalPostMessage

This would seem counter-productive at best, if not prone to breakage still as the above would need to be run at precisely the right moment after RN's replacement of window.postMessage? Original page's code would break if it calls window.postMessage that's RN's replacement instead of the real original.

@jacobp100
Copy link
Contributor Author

I'm not sure I see the benefit, since postMessage does nothing by default in web views. The implemented behaviour is almost identical to what you would see in a window opened with window.open.

We could namespace this though: native.postMessage?

@fungilation
Copy link

fungilation commented Nov 19, 2016

You sure postMessage does nothing by default? I was testing with Google
search results in WebView and the page itself seemed to be invoking the
wrong (RN's) postMessage.

I'm for namespacing just to avoid any kind of future possible headache with
namespace collision like this here. If RN devs want to use the right
postMessage, there'd be no confusion given documentation.
On Sat, Nov 19, 2016 at 10:08 AM Jacob Parker notifications@github.com
wrote:

I'm not sure I see the benefit, since postMessage does nothing by default
in web views. The implemented behaviour is almost identical to what you
would see in a window opened with window.open.

We could namespace this though: native.postMessage?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#10941 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADoJSvxmx6tKVzo0XAjTkG_Q5i3VJNGbks5q_zsLgaJpZM4KyUWf
.

@satya164
Copy link
Contributor

satya164 commented Nov 19, 2016

I'm not sure I see the benefit, since postMessage does nothing by default in web views.

postMessage can be used to communicate with iframes, so if the page in webview has a iframe, then it'll need to use the native postMessage to communicate with iframe.

Also I think it was used in some polyfill for setImmediate where the page will postMessage to itself.

How about revisiting the decision, and always require a targetOrigin?

window.postMessage = function(message, targetOrigin, transfer) {
  if (targetOrigin !== 'react') {
    window.originalPostMessage(message, targetOrigin, transfer);
  }
  bridge.postMessage(message);
}

Though why overlap and break pages that already use window.postMessage, when RN can use a different name

Many existing third party websites use postMessage for communication and implementing it means it becomes easy to use them. It's unlikely that every website will implement a RN specific API.

We could avoid breaking existing pages. It's just the current implementation.

@fungilation
Copy link

If adding targetOrigin allows differentiating iframe and RN calls of postMessage in all cases, I'm for it.

@jacobp100
Copy link
Contributor Author

I don't think allowing a targetOrigin of 'react' is spec compliant.

I'd vote to namespace it and make it behave like postMessage api in node's child_process module, which does not use the targetOrigin parameter.

@satya164
Copy link
Contributor

satya164 commented Nov 20, 2016

I don't think allowing a targetOrigin of 'react' is spec compliant.

making it work without a targetOrigin is not spec compliant either. it doesn't matter, it's a different environment and sometimes we need to extend APIs to make it work better. if you don't like 'react', then let's come up with a better one, say { target: 'bridge' } or something.

I'd vote to namespace it and make it behave like postMessage api in node's child_process module, which does not use the targetOrigin parameter.

what about the use case where I'd like to receive messages from an existing website via post message? this will add a new API with almost the same behaviour, which doesn't sound great.

@jacobp100
Copy link
Contributor Author

Just checked, the web worker spec has a postMessage method without a targetOrigin.

@satya164
Copy link
Contributor

the web worker spec has a postMessage

it's worker.postMessage though, not window.postMessage

@jacobp100
Copy link
Contributor Author

jacobp100 commented Nov 20, 2016

We could expose a worker-instace-like object—native—and it could be native.postMessage and native.addEventListener.

@mkonicek
Copy link
Contributor

mkonicek commented Nov 23, 2016

How about:

window.postMessage = function(message, targetOrigin, transfer) {
  if (targetOrigin !== 'react-native') {
    window.originalPostMessage(message, targetOrigin, transfer);
  }
  reactNativeBridge.postMessage(message);  // Clearer than 'bridge' or 'native'
}

@satya164 Would every user have to add this code to their webpage that's displayed in the WebView?

@fungilation said:

This is better. Though why overlap and break pages that already use window.postMessage, when RN can use a different name? Say, window.rnPostMessage. Then this issue of namespace collision is sidestepped, and original webpages that uses window.postMessage can load on page without breakage.

I like this idea - very simple. Just call window.rnPostMessage from the webpage.

Maybe I'm missing something here? :)

@mkonicek
Copy link
Contributor

mkonicek commented Nov 23, 2016

It's nice if you can make it spec-compliant but the spec probably didn't think about the use case of a WebView inside a React Native app so I don't mind.

We could expose a worker-instace-like object

The React Native app is not a worker though. No need to make this look like something from the spec when they're not related.

@mkonicek
Copy link
Contributor

To me it looks like this PR removes detection of an important bad behavior.

As @fungilation said:

You sure postMessage does nothing by default? I was testing with Google
search results in WebView and the page itself seemed to be invoking the
wrong (RN's) postMessage.

We should not silently ignore this.

@mkonicek
Copy link
Contributor

Based on the last comment I'll close this PR but please feel free to send a new PR for the window.reactNativePostMessage.

@mkonicek mkonicek closed this Nov 23, 2016
@jacobp100
Copy link
Contributor Author

Sorry, I meant close the other pr. I was going to finish this once we had settled on a direction.

@satya164
Copy link
Contributor

satya164 commented Dec 1, 2016

Would every user have to add this code to their webpage that's displayed in the WebView?

No. This can be injected automatically.

I like this idea - very simple. Just call window.rnPostMessage from the webpage.

The reason we initially decided to use window.postMessage is, so that existing webpages will work seamlessly on RN. For example, say you are using a third party widget, and the widget is supposed to communicate with the page via postMessage, it'll work fine in RN. We had something like this in my previous job.

I don't see why it's an issue here to introduce a non-standard origin property. It's not spec compliant, but again the spec certainly didn't consider environments like RN.

This is because pages are replacing this value.

Isn't it that we're replacing value, not pages, that's why there's the warning? I wasn't aware that the native postMessage is defined in webviews. In that case, it makes sense to not have any warning.

@satya164 satya164 reopened this Dec 1, 2016
@satya164
Copy link
Contributor

satya164 commented Dec 1, 2016

How about this,

type Message = {
  target: ?string;
  origin: string;
  data: string;
}
  • window.bridge.postMessage(message: string) - when someone wants to send a message to native from webview, target is null in this case
  • window.postMessage(message, targetOrigin, [transfer]) - when someone wants to send a message via native postMessage, the target property received by native is set to the targetOrigin specified.

The onMessage prop receives messages from both of these and the target property can be used to distinguish between messages if needed. The intent is also explicit when you want to send something to specifically native.

@jacobp100
Copy link
Contributor Author

We’ll need to put the origin parameter on message events too, so the user can filter events. Maybe just react-native or something?

@fungilation
Copy link

fungilation commented Dec 1, 2016

As I originally objected to this charade of renaming/calling window.originalPostMessage, is issue of timing. If we are keeping window.postMessage for RN use with an automatically injected RN version, it has to be injected after the webpage's own window.postMessage is loaded to ensure webpage's own will be permanently replaced.

If that is ensured, I have no objection. Otherwise, I just feel it's error prone. Especially if a webpage decides to override window.postMessage at an undetermined time later in an unpredictable way for whatever reason. Letting existing web JS code to call window.postMessage when placed inside RN is a rather edge case for this trouble. If people want to run existing code calling window.postMessage inside RN, a global search for window.postMessage and replace with window.rnPostMessage (or whatever for RN use) only takes a few seconds so why bother with this cross compatibility business?

If webpage's original window.postMessage isn't broken, don't fix it? (or rather, don't tamper with it)

@fungilation
Copy link

+1 to window.rnBridge.postMessage naming. Cleaner than my suggested rnPostMessage.

@axten
Copy link

axten commented May 17, 2017

postMessage is broken for nearly every usecase, see #10865
why not simply fixing this bug?

@fungilation
Copy link

fungilation commented May 17, 2017 via email

@davidhellsing
Copy link

I gave up on any updates on this and made a post-yarn shell script that patches native code:

#10865 (comment)

Works great so far!

@fungilation
Copy link

While I'm "stuck" with https://github.com/alinz/react-native-webview-bridge (patched for RN 0.40+), still.

@twairball
Copy link

twairball commented Jul 5, 2017

WKWebview in iOS namespaces their postMessage. Coming from native, I would've expected Webview to do JS bridging similarly. Current implementation overloads browser API for window.postMessage(). I think it is incorrect for Webview to alter browser API.

For example, this breaks websites that use meteor.js which also use window.postMessage (see #12997 with PR for a rename).

From practical standpoint, the use-case here for RN apps is to communicate between webview and RN, in which case passing messages via existing browser api for window.postMessage has very little gain.

I hope RN devs can consider to add a new API e.g. postMessageNative ASAP! and consider to add debug warnings for postMessage if it needs deprecation

References:

http://www.joshuakehn.com/2014/10/29/using-javascript-with-wkwebview-in-ios-8.html
http://nshipster.com/wkwebkit/
https://developer.apple.com/documentation/webkit/wkscriptmessage

@shergin
Copy link
Contributor

shergin commented Aug 17, 2017

I think we have to reconsider this.

@shergin shergin reopened this Aug 17, 2017
@fungilation
Copy link

fungilation commented Aug 17, 2017

@shergin, quite an understatement! 😂 But thank you.

I want to repeat that this in its current form in RN is unusable (to me), and I'm resorting to use a mostly unmaintained and monkey patched library like https://github.com/alinz/react-native-webview-bridge for 2 way communication/RPC between RN and webview.

@shergin
Copy link
Contributor

shergin commented Aug 18, 2017

Mmm... Interesting. How does that library work?

I am starting believe that what we have to do it:

So, this particular solution probably bit obsolete. Oops.

@shergin shergin closed this Aug 18, 2017
@fungilation
Copy link

Agreed that pulling something like https://github.com/CRAlpha/react-native-wkwebview into RN core would be killing 2 birds/bugs with 1 stone.

@twairball
Copy link

twairball commented Aug 18, 2017

react-native-wkwebview doesn't address Android.

@fungilation
Copy link

New WebView can use WKWebView on iOS side only obviously. But something like "Call originalPostMessage as part of overridden postMessage." should apply also on Android side.

@shergin
Copy link
Contributor

shergin commented Aug 18, 2017

So, my vision of this problem is:

  • We have to call originalPostMessage as part of overridden postMessage;
  • Using window.location to transfer events to native side makes calling originalPostMessage impossible (probably because it stops all JS execution);
  • UIWebView does not have another way to transfer data to native side, but WKWebView does have special messaging subsystem for this;
  • So, switching to WKWebView finally will allow us to fix postMessage problem (at least on iOS).

I don't know much about Android, unfortunately, but I hope:

Anyways, using WKWebView instead of UIWebView is a right movement which we have to do.

I have not investigated this probmem deeply enough though, so I can be completely wrong. 😄

@fungilation
Copy link

I don't know about the android side as I'm working solely with iOS so far with my RN app. But https://github.com/alinz/react-native-webview-bridge have long estabalished 2 way RN <> webview communication on both iOS (UIWebView) and Android so ideally in terms of expertise:

  1. Get contributors/merge from https://github.com/alinz/react-native-webview-bridge to help with RN's new WebView, on the android side
  2. Similarly, contributors/merge from https://github.com/CRAlpha/react-native-wkwebview on new WebView's iOS side

@farazs
Copy link

farazs commented Aug 19, 2017

The current webview on Android is fine, you can use window.__REACT_WEB_VIEW_BRIDGE.postMessage() instead and it works perfectly. This however is dependent on the current implementation. We should just document this behavior (maybe with slightly different naming) and use that instead of the current window.postMessage() so that there's no delay before it's initialized like there is currently.

@fungilation
Copy link

window.__REACT_WEB_VIEW_BRIDGE.postMessage() could be the naming on iOS side as well. As long as it's separated into its own namespace to (mostly) avoid collision with what websites in the wild may call, and iOS/Android calls for RNpostMessage() should have consistent naming

@shergin
Copy link
Contributor

shergin commented Aug 19, 2017

WKWebView has dedicated and very simple window.webkit.messageHandlers.{NAME}.postMessage().
http://nshipster.com/wkwebkit/

@twairball
Copy link

window.__REACT_WEB_VIEW_BRIDGE.postMessage() doesn't work for iOS.

Comments from users on Issue #11594 also mention that this method is not available for Android release, only for debug.

@farazs
Copy link

farazs commented Aug 25, 2017

@twairball that is apparently caused by proguard. I am using it in release just fine.

@samermurad
Copy link

TL;DR
Here is the quick fix for react native 0.44.2

Fixing the Problem should be a lot easier.
After Inspecting the iOS RCTWebView.m code.
The error Setting onMessage on a WebView overrides existing values of window.postMessage, but a previous value was defined is raised in dev only, after evaluating the following JS code:
String(window.postMessage) === String(Object.hasOwnProperty).replace('hasOwnProperty', 'postMessage')
if this returns false, the error is shown.

Printing String(Object.hasOwnProperty) in a browser console prints out the following:
function hasOwnProperty() { [native code] }

React Native on iOS tries to check that basically no one has touched the window.postMessage function.
Two lines afterwards, the code alters the window.postMessage function
(the alteration code depends on the react native version) to the following: window.postMessage = function(data) { messageQueue.push(String(data)); processQueue(); }; // react native 0.49.3

This piece of code runs in the UIWebView's - (void)webViewDidFinishLoad:(UIWebView *)webView delegate method.
Each time the UIWebView detects a some sort of a navigation, it calls it's delegate method
- (BOOL)webView:(UIWebView *)webView shouldStartLoadWithRequest:(NSURLRequest *)request navigationType:(UIWebViewNavigationType)navigationType
which will continue to execute the request if true/YES is returned.
upon successful request the method - (void)webViewDidFinishLoad:(UIWebView *)webView will be called.

This is okay when you actually navigate to a new website, but is not the case when loading an iframe for example, because every iframe redirection will invoke this process again, meaning that the above code will run again creating this issue.

The quick fix in this case, is to check if window.originalPostMessage != undefined, if the method is already defined, there is not reason for it to reassign window.originalPostMessage = window.postMessage;

This is how I solved it locally, it may not be the best solution, but It doesn't break the current logic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.