-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
iOS: Support HTTP headers for source prop on <Image> components #7338
Conversation
By analyzing the blame information on this pull request, we identified @nicklockwood and @ide to be potential reviewers. |
@@ -63,10 +63,14 @@ var Image = React.createClass({ | |||
* `uri` is a string representing the resource identifier for the image, which | |||
* could be an http address, a local file path, or the name of a static image | |||
* resource (which should be wrapped in the `require('./path/to/image.png')` function). | |||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-trailing-spaces: Trailing spaces not allowed.
796939f
to
426ad27
Compare
@rigdern updated the pull request. |
426ad27
to
fed4637
Compare
@rigdern updated the pull request. |
1 similar comment
@rigdern updated the pull request. |
1eb2054
to
32d7c77
Compare
@rigdern updated the pull request. |
32d7c77
to
677ee37
Compare
@rigdern updated the pull request. |
CGSize size = CGSizeZero; | ||
CGFloat scale = 1.0; | ||
BOOL packagerAsset = NO; | ||
if ([json isKindOfClass:[NSDictionary class]]) { | ||
if (!(imageURL = [self NSURL:RCTNilIfNull(json[@"uri"])])) { | ||
return nil; | ||
} | ||
headers = [self NSDictionary:RCTNilIfNull(json[@"headers"])]; | ||
#if RCT_DEBUG | ||
if (headers != nil) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just check if (!headers)
@rigdern You're awesome for submitting both Android and iOS implementations that both look good at a glance. Would like to get more reviewers on the code, but this PR looks promising. |
@rigdern This is a good idea, but changing these iOS APIs will break quite a lot of our stuff internally. I think the better approach here would be to add a new method to RCTImageLoader that takes an NSURLRequest, which we already have an RCTConvert method for processing in a standard format (it's already used by WebView.source). Validating that the headers are all strings should also be done inside RCTConvert (though it isn't currently). The existing methods of RCTImageLoader can then be refactored to call the new method, so we don't break anything, and we can deprecate the old methods. Also, while I echo @ide's sentiment about building both iOS and Android implementations, it's much easier for us to merge if it's split into separate PRs for each platform. |
Allows developers to specify headers to include in the HTTP request when fetching a remote image. For example, one might leverage this when fetching an image from an endpoint that requires authentication: ``` <Image style={styles.logo} source={{ uri: 'http://facebook.github.io/react/img/logo_og.png', headers: { Authorization: 'someAuthToken' } }} /> ``` Note that the header values must be strings. When doing image requests, this also gives developers control over the other properties supported by [RCTConvert NSURLRequest:] like `method` and `body`. This change deprecates some APIs on RCTImageLoader in order to replace them with more flexible APIs which take an NSURLRequest rather than just an NSURL.
677ee37
to
ca1f5a7
Compare
@rigdern updated the pull request. |
@nicklockwood As you suggested, I created several new RCTImageLoader methods which accept an NSURLRequest and deprecated the old ones which accepted an NSURL. I also removed the Android implementation of the feature and will submit it in a separate PR. |
Android PR is #7791 |
@facebook-github-bot import |
ee8496f
Summary: Allows developers to specify headers to include in the HTTP request when fetching a remote image. For example, one might leverage this when fetching an image from an endpoint that requires authentication: ``` <Image style={styles.logo} source={{ uri: 'http://facebook.github.io/react/img/logo_og.png', headers: { Authorization: 'someAuthToken' } }} /> ``` Note that the header values must be strings. Works on iOS and Android. **Test plan (required)** - Ran a small example like the one above on iOS and Android and ensured the headers were sent to the server. - Ran a small example to ensure that \<Image\> components without headers still work. - Currently using this code in our app. Adam Comella Microsoft Corp. Closes facebook/react-native#7338 Reviewed By: javache Differential Revision: D3371458 Pulled By: nicklockwood fbshipit-source-id: cdb24fe2572c3ae3ba82c86ad383af6d85157e20
@rigdern - Did this PR get accepted and, if so, which version of RN did it first appear in? EDIT -- nevermind... I see it was accepted. |
Summary: Allows developers to specify headers to include in the HTTP request when fetching a remote image. For example, one might leverage this when fetching an image from an endpoint that requires authentication: ``` <Image style={styles.logo} source={{ uri: 'http://facebook.github.io/react/img/logo_og.png', headers: { Authorization: 'someAuthToken' } }} /> ``` Note that the header values must be strings. Works on iOS and Android. **Test plan (required)** - Ran a small example like the one above on iOS and Android and ensured the headers were sent to the server. - Ran a small example to ensure that \<Image\> components without headers still work. - Currently using this code in our app. Adam Comella Microsoft Corp. Closes facebook#7338 Reviewed By: javache Differential Revision: D3371458 Pulled By: nicklockwood fbshipit-source-id: cdb24fe2572c3ae3ba82c86ad383af6d85157e20
Summary: Allows developers to specify headers to include in the HTTP request when fetching a remote image. For example, one might leverage this when fetching an image from an endpoint that requires authentication: ``` <Image style={styles.logo} source={{ uri: 'http://facebook.github.io/react/img/logo_og.png', headers: { Authorization: 'someAuthToken' } }} /> ``` Note that the header values must be strings. Works on iOS and Android. **Test plan (required)** - Ran a small example like the one above on iOS and Android and ensured the headers were sent to the server. - Ran a small example to ensure that \<Image\> components without headers still work. - Currently using this code in our app. Adam Comella Microsoft Corp. Closes facebook#7338 Reviewed By: javache Differential Revision: D3371458 Pulled By: nicklockwood fbshipit-source-id: cdb24fe2572c3ae3ba82c86ad383af6d85157e20
Are there any working examples? This is not working for me and I am not sure how to debug this.. |
@IlyaRepo You want to do something like this, which is working for me
|
Summary: Allows developers to specify headers to include in the HTTP request when fetching a remote image. For example, one might leverage this when fetching an image from an endpoint that requires authentication: ``` <Image style={styles.logo} source={{ uri: 'http://facebook.github.io/react/img/logo_og.png', headers: { Authorization: 'someAuthToken' } }} /> ``` Note that the header values must be strings. Works on iOS and Android. **Test plan (required)** - Ran a small example like the one above on iOS and Android and ensured the headers were sent to the server. - Ran a small example to ensure that \<Image\> components without headers still work. - Currently using this code in our app. Adam Comella Microsoft Corp. Closes facebook#7338 Reviewed By: javache Differential Revision: D3371458 Pulled By: nicklockwood fbshipit-source-id: cdb24fe2572c3ae3ba82c86ad383af6d85157e20
Summary: Hi there, As ospfranco found there is some documentation for the code coming from #7338 missing, so I added it. It concerns the method, headers and bdoy field on the Image source object. If you would like to have any changes made, please don't hesitate to comment, I will add them. Have a nice day and thank you for maintaining React Native! Closes #9304 Differential Revision: D4913262 Pulled By: javache fbshipit-source-id: 922430ec3388560686e1cf53cb5dff7f30e4e31f
Summary: Hi there, As ospfranco found there is some documentation for the code coming from facebook#7338 missing, so I added it. It concerns the method, headers and bdoy field on the Image source object. If you would like to have any changes made, please don't hesitate to comment, I will add them. Have a nice day and thank you for maintaining React Native! Closes facebook#9304 Differential Revision: D4913262 Pulled By: javache fbshipit-source-id: 922430ec3388560686e1cf53cb5dff7f30e4e31f
Summary: Hi there, As ospfranco found there is some documentation for the code coming from facebook#7338 missing, so I added it. It concerns the method, headers and bdoy field on the Image source object. If you would like to have any changes made, please don't hesitate to comment, I will add them. Have a nice day and thank you for maintaining React Native! Closes facebook#9304 Differential Revision: D4913262 Pulled By: javache fbshipit-source-id: 922430ec3388560686e1cf53cb5dff7f30e4e31f
Headers not working on ios, android works well |
Summary: Allows developers to specify headers to include in the HTTP request when fetching a remote image. For example, one might leverage this when fetching an image from an endpoint that requires authentication: ``` <Image style={styles.logo} source={{ uri: 'http://facebook.github.io/react/img/logo_og.png', headers: { Authorization: 'someAuthToken' } }} /> ``` Note that the header values must be strings. Works on iOS and Android. **Test plan (required)** - Ran a small example like the one above on iOS and Android and ensured the headers were sent to the server. - Ran a small example to ensure that \<Image\> components without headers still work. - Currently using this code in our app. Adam Comella Microsoft Corp. Closes facebook/react-native#7338 Reviewed By: javache Differential Revision: D3371458 Pulled By: nicklockwood fbshipit-source-id: cdb24fe2572c3ae3ba82c86ad383af6d85157e20
Summary: Allows developers to specify headers to include in the HTTP request when fetching a remote image. For example, one might leverage this when fetching an image from an endpoint that requires authentication: ``` <Image style={styles.logo} source={{ uri: 'http://facebook.github.io/react/img/logo_og.png', headers: { Authorization: 'someAuthToken' } }} /> ``` Note that the header values must be strings. Works on iOS and Android. **Test plan (required)** - Ran a small example like the one above on iOS and Android and ensured the headers were sent to the server. - Ran a small example to ensure that \<Image\> components without headers still work. - Currently using this code in our app. Adam Comella Microsoft Corp. Closes facebook/react-native#7338 Reviewed By: javache Differential Revision: D3371458 Pulled By: nicklockwood fbshipit-source-id: cdb24fe2572c3ae3ba82c86ad383af6d85157e20
Allows developers to specify headers to include in the HTTP request
when fetching a remote image. For example, one might leverage this
when fetching an image from an endpoint that requires authentication:
Note that the header values must be strings.
Works on iOS and Android.
Test plan (required)
Adam Comella
Microsoft Corp.