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.

Sign up for GitHub

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

Jump to bottom

Upgrade WebView #3296

Merged
merged 4 commits into from
Mar 27, 2019
Merged

Upgrade WebView #3296

merged 4 commits into from
Mar 27, 2019

Conversation

borisyankov
Copy link
Contributor

The WebView component is being moved out of react-native.
While it is still available from the main package, Its development has stopped and it is being deprecated (and soon to be removed completely)

The very same component is under active development and eagerly accepting PRs at react-native-webview.

This PR migrates to that package (and as separate commits does few more changes).

@gnprice gnprice mentioned this pull request Jan 24, 2019
Closed
@borisyankov borisyankov mentioned this pull request Mar 20, 2019
Closed
@borisyankov
Copy link
Contributor Author

@gnprice it turns out this is a blocker for upgrading to RN 0.59 (#3399)
So I am putting it at P1 too.

@gnprice
Copy link
Member

gnprice commented Mar 20, 2019

@borisyankov Thanks for this PR! And for the ping <_<

48132ac44 Add 'react-native-webview' dependency

  • The commit message says we want 2.0, but the yarn.lock shows we actually get 2.15.0. Is that OK? Should probably be acknowledged -- e.g. say "Version 2.x" instead, if that's what you want.

    Small Git tip: when there are yarn.lock changes and I actually want to see them, I add -a to my git log command. Or add to .git/info/attributes a line like yarn.lock diff, overriding our .gitattributes file.

ebbfa016f Link 'react-native-webview' library

Looks good, thanks!

And thanks in particular for the command saying what you did, since the iOS/Xcode part of this is hard to read.

5f492bdf0 Use WebView from 'react-native-webview' instead of 'react-native'

  • It'd be really good to have at least a brief explanation of what this means. What is this react-native-webview library? Why would we want to use it instead?

    I think the answer is that this is where FB spun off the upstream WebView component, taking it out of RN core. But that is just a guess from vaguely recalling a mention in some blog posts that that was going to happen. 🙂 A sentence or two saying that, and a link to a relevant announcement, would be super helpful.

    (Or if the story is something else, then an explanation of that would be extra helpful!)

a04ad0e19 webview: Replace UIWebView with WKWebView

Cool. Thanks for the very informative commit message!

7c3864062 WIP: XCode chages. Are these needed?

Huh indeed. Are they needed?

(What are they intended to do? Or, how did they get made?)

@gnprice
Copy link
Member

gnprice commented Mar 20, 2019

I think the answer is that this is where FB spun off the upstream WebView component, taking it out of RN core. But that is just a guess from vaguely recalling a mention in some blog posts that that was going to happen. 🙂 A sentence or two saying that, and a link to a relevant announcement, would be super helpful.

Ah, I see the PR description has more or less this information. Please put that in the commit message too. 😉

@borisyankov borisyankov force-pushed the react-native-webview branch 2 times, most recently from 3ce4c47 to d4b1c20 Compare March 21, 2019 14:29
@borisyankov borisyankov changed the title Upgrade WebView WIP: Upgrade WebView Mar 22, 2019
@borisyankov
Copy link
Contributor Author

Note: I renamed the PR to WIP as there are a few more changes that need to be done.
I am also examining all the changes made to the component in the last few months.
Both exciting and concerning changes. (exciting - caching, concerning - postMessage rework)
We just need to be aware of the made and planned changes.

@gnprice gnprice mentioned this pull request Mar 23, 2019
Merged
@gnprice
Copy link
Member

gnprice commented Mar 23, 2019

Note: I renamed the PR to WIP as there are a few more changes that need to be done.
I am also examining all the changes made to the component in the last few months.
Both exciting and concerning changes. (exciting - caching, concerning - postMessage rework)
We just need to be aware of the made and planned changes.

Sounds like a good plan -- thanks!

@borisyankov borisyankov force-pushed the react-native-webview branch 5 times, most recently from d95b13a to a91104b Compare March 25, 2019 14:29
@borisyankov borisyankov changed the title WIP: Upgrade WebView Upgrade WebView Mar 25, 2019
@borisyankov borisyankov force-pushed the react-native-webview branch from a91104b to 53d1e34 Compare March 25, 2019 20:50
@gnprice
Copy link
Member

gnprice commented Mar 26, 2019

I see you've taken the "WIP" back out of the title -- is the latest version of the branch intended for review and merge?

@borisyankov
Copy link
Contributor Author

Yeah. Ready for a review.
I did merge the commit 'XCode: not sure if needed' into 'Link 'react-native-webview' library's native dependencies' as it was definitely needed :)

@borisyankov @gnprice
webview: Add 'react-native-webview' dependency.
3447f5f 
The `WebView` component is being removed out of RN Core
as part of the "Lean Core" project:
  facebook/react-native#23313

Its development continues here:
  https://github.com/react-native-community/react-native-webview

Just add the dependency to `package.json` and `yarn.lock`.

Version 2.0 is the exact same version that is in `react-native`.
While there are newer versions that have exciting features, the
first step is to simply start using this new package.
@borisyankov @gnprice
webview: Link 'react-native-webview' library's native dependencies.
5c3b35a 
This is the result of running:
`react-native link react-native-webview`

Then manually reviewing and cleaning up any accidental and
unrelated changes.
@borisyankov @gnprice
webview: Use 'react-native-webview' instead of RN core's WebView.
32b6bc4 
The WebView component has moved out of RN core into its own library;
see the grandparent commit.

Now, import the WebView library from the new package.
No functional change introduced, because we're using the version
that corresponds to what's in RN core.
@borisyankov @gnprice
webview: Replace UIWebView with WKWebView, on iOS.
3ad1269 
React Native 0.57+ supports the new `WKWebView` iOS component in
`WebView` instead of the legacy `UIWebView`.

The UIWebView component is deprecated since iOS 12:
  https://developer.apple.com/documentation/uikit/uiwebview

WKWebView is supported on iOS 8 and newer so that is not a concern.
  https://developer.apple.com/documentation/webkit/wkwebview

An article on the `WKWebView` support in React Native here:
  http://facebook.github.io/react-native/blog/2018/08/27/wkwebview

The change code-wise is trivial, I have tested it on an emulator
and as expected everything seems to be working correctly.

We should do an extended TestFlight testing just in case.
To reiterate, this is a iOS-specific change.
@gnprice gnprice force-pushed the react-native-webview branch from 53d1e34 to 3ad1269 Compare March 27, 2019 20:23
@gnprice
Copy link
Member

gnprice commented Mar 27, 2019

Thanks @borisyankov ! Looks great -- merged.

@gnprice gnprice merged commit 3ad1269 into zulip:master Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
P1 high-priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@borisyankov @gnprice