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

Fix iOS imports on RN0.40+ and replace UIWebview with WkWebview #217

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

guilhermebruzzi
Copy link

@guilhermebruzzi guilhermebruzzi commented Mar 7, 2017

No more deprecated webview on iOS and let people use webviewbridge API with RN 0.40+
I also included a new example SampleRN42 that shows that the main features still work.

@fungilation
Copy link
Contributor

Wow, moving to WKWebview is a big jump. Which I wanted.

Are you using it in your own app and tested? Does it maintain same API as before, more or less?

@guilhermebruzzi
Copy link
Author

@fungilation
Are you using it in your own app and tested? <- Yes
Does it maintain same API as before, more or less? <- Yes, only scalesPageToFit is not supported by wkwebview (all other props we used worked well)

@fungilation
Copy link
Contributor

I commented inline in code:

Any reason why this isn't changed to #import <React/UIView+React.h>?

Same in ios/RCTWebViewBridge.m

@guilhermebruzzi
Copy link
Author

@fungilation No reason, sent a fix, TY! :)

@fungilation
Copy link
Contributor

fungilation commented Mar 9, 2017

Switched to your PR in my app, no bug so far that I can see. With noticeable improvement in WebViews' loading performance, much thanks for this!

Copy link
Contributor

@fungilation fungilation left a comment

Choose a reason for hiding this comment

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

Tested and working in my app, with noticeable improvement in initial loading performance due to WKWebView.

@guilhermebruzzi
Copy link
Author

@fungilation Nice! :D

@peterept
Copy link

The injectedJavaScript property no longer works.

Do you have a good idea how to add that back in for WkWebView ?

@fungilation
Copy link
Contributor

fungilation commented Mar 25, 2017

I confirm that the WKWebView fork never worked. I mistakenly had the old version in my test app. On linking the new fork, app just crash to home screen on WebView's render.

I am using and need the injectedJavaScript prop.

@peterept
Copy link

@fungilation it mostly works for me with a few tweaks (which were ok for my situation):

  • I moved my code from injectedJavaScript and embedded into my document
  • WebViewBridge was not defined on initial document load, so I wait for it with a handler:
<script>
 (function () {
    function onWebViewBridgeReady() {
      document.removeEventListener('WebViewBridge', onWebViewBridgeReady, false);
      WebViewBridge.onMessage = function (message) {
          // assume message has a color 
          document.body.style.backgroundColor = message;
        }
     }
      WebViewBridge.send("webviewbridge-ready");
    }
   document.addEventListener('WebViewBridge', onWebViewBridgeReady, false)
  }())
</script>

My document works with that and I can send/receive data over the bridge.

No more deprecated webview on iOS and let people use webviewbridge API with RN 0.40+

Commit messages:

Fix iOS imports on RN 0.40+

Replacing UIWebview with WkWebview

Fix WKWebview Bridge

Remove debug informations

Fix injectedJavaScript property
@guilhermebruzzi
Copy link
Author

guilhermebruzzi commented Mar 25, 2017

@fungilation @peterept I fixed now this injectedJavaScript property and created a new example: examples/SampleRN42/ that you can use to test

@guilhermebruzzi
Copy link
Author

@fungilation @peterept Anyway I never used injectedJavaScript in my projects.
I always embedded in the document like @peterept said, because is way faster and should be your solution if you have control over the document

@fungilation
Copy link
Contributor

For me I need to inject JS, as I'm loading external websites. Thanks @guilhermebruzzi

@peterept
Copy link

Right. The code needs to use -[WKWebView evaluateJavaScript:completionHandler:] to do the inject, but I'm not sure where react-native-webview-bridge needs to do the injection exactly.

@guilhermebruzzi
Copy link
Author

@peterept I called the method on the same places that UIWebview used to call his inject method.
Even If you call It earlier, the webview (on Android too) only allows the code to run after it has finished loading (If i'm not mistaken)

@peterept
Copy link

holy moley @guilhermebruzzi . I'm a fool... I cloned from your master branch on your fork (which has an older version of your code with WkWebView - missing the injection code). I'll switch to your branch feature/wkwebview and report back!

@peterept
Copy link

Confirmed it works! That's awesome work @guilhermebruzzi.

I installed from the correct branch (although I see you've updated master now also):

$ npm install --save git://github.com/vtex/react-native-webview-bridge.git#feature/wkwebview
$ react-native run-ios

And now the injectedJavaScript={injectScript} works and also the WebViewBridge variable is set already (as in the sample code) and so no need to wait for it:

const injectScript = `
(function () {
  if (WebViewBridge) {
    document.body.style.backgroundColor = "red";
  }
}());
`;

Thank you so much for this.

@guilhermebruzzi
Copy link
Author

@peterept no problem! :) Yeah, you can install from master branch also.

@iOSHw
Copy link

iOSHw commented May 9, 2017

@guilhermebruzzi how to use this pull request ? the master branch have problem!

@guilhermebruzzi
Copy link
Author

@iOSHw What kind of problem?

To use the master branch you insert:

"react-native-webview-bridge": "vtex/react-native-webview-bridge",

on package.json dependencies

And than run npm install and react-native link

@UnsignedInt8
Copy link

@guilhermebruzzi Hi, bro. I really appreciate your hard working, but it seems not working under RN 0.44.2. Can you fix it?
screen shot 2017-05-28 at 16 15 48

@jenskuhrjorgensen
Copy link

Awesome!
I couldn't get this library to work ("Cannot read property 'NavigationType' of undefined") until I installed this PR - THANKS! :D

@jenskuhrjorgensen
Copy link

@guilhermebruzzi @alinz is anybody working on fixing the conflicts and merging this branch into master?

@alinz
Copy link
Owner

alinz commented Sep 27, 2017

@jenskuhrjorgensen I'm extremely busy that's why I'm constantly looking for good maintainer? Are you interested?

@jenskuhrjorgensen
Copy link

@alinz That's a shame - your library has been very useful to me! :) I'm afraid I don't have the time either.

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.

7 participants