Add headers prop to Android WebViews #5494

mtostenson · 2016-01-22T23:18:26Z

This is a follow-up to this previous pull request.

~~Adds a new ReactProp 'urlWithHeaders' to Android WebViews that takes an object with a 'url' string and a 'headers' map.~~

[Update] Adds a new prop 'source' to Android WebViews

{
   html: string,
   url: string,
   headers: map<string, string>,
}

Update: resolves TODO 8495359

facebook-github-bot · 2016-01-22T23:22:17Z

@mtostenson updated the pull request.

eslint-bot · 2016-01-23T00:02:03Z

Libraries/Components/WebView/WebView.android.js

@@ -127,12 +133,23 @@ var WebView = React.createClass({
      domStorageEnabled = this.props.domStorageEnabledAndroid;
    }

+    var urlProp, urlWithHeaders;


space-after-keywords: Keyword "if" must be followed by whitespace.

facebook-github-bot · 2016-01-23T00:47:15Z

@mtostenson updated the pull request.

satya164 · 2016-01-23T18:28:44Z

LGTM

cc @mkonicek @nicklockwood

facebook-github-bot · 2016-01-23T19:50:19Z

@mtostenson updated the pull request.

satya164 · 2016-01-23T20:04:44Z

@vjeux @mkonicek The bot seems to be replying to PRs even when nothing changed :(

janicduplessis · 2016-01-24T01:32:13Z

ReactAndroid/src/main/java/com/facebook/react/views/webview/ReactWebViewManager.java

@@ -280,6 +283,19 @@ public void setHtml(WebView view, @Nullable String html) {
    }
  }

+  @ReactProp(name = "headers")


I don't think adding a third prop that calls loadUrl is a good idea, there's actually to TODO comment in setUrl to do something about setUrl and setHtml. It could be a good time to fix that.

We could do something like a source prop to the native component that takes an object like

{ html: string, url: string, headers: Map<string, string>, }

and then if html is defined, use it, else use url and if headers are defined, add them. That way changing between html, url and url with headers won't the make webview load the wrong thing because of the order props were set.

Edit: Just noticed something similar was already mentioned in the original PR :)

satya164 · 2016-01-24T09:43:31Z

A single object will be really great, as Nick suggested. But we also have to support the old way for a while before removing them. We can show console.warn for a release it two before completely removing it.

mtostenson · 2016-01-25T18:40:50Z

That seems similar to the first solution I proposed. I would be in favor of a single source prop, but I agree @satya164 we will have to include warnings for both the url and html props that they will be deprecated.

This deprecates url and html props for Android WebView. source prop takes an object { html: string, url: string, headers: map<string, string> }

facebook-github-bot · 2016-01-26T01:24:16Z

@mtostenson updated the pull request.

eslint-bot · 2016-01-26T03:07:22Z

Libraries/Components/WebView/WebView.android.js

+      console.warn('html is deprecated. Use source instead.');
+      htmlProp = this.props.html;
+    } else if(this.props.url) {
+      console.warn('url is deprecated. Use source instead.');


space-after-keywords: Keyword "if" must be followed by whitespace.

I'd prefer a bit more informative warnings. When I see YellowBox with this warning, it'll not be obvious to me regarding where it's coming from and what to do.

Some thing like,

WebView: url prop is deprecated. Use the source prop instead.

Since this pattern comes up a lot, perhaps we can come up with a standard way to mark props as deprecated that includes the component info? I think someone mentioned such a thing already exists in React.js?

@nicklockwood That'll be awesome. Though I'm not aware of any such functionality in React. Maybe we could do this with propTypes?

Something like,

function deprecatedPropType(checker, message) { return (props, propName, componentName) => { console.warn(`${componentName}: ${propName} prop is deprecated. ${mesage}`); return checker(props, propName, componentName); }; } propTypes = { url: deprecatedPropType(React.PropTypes.string, "Use source prop instead.") }

This is a good idea, do you have a recommendation for where this deprecatedPropType method should go so that any component can access it?

It should be a module in its own right, so it can be imported from anywhere.

There doesn't seem to be anything in react to do this. Something like https://github.com/react-bootstrap/react-prop-types/blob/master/src/deprecated.js would be nice, it could eventually be integrated with the website too.

If you like I can submit a PR that implements that and change the places we deprecate props manually.

Sounds good to me, I'll leave the warnings as they are for the now.

nicklockwood · 2016-01-26T09:15:24Z

I really like this solution :-)

nicklockwood · 2016-01-26T09:22:01Z

Libraries/Components/WebView/WebView.android.js

+      urlProp = this.props.url;
+    } else {
+      sourceProp = this.props.source;
+    }


In the interests of code cleanliness, you can probably just remove the native html and url props completely, and implement support for them on the JS side by using them to build the source object. Something like:

var source = this.props.source || {}; if (this.props.html) { console.warn('html is deprecated. Use source instead.'); source.html = this.props.html; } else if (this.props.url) { console.warn('url is deprecated. Use source instead.'); source.url = this.props.url; } <RCTWebView ref={RCT_WEBVIEW_REF} key="webViewKey" style={webViewStyles} source={source} // no need for url/html any more ... />

Heh, that's a really good idea.

janicduplessis · 2016-01-26T13:44:50Z

👍

janicduplessis · 2016-01-26T13:57:13Z

It would be great to make the source prop work on iOS too but that could be in another PR.

Improve deprecated prop warnings.

facebook-github-bot · 2016-01-26T18:08:14Z

@mtostenson updated the pull request.

ncnlinh · 2016-01-29T21:35:51Z

I hope this will be merged and released soon!

nicklockwood · 2016-01-29T21:55:54Z

Sorry for the wait. I'll see what I can do to get this merged.

@facebook-github-bot shipit

facebook-github-bot · 2016-01-29T21:56:27Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/179867235705034/int_phab to review.

nicklockwood · 2016-01-30T22:20:51Z

@mtostenson How would you feel about me changing the url prop to uri? It's just that this source object is quite similar to the one we use for images, and I see some potential for code reuse in the future if they follow the same structure as much as possible.

mtostenson · 2016-01-30T23:06:53Z

@nicklockwood This prop results in a call to loadUrl so I think URL is preferable because it sticks with the naming convention of the native view.

nicklockwood · 2016-01-30T23:10:34Z

@mtostenson we're already moving away from the native view conventions by using a source property, so I think consistency with React conventions is more important, don't you? It seems odd to me to have

<Image source={{uri: foo}}/>

But then

<WebView source={{url: foo}}/>

But if you're concerned that it will be confusing to use uri for WebViews, we could always check for url and display an error, so users are clear on what they've got wrong.

mtostenson · 2016-01-31T06:52:33Z

@nicklockwood I see what you mean, I'm fine gonig with URI. Since it's all documented I don't think it's necessary to include the URL warning message, but it can't hurt.

nicklockwood · 2016-01-31T08:23:45Z

@mtostenson great, I'll make that change then.

I'm also working on adding this to iOS so we can use it x-platform.

Thanks again for the PR, this is an awesome improvement.

janicduplessis · 2016-01-31T08:29:00Z

@nicklockwood deprecatedPropType got merged in master if you want to make the change to use it before merging this.

nicklockwood · 2016-01-31T09:01:03Z

@janicduplessis Yep, I'll do that.

janicduplessis · 2016-02-01T22:33:34Z

It got merged :)

mtostenson · 2016-02-01T22:42:56Z

thanks @janicduplessis

nicklockwood · 2016-02-02T00:23:52Z

I have a follow up commit waiting that adds support for iOS as well. Just dealing with some Flow validation issues.

Summary: public #5494 added a new `source` property to WebView on Android that provides a better API, as well as allowing for request headers to be set. This diff ports that functionality over to iOS, so we can have a consistent API cross-platform. I've also extended the API to include `method` (GET or POST) and `body` when setting the WebView content with a URI, and `baseUrl` when setting static HTML. Reviewed By: javache Differential Revision: D2884643 fb-gh-sync-id: 83f24494bdbb4e1408aa8f3b7428fee33888ae3a

Add headers prop to Android WebViews

6abb684

mtostenson mentioned this pull request Jan 22, 2016

Add custom header support for Android WebViews #5419

Closed

facebook-github-bot added the GH Review: review-needed label Jan 22, 2016

eslint-bot reviewed Jan 23, 2016
View reviewed changes

Update per linter bot recommendations

b0ba22e

janicduplessis reviewed Jan 24, 2016
View reviewed changes

Add source prop to Android WebView

06b8935

This deprecates url and html props for Android WebView. source prop takes an object { html: string, url: string, headers: map<string, string> }

eslint-bot reviewed Jan 26, 2016
View reviewed changes

nicklockwood reviewed Jan 26, 2016
View reviewed changes

Cleanup deprecated props methods from WebViewManager

63e3be1

Improve deprecated prop warnings.

ghost closed this in 80a2f5d Feb 1, 2016

This pull request was closed.

Add headers prop to Android WebViews #5494

Add headers prop to Android WebViews #5494

Conversation

mtostenson commented Jan 22, 2016

facebook-github-bot commented Jan 22, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Jan 23, 2016

satya164 commented Jan 23, 2016

facebook-github-bot commented Jan 23, 2016

satya164 commented Jan 23, 2016

Choose a reason for hiding this comment

satya164 commented Jan 24, 2016

mtostenson commented Jan 25, 2016

facebook-github-bot commented Jan 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklockwood commented Jan 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis commented Jan 26, 2016

janicduplessis commented Jan 26, 2016

facebook-github-bot commented Jan 26, 2016

ncnlinh commented Jan 29, 2016

nicklockwood commented Jan 29, 2016

facebook-github-bot commented Jan 29, 2016

nicklockwood commented Jan 30, 2016

mtostenson commented Jan 30, 2016

nicklockwood commented Jan 30, 2016

mtostenson commented Jan 31, 2016

nicklockwood commented Jan 31, 2016

janicduplessis commented Jan 31, 2016

nicklockwood commented Jan 31, 2016

janicduplessis commented Feb 1, 2016

mtostenson commented Feb 1, 2016

nicklockwood commented Feb 2, 2016