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

Proposal 0001 - Extract WebView From Core #3

Merged
merged 4 commits into from Sep 10, 2018

Conversation

Projects
None yet
@jamonholmgren
Copy link
Collaborator

commented Jul 31, 2018

This proposes that we extract the current WebView implementation from React Native Core and move it into a third party package.


## Motivation

The current WebView implementation is (to my knowledge) not used by Facebook internally. This leads (understandably) to a lack of first-class support for this core API. By moving the component to a third party that is more invested in supporting it, there will be better support.

This comment has been minimized.

Copy link
@axe-fb

axe-fb Jul 31, 2018

Collaborator

Actually, we do use WebView internally, but have not been maintaining it as much. However, I still believe that it makes sense if the community maintains it, since the number of PRs/Issues in the community are possibly higher.


For the first release, the goal is to provide the same API, only replacing the import statement and otherwise backwards compatible.

We will also want to update generated project boilerplates, such as create-react-native-app and Ignite, to use the new package.

This comment has been minimized.

Copy link
@axe-fb

axe-fb Jul 31, 2018

Collaborator

Should we do it in the first add more features/bigs and then look at a replacement ?

This comment has been minimized.

Copy link
@jamonholmgren

jamonholmgren Jul 31, 2018

Author Collaborator

I'm definitely open to that. I mainly wanted to keep the surface area of the change smaller for those who have to migrate their existing apps.


## Unresolved questions

None so far.

This comment has been minimized.

Copy link
@axe-fb

axe-fb Jul 31, 2018

Collaborator

I think we also need to talk about how we move existing bugs and PRs over to the new location ?

This comment has been minimized.

Copy link
@jamonholmgren

jamonholmgren Jul 31, 2018

Author Collaborator

Right, I'll add that.

This would move it to an external package, like so:

```jsx
import { WebView } from '@infinitered/react-native-webview';

This comment has been minimized.

Copy link
@axe-fb

axe-fb Jul 31, 2018

Collaborator

What should be the right source ? Should this be in infinitered, or should it be in react-native-community ?

This comment has been minimized.

Copy link
@jamonholmgren

jamonholmgren Jul 31, 2018

Author Collaborator

I'm open to react-native-community, but am more motivated to devote my company resources toward it if it lives in infinitered.

This comment has been minimized.

Copy link
@brentvatne

brentvatne Aug 2, 2018

packages get handed off as companies/individuals either lose interest in a tool, close their doors, no longer want to maintain the package itself, the primary maintainer moves to another organization, or a handful of other reasons. this has downstream consequences - for example, if react-native-maps was scoped under @airbnb on npm, we'd be stuck with that or have to force consumers to change their imports and package.json just because of a change in project maintainer structure.

I understand your concern about wanting to get some return for the time you invest, but I think there are better ways we can do this than brand the package name itself.

This comment has been minimized.

Copy link
@jamonholmgren

jamonholmgren Aug 7, 2018

Author Collaborator

@brentvatne Thanks for the feedback. I've removed the namespace and went with just react-native-webview.

@kelset

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

To give some extra context:

  • link to the open issues related to WebView in the main repo (yes it's also linked in the .md but for those scrolling through may be easier to have it here)
  • the discussion issue of moving to WKWebWview instead of UIWebView
@LinusU

This comment has been minimized.

Copy link

commented Jul 31, 2018

I'm personally not a fan of moving WebView out of core, even though I feel the frustration of submitting PRs that neither gets comments nor gets merged.

My reasoning builds on two things:

1) There are a lot of small useful modules which uses WebView behind the scenes for things to work. Some quick examples are react-native-qrcode, react-native-webview-crypto, react-native-editor, etc...

If we move WebView out of core, these will no longer "just work" after an npm install. Simply declaring a dependency on the WebView-module won't work either, since it will require manual setup to work.

Having dependencies on modules that requires manual setup provides a very poor user experience for your users. You basically have to duplicate the other library's setup instructions in your own readme, and hope that the users are paying attention.

Until React Native has a way to import native dependencies which require zero input from the user (kind of like how prebuilt native modules works in Node.js) I don't think it's a good idea to move this out.

and also 2) there is nothing stopping anyone from developing a user-land WebView. Instead of making a hard switch, I think that a better approach is to let one of the user-land implementations prove themselves first, and then we can first deprecate, and then remove, the built-in WebView.

Just my 2¢ ☺️

@eliperkins

This comment has been minimized.

Copy link

commented Jul 31, 2018

What's preventing the community from creating a package that does this on it's own first? It feels like we should be providing the implementation first, then deprecating WebView and recommending moving to the new package, before we set out down this road.

there is nothing stopping anyone from developing a user-land WebView. Instead of making a hard switch, I think that a better approach is to let one of the user-land implementations prove themselves first, and then we can first deprecate, and then remove, the built-in WebView.

I should read other comments first. I'm onboard with @LinusU here!

@jamonholmgren

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 31, 2018

There have been a few user-land WebView implementations, and we've used them at Infinite Red (they're not bad).

As long as there's an "official" one, though, it'll get the benefit of being the default (like how Apple Maps has higher usage on iPhones than Google Maps, despite the latter being generally better).

I'm open to this discussion, for sure, but just want to point out that so far, none of the other webviews have attracted enough attention to become the overwhelming favorite.

@jordoh

This comment has been minimized.

Copy link

commented Jul 31, 2018

let one of the user-land implementations prove themselves

There are a number of user-land WebView implementations out there. On iOS, react-native-wkwebview has a clear mission statement (UIWebView deprecation) that makes it an obvious alternative. The landscape isn't so clear on Android, where there's: react-native-webview-android, react-native-webview-file-upload-android, react-native-advanced-android-webview, react-native-custom-android-webview - and likely more I'm omitting - each of which describe a very specific feature (e.g. file upload) as their goal.

Perhaps the Android landscape shows that there aren't significant issues preventing use of the built-in WebView for most purposes, but it's clear that a user-land implementation hasn't been able to prove itself on Android.

The combination of a clear - and well established - alternative on iOS and the lack of one on Android makes it very unlikely that a user-land cross-platform alternative will prove itself enough to replace the built-in version.

Until React Native has a way to import native dependencies which require zero input from the user (kind of like how prebuilt native modules works in Node.js) I don't think it's a good idea to move this out.

Does react-native link ... get close enough to that?

@axe-fb

This comment has been minimized.

Copy link
Collaborator

commented Jul 31, 2018

I think the goal here is to enable a WebView that is maintained and supported well. I am agnostic to picking any existing community packages, or creating a new one, as long it is well maintained and owned in the community.

I think that we have not been able to give web views enough attention, and thats the reason I would like use to move this out.

Regarding other modules depending on WebView - I think we don't have a story for React-native modules depending on each other. Folks from Artsy were working on a proposal for better react-native link, with cocoapods, but if there are other proposals that the community has, lets add them to this repo.

@grabbou

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

The link itself should work just fine. I am actually working on a new proposal (as a part of this repository) to kick-start the discussion on the new format and dependency management.

Right now, if the module depends solely on its own files and contains no extraneous native dependencies, things work just fine. We also support the use of Cocoapods. On Android, this is not an issue since Gradle is a preferred build tool.

The need of linking the module itself shouldn't be an issue since I guess everyone got used to this process over time already.

@erictraut

This comment has been minimized.

Copy link

commented Jul 31, 2018

I think you are underestimating the impact this proposal would have on existing source bases. Many apps, RN extensions, and shared components make use of RN.WebView, and this change will break all of them. I strongly urge you to reconsider this.

@satya164

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

It's not going to happen without a deprecation path. And most the current WebView can be published as a separate legacy package so folks can switch to it with minimal changes when it's removed from the core.

@erictraut

This comment has been minimized.

Copy link

commented Aug 1, 2018

Deprecation of core components and features results in varying degrees of pain for those who have built code on top of the existing RN core. Breaking changes like this shouldn't be done cavalierly, even if there is a well-articulated deprecation path. This one would be quite painful for developers who have built apps using ReactXP.

I appreciate that there are times when breaking changes are needed to move the platform forward, but this doesn't seem like one of them. The reasons provided above don't appear to justify the pain. Why not continue to support RN.WebView and switch over to WKWebView on iOS? Are there any good technical reasons?

@satya164

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

Why not continue to support

That's exactly the problem. The core team doesn't support the WebView components and has other high priority tasks, so it's not going to change very soon. There are several bugs and missing features in the WebView component which have been there for a long time. Moving it to out of core means it can get more attention from the community. Would you really prefer an unmaintained module in the core than having a better-maintained version as a third-party package?

Regarding how much pain it causes, it should be minimal if we just publish it as a standalone package and will only require changing some import statements. I don't think this one is as massive as many previous breaking changes.

@axe-fb

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2018

Note that This should NOT be a breaking change. The way I am envisioning this happening is that react-native init would simply refer to the other module eventually. We are also working on a greater "slimmening" proposal (which I propose to write up here), so that we move many of the modules outside core.

The hope is that we reduce React Native's surface area and merge issues and PRs faster.

@gwmccull

This comment has been minimized.

Copy link

commented Aug 1, 2018

@axe-fb if it's still referenced by react-native init will that help with bundle sizes at all? It seems like one of the benefits of moving WebView outside the main repo would be to remove that code from bundles that don't use it

@kelset

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

Created a generic "overview" of the Slimmening discussion to create a context for this PR and avoid to go too "outside" the purpose of this PR #6

@axe-fb

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2018

@gwmccull - The goal of "the slimmening" is not to reduce bundle size, but to make components more maintainable. We could start moving things out of the bundle, but thats a different discussion

@satya164

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

Don't think react-native init referencing to the module can be considered a non-breaking change. If you're starting an app from scratch, there is nothing to break anyway. But it will be a breaking change for existing apps which are upgrading to a new version of react-native.

@jamonholmgren

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2018

it will be a breaking change for existing apps which are upgrading to a new version of react-native

@satya164 is correct here, this will be a breaking change for existing apps that use WebView.

In another life, I built a RubyMotion library that had an integrated maps and webview. I eventually moved maps into its own repo, but kept webview as a part of core because 1) it wasn't very big (same here), and 2) many people depended on it (same here).

However, the reality with React Native core is that the WebView as it currently stands is not sufficiently maintained, unlike my library's WebView.* So the overall goal is to reach a place where we have a WebView that is well maintained and reliable. And with the upcoming deprecation of UIWebView in iOS 12, the need has become more urgent.

I think you are underestimating the impact this proposal would have on existing source bases. Many apps, RN extensions, and shared components make use of RN.WebView, and this change will break all of them. I strongly urge you to reconsider this.

I hear you @erictraut -- I'd like to see some examples so we can examine what the migration path looks like to those apps, extensions, and shared components. If it's as simple as adding a new import statement to the external legacy package, is that sufficient?

* I don't blame Facebook for this at all -- they are providing an amazing community service by open sourcing React Native in the first place, and should be expected to focus on the things that mostly serve them. Just as Infinite Red focuses on the things, like Ignite, that we need.

@fossecode

This comment has been minimized.

Copy link

commented Aug 1, 2018

Please make the extracted component compatible with Expo 🙏

@grabbou

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

I've been working on many apps recently that involve heavy-loaded interactions encapsulated inside a WebView and in most cases, I ended up using alternative packages provided by the community from superior performance.

I understand and agree with others that this is definitely going to be a breaking change, but at the same time, it's going to improve collaboration between different WebView projects out there, resulting in better performance for everyone.

Once the package is extracted and fully tested to be working with react-native link and compatible with Expo at the same time, I think we are good to go.

@axe-fb

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2018

I would like to propose that we move this proposal forward.

Action items

  • manually import existing PRs and issues. Once PRs are merged and issues addressed, close them on react-native's main repo (@jamonholmgren)
  • Once all PRs are merge, folks from expo should make a call on whether this is included instead of the stock react-native-webview (expo)
  • Once PRs are merged and issues are fixed, we will change react-native's documentation to point people to the fork. We will also start moving issues/PRs to the forked repo, and add a note in the WebView directory about this new location.
  • We will visit this issue again in 2 months. If the forked repo continues to be healthy, we will then device if we remove webview in the main repo and make react-native-link automatically add the forked webview.

For backward compat, people can continue to use the webview in the main repo. However, note that we will ONLY FIX critical issues.

If this proposal sounds ok, please vote with a thumbs up. If you still think this proposal has issues, please comment on this thread with the reasons

@anp

This comment has been minimized.

Copy link

commented Aug 2, 2018

@ide heads up that we should start evaluating what we're going to do with WebView.

@ide

This comment has been minimized.

Copy link
Member

commented Aug 2, 2018

For Expo: At minimum, for a smooth migration path and to help developers get to a good place, we'd work hard to maintain compatibility with the existing ReactNative.WebView API in Expo (including the import syntax, with perhaps a deprecation warning to give people advance notice about changing their import syntax).

On naming: I think it sets a better precedent if company-scoped modules aren't under the react-native-community GitHub org unless the module is specifically for integration with that company's services (e.g. @google/react-native-google-analytics).

(With regard to Expo, if this component were published under the @infinitered scope, we'd probably fork and republish the package under something more communal or Expo-related to more clearly communicate that Expo isn't a kitchen sink of modules written by various companies and not to set an expectation or precedent that we'd arbitrarily include @someotherco/module.)

Ideal outcome: From my experiences working at/on Facebook & React Native & Expo one thing I keep coming back to is that the more we can focus the React Native repo on the bridge and set clear expectations that Facebook is responsible primarily for the actual core of React Native and not the very long tail of modules, the healthier the React Native repo will become and the greater the chance React Native has at succeeding. @axe-fb has mentioned maintenance cost several times in this discussion and, in my experience, the tradeoffs are much more natural to understand by working at companies like Facebook. In that light, I agree with @axe-fb's proposal and think we should add a fifth step to even more clearly set expectations for the maintenance and responsibility of the React Native repo by eventually pointing developers towards this repo in a doc or react-native init and providing them with instructions on how to install WebView and other modules.

@fungilation fungilation referenced this pull request Aug 21, 2018

Closed

0.57.0 Discussion #34

@fungilation

This comment has been minimized.

Copy link

commented Aug 21, 2018

Finally, WKWebView!

I'm stuck using abandoned https://github.com/alinz/react-native-webview-bridge due to this: facebook/react-native#9762 (comment). Would be great to allow setting window.postMessage to override any existing one in an arbitrary webpage, or avoid name collision by moving RN's injection to something like window.rnPostMessage. Had a long discussion before on #9762 and don't remember where else on this and it didn't go anywhere.

Alongside or after the work on adding WKWebView, which as I understand it has new APIs which makes implementing this easier? I'm willing to investigate and implement if lack of interest is the issue with having a functional, non-colliding window.postMessage. I just need direction on how to approach getting this into RN, if it's wanted. As I need this in my app, and I want to move off unmaintained react-native-webview-bridge.

PS. If WebView in RN is spinning out of core, I'm for it going under the react-native-community org. An authoritative but neutral third party for a component as central and widely used as WebView, even if it's just a naming issue. RN consultancies/agencies can be credited with badges and what not for contributions, so all improvements can go towards this common ground. The biggest issue against OSS is fragmentation, and as mentioned above with the various third party WebViews on Android going after different small features, that's not a healthy thing. Developers (me) don't want a dozen mutually incompatible WebView forks, I just want the best one.

@axe-fb

This comment has been minimized.

Copy link
Collaborator

commented Aug 28, 2018

@jamonholmgren - What are the next steps on this ? Would you still want to lead this effort ?

@axe-fb

This comment has been minimized.

Copy link
Collaborator

commented Aug 28, 2018

@fungilation - regarding postMessage - we have not made the change since it was a breaking API. However, now that we are spinning it out of core, breaking changes are fair game. Once this is moved out into the community, we can continue the discussion on the repo.

@kelset

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

BTW, there is a blogpost now on the RN website about the new WebView -> http://facebook.github.io/react-native/blog/2018/08/27/wkwebview

@fungilation

This comment has been minimized.

Copy link

commented Aug 28, 2018

Thanks @axe-fb. Agreed with order of things. Glad to hear that spinning WebView out of core means more breaking change but necessary improvement like postMessage can happen. Especially since RN<>WebView message passing/RPC can now happen natively without hacks with WKWebView.

@jamonholmgren

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 8, 2018

Apologies for being out of contact on this. I've been way more busy with work than I expected. Big kudos to the Facebook team for landing this work!

@axe-fb I'm still interested in working on spinning this out of core, depending on how quickly you want it done. I have some work partially done that I can adapt to this, and then push to a new repo on react-native-community. I realize it's been a couple weeks here, so if anything has changed in the meantime, feel free to ping me on Slack.

@jamonholmgren

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 8, 2018

One comment I have is that it seems that the extraction could go all-in on WKWebView and leave behind the legacy UIWebView code. Right now, there is code that switches between the two view managers. I would suggest that we drop that in the extracted code base and only support WKWebView. Any thoughts?

@fungilation

This comment has been minimized.

Copy link

commented Sep 8, 2018

I'm all for going with WKWebView. Performance, maintained by Apple, not deprecated, don't have weird JS/browser quirks, probably less crash prone (most of my app crashes trace back to webview native code based on Sentry reports) and all that. But perhaps have a phase out period (after spinout) so it's more battle tested before dropping UIWebView code?

@jamonholmgren

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 8, 2018

@fungilation I have UIWebView and WKWebView both working in my third-party package, now. It makes sense to support them both for at least a short while.

@jamonholmgren

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 9, 2018

I've published an initial draft of react-native-webview here:

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

Notes:

  • No tests yet
  • I've manually tested it in small apps, but nothing big yet
  • We'll need to move documentation over to that repo at some point
  • UIWebView is still the default, but WKWebView is supported through the useWebKit flag
  • Android works
  • Windows 10 support is planned (@rozele mentioned he might be willing to help with that)
  • It should work, with no conflicts, with React Native 0.56+
  • No Flow or TypeScript, yet. Any preferences as to which? (I prefer TypeScript by quite a lot, but am open to suggestions)

I'd like people to test it out in real world apps and see what they run into, and report the issues on the repo. Known issues with the existing RN core WebView are not going to be addressed, yet -- the goal is to make sure it's stable enough to release as-is. I'm looking for bugs I introduced in the conversion/extraction process.

Note: I've reached out to the owner of the abandoned react-native-webview package on NPM and will hopefully be able to use that package name.

@kelset

This comment has been minimized.

Copy link
Member

commented Sep 9, 2018

Awesome work @jamonholmgren , we'll make sure to link it into the Changelog :)

@satya164

This comment has been minimized.

Copy link
Member

commented Sep 9, 2018

No Flow or TypeScript, yet. Any preferences as to which?

I think Flow is a better choice because RN core uses Flow. But I also think ideally we should have both.

@fungilation

This comment has been minimized.

Copy link

commented Sep 9, 2018

Does what RN uses matter for WebView after it is spun out? If not, TypeScript have my vote.

@Titozzz

This comment has been minimized.

Copy link

commented Sep 9, 2018

I'm using typescript too but better solution would be to support both I guess

@jamonholmgren

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 10, 2018

I've migrated (with @Titozzz's help) the issues labeled WebView over to https://github.com/react-native-community/react-native-webview/issues. If the RN core team is okay with moving forward with this proposal, all of the WebView-related issues on the React Native repo can be closed, and going forward new WebView-related issues can be posted to the new repo.

We plan to go through pull requests at a later date.

@koenpunt

This comment has been minimized.

Copy link

commented Sep 10, 2018

  1. There are a lot of small useful modules which uses WebView behind the scenes for things to work. Some quick examples are react-native-qrcode, react-native-webview-crypto, react-native-editor, etc...

I think the solution for these implementations is to not use a webview (e.g., the QR code thing can also be achieved using React ART), or include a custom implementation of a native webview themselves.

@axe-fb

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2018

Merging this PR, since this is now approved. Thanks a lot @jamonholmgren for the amazing work !!

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.