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

Add WKWebViewIOS component #10456

Closed
wants to merge 13 commits into from

Conversation

Projects
None yet
7 participants
@cdlewis
Copy link
Contributor

commented Oct 19, 2016

This diff adds a new component WKWebViewIOS, which wraps WKWebView.

Motivation

WKWebView is Apple's recommended WebView and has several advantages over UIWebView (such as speed, due to Nitro). However it does not have feature parity with UIWebView and is missing useful features such as NSURLProtocol support. Swapping out one for the other therefore doesn't make sense. Some existing React Native users may need to stick with UIWebView whilst others may find WKWebView the more optimal choice.

History

There has already been some discussion around implementing WKWebView, see: #9878 and #321. @vjeux suggested creating separate components instead of a single generic implementation.

Some promising WKWebView implementations for React Native already exist (such as https://github.com/CRAlpha/react-native-wkwebview). I've tried to follow their structure where possible.

This pull request implements a basic version of WKWebView with an interface that mimics the existing WebView component as much as possible. My intention is to keep this request as small as is practical, incorporating additional WKWebView features over time. (I was on the fence about including onMessage support given that it does bump up the line count a bit).

Test Plan

Not sure what best practices are for testing new React Native components. I've added a new section to UIExplorer for WKWebViewIOS, which demonstrates its functionality.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 19, 2016

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

@cdlewis cdlewis changed the title Add WKWebView Add WKWebViewIOS component Oct 19, 2016

};

goForward = () => {
this.refs[WEBVIEW_REF].goForward();

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Oct 19, 2016

react/no-string-refs: Using this.refs is deprecated.

}

goBack = () => {
this.refs[WEBVIEW_REF].goBack();

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Oct 19, 2016

react/no-string-refs: Using this.refs is deprecated.

};

reload = () => {
this.refs[WEBVIEW_REF].reload();

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Oct 19, 2016

react/no-string-refs: Using this.refs is deprecated.

* Returns the native webview node.
*/
getWebViewHandle = (): any => {
return ReactNative.findNodeHandle(this.refs[RCT_WEBVIEW_REF]);

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Oct 19, 2016

react/no-string-refs: Using this.refs is deprecated.

});
}
// dismiss keyboard
this.refs[TEXT_INPUT_REF].blur();

This comment has been minimized.

Copy link
@eslint-bot

eslint-bot Oct 19, 2016

react/no-string-refs: Using this.refs is deprecated.

@jacobp100
Copy link
Contributor

left a comment

Looks really neat! I reviewed the bits I knew, so might be worth getting someone else to take a look—but seems mostly solid.


if (isJSNavigation) {
// Handle messages posted
if ([request.URL.host isEqualToString:RCTJSPostMessageHost]) {

This comment has been minimized.

Copy link
@jacobp100

jacobp100 Oct 19, 2016

Contributor

I think you handle this in didReceiveScriptMessage—is this redundant?

This comment has been minimized.

Copy link
@cdlewis

cdlewis Oct 19, 2016

Author Contributor

yup -- redundant code from WebView, I'll remove.

didFinishNavigation:(__unused WKNavigation *)navigation
{
if (_injectedJavaScript != nil) {
[webView evaluateJavaScript:_injectedJavaScript completionHandler:^(id result, NSError *error) {

This comment has been minimized.

Copy link
@jacobp100

jacobp100 Oct 19, 2016

Contributor

Is it possible to use [controller addUserScript:] here?

This comment has been minimized.

Copy link
@cdlewis

cdlewis Oct 19, 2016

Author Contributor

I don't have a good understanding of addUserScript but it sounds to me that if the controller is instantiated on initWithFrame but scripts are added here, we'd just keep adding more scripts every time navigation finished.

I played around with adding it to initWithFrame but I don't think we have access to _injectedJavascript yet.

Any ideas?

* available on the event object, `event.nativeEvent.data`. `data`
* must be a string.
*/
onMessage: PropTypes.func,

This comment has been minimized.

Copy link
@jacobp100

jacobp100 Oct 19, 2016

Contributor

I think currently the postmessage is exposed as webkit.messageHandlers...postMessage. Either we need some JS to get window.postmessage to call the previous, or update the docs to state the previous.

This comment has been minimized.

Copy link
@cdlewis

cdlewis Oct 19, 2016

Author Contributor

My preference would be to avoid injecting JS into the webview where possible so I'm going to update the docs.

This comment has been minimized.

Copy link
@jacobp100

jacobp100 Oct 19, 2016

Contributor

Just a thought on this, if we ever wanted to update from RCTWebView to this in the WebView component, it might be useful to keep the same API. But do what you think is best!

This comment has been minimized.

Copy link
@cdlewis

cdlewis Oct 19, 2016

Author Contributor

Hmm yeah that's a good point, it would be kinda annoying. Another thing to consider is that it looks like window.postMessage is already set in WKWebView but has different behaviour to what we expected in WebView (https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage).

So we'd be guaranteed to be overwriting a native function although it probably wouldn't matter because the injected code will call back to it too.

I'll take a look at doing this.

@@ -38,7 +38,6 @@ - (UIView *)view
RCT_REMAP_VIEW_PROPERTY(scrollEnabled, _webView.scrollView.scrollEnabled, BOOL)
RCT_REMAP_VIEW_PROPERTY(decelerationRate, _webView.scrollView.decelerationRate, CGFloat)
RCT_EXPORT_VIEW_PROPERTY(scalesPageToFit, BOOL)
RCT_EXPORT_VIEW_PROPERTY(messagingEnabled, BOOL)

This comment has been minimized.

Copy link
@jacobp100

jacobp100 Oct 19, 2016

Contributor

Was this intentional? We use this to know whether we should override window.postmessage.

This comment has been minimized.

Copy link
@cdlewis

cdlewis Oct 19, 2016

Author Contributor

nope not intentional ><

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 19, 2016

@cdlewis updated the pull request - view changes

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 19, 2016

@cdlewis updated the pull request - view changes

@cdlewis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

Looking at similar diffs, @mkonicek do you think you could review this from a JS perspective?

One open question remaining from @jacobp100's comments is maintaining API compatibility with WebView's onMessage. The advantage would be, well, compatibility. The disadvantage would be needing to inject JS. Dunno if you have opinions on that.

Or perhaps even just removing the onMessage stuff to simplify this PR?

});

var source = this.props.source || {};
console.log('@>> i', this.props.injectedJavaScript);

This comment has been minimized.

Copy link
@cdlewis

cdlewis Oct 20, 2016

Author Contributor

whoops -- removing this

cdlewis added some commits Oct 20, 2016

@cdlewis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2016

Or, alternatively, @spicyj do you think you'd be able to review this?

@jacobp100

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2016

LGTM.

The postMessage api compatibility could be added later if we want to merge the components at a later date, so I don't think that should block anything.

@lacker lacker self-assigned this Oct 25, 2016

@lacker

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2016

Why not make this a separate library that you can install with rnpm rather than putting it into the core React Native library? If there aren't any members of the React Native core contributors that would be maintaining this in the future, it doesn't necessarily seem like the right strategy to put this component into the core library.

@nihgwu

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2016

RN now supports iOS8+ only, I think it makes sense to add WKWebView to the core, the old UIWebView is too slow to use in production, I've been using WKWebView to replace UIWebView for quite a long time.

cdlewis added some commits Oct 25, 2016

@cdlewis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2016

I guess my reasoning for putting it in React Native is that this is a very general component that is preferable to UIWebView in most cases but we're prevented from deprecating the current implementation as we can't reach feature parity.

There seems to be broad consensus for taking this approach. See, e.g. #321.

There also seems to be precedent for community-maintained components in React Native. As someone who plans to use this component extensively I'm willing to commit to maintaining it.

@lacker

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2016

Is there a downside to making it a separate module, though? I think the previous consensus that this would make sense in the core happened before separate modules were as fully-featured as they are now. Now that react-native link works pretty well, I believe the ideal strategy is to only put modules in the core when there is a particular reason they should be in the core. @mkonicek is going to spin out some modules, for example.

An unrelated point - in terms of naming there is nothing else that we name with the iOS prefix mapped over into JavaScript. It might make more sense just to name this WebViewIOS.

@jacobp100

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2016

I think merging it into the core makes sense. Eventually, we will want to replace uiwebview, and this is just the first step.

@mkonicek

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

Hey, thanks a lot for working on this! Please release this as a module to npm - it will be super easy for anyone to use your module thanks to react-native link.

There are so many high-quality modules out there and we shouldn't merge of all them into the RN repo: https://js.coach/react-native

We're even switching to a 3rd party module as the default implementation of MapView and removing the MapView from the RN repo, check out: #10500

I think @lacker summarized it well too.

For inspiration on how to release this as a library, check out for example https://github.com/EstebanFuentealba/react-native-share and https://github.com/frostney/react-native-create-library

Thanks again for understanding and for working on this!

@mkonicek mkonicek closed this Nov 3, 2016

@mkonicek

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

RN now supports iOS8+ only, I think it makes sense to add WKWebView to the core, the old UIWebView is too slow to use in production, I've been using WKWebView to replace UIWebView for quite a long time.

Good point, if there's a lot of feedback the default WebView in React Native is not good, we should probably deprecate and remove it, same as what we're doing with MapView. And maybe the module in this PR will become the new default?

I understand having code directly in the RN repo increases discoverability but then we'd have to pull in all those other great modules that are on npm, which doesn't scale very well.

By having your own Github repo you'll be able to move faster - the code in the RN repo is synced with the internal fb codebase and you'd need to wait for someone to review changes while you're working on a module.

@cdlewis

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

Ok sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.