Android: Support HTTP headers for source prop on <Image> components #7791

rigdern · 2016-05-27T01:40:13Z

This PR depends on Fresco PR facebook/fresco#1459

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.

Related to PR #7338 which is for the iOS implementation.

Test plan (required)

Ran a small example like the one above and ensured the headers were sent to the server.
Ran a small example to ensure that components without headers still work.
Currently using this code in our app.

Adam Comella
Microsoft Corp.

ghost · 2016-05-27T01:41:22Z

By analyzing the blame information on this pull request, we identified @foghina and @brentvatne to be potential reviewers.

nicklockwood · 2016-06-06T13:29:19Z

cc: @dmmiller - we merged the iOS version of this last week, so it would be great to get Android support landed too!

dmmiller · 2016-06-06T16:16:27Z

ReactAndroid/src/main/java/com/facebook/react/modules/fresco/FrescoModule.java

@@ -99,8 +99,8 @@ private static ImagePipelineConfig getDefaultConfig(
    }

    OkHttpClient okHttpClient = OkHttpClientProvider.getOkHttpClient();
-    ImagePipelineConfig.Builder builder =
-        OkHttpImagePipelineConfigFactory.newBuilder(context.getApplicationContext(), okHttpClient);


I think you can remove the import for OkHttpImagePipelineConfigFactory.

dmmiller · 2016-06-06T16:20:03Z

Few questions inline, seems good to me, but also ccing @andreicoman11 who has been looking at the image fetching code lately.

dmmiller · 2016-06-06T16:39:39Z

@rigdern Looking at this, I think it would be better to add this capability first to Fresco (also open source) and then we can pull the update into RN. I'd rather not create an additional copy of a random Fresco file for just this. It seems like you can add the addHeader call when Fresco builds the request pretty easily, and then we can take this more easily.

ghost · 2016-07-06T17:00:39Z

@rigdern do you have any updates for this pull request? It's been a while since the last update so wanted to check in and see if you've looked at the requested changes.

rigdern · 2016-07-06T18:37:46Z

Thanks for checking in and sorry for the delay. I've been busy with some other stuff but I plan on making the requested changes soon.

ospfranco · 2016-08-18T15:19:35Z

Any word on when will this be merged?

ospfranco · 2016-08-29T23:53:18Z

Can somebody else take over this ticket and finish the android integration? @dmmiller @andreicoman11 @rigdern

rigdern · 2016-08-30T00:42:59Z

@ospfranco I apologize I haven't been able to finish this PR yet. Based on the earlier discussion in this PR, part of my change needs to be redone within the Fresco repo. Before I can do that, I need to wait for an internal request to be approved which will ensure I can safely contribute to the Fresco project.

Adam Comella
Microsoft Corp.

ospfranco · 2016-09-06T14:58:05Z

I understand, thanks @rigdern looking forward to having this released

facebook-github-bot · 2016-09-07T00:34:49Z

@rigdern updated the pull request - view changes

rigdern · 2016-09-07T00:38:57Z

@dmmiller I moved some of the functionality into Fresco (PR facebook/fresco#1459). I also pushed an update to this PR which consumes the new Fresco feature. Before this PR can be merged, we'll have to get the Fresco PR merged and then update React Native to depend on that version of Fresco.

rigdern · 2017-02-02T05:25:22Z

@AndrewJack thanks for the suggestion. I updated to Fresco 1.0.1 and fixed the buck compilation errors. Let's see if the CI system is happy.

mkonicek · 2017-02-06T14:30:15Z

@rigdern Tells me this has been reviewed and should be merged.

@facebook-github-bot shipit

facebook-github-bot · 2017-02-06T15:48:14Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

facebook-github-bot · 2017-02-07T15:48:20Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

mobinni · 2017-02-13T15:28:04Z

Maybe we should update to 1.0.1, since it solves EXIF problems:
facebook/fresco#1558

@rigdern

rigdern · 2017-02-13T20:12:08Z

@mobinni Thanks, the PR was already updated to Fresco 1.0.1.

robinvw1 · 2017-02-15T09:53:18Z

Also would like to see this getting merged, since it already works on iOS. 👍

Summary: A copy of #7791 because of our very imperfect tools that mirror the changes from pull requests in the fb monorepo. The internal Phabricator revision for #7791 is in an 'abandoned' state (by foghina probably because of changing teams) and Phabricator doesn't allow me to claim that revision and merge it. Therefore I'm creating a new one. (It's not foghina's fault, no one probably knew about this "abandoned Phabricator revision" edge case, don't remember we hit it before.) Will try to keep attribution (git blame) to rigdern when merging. Closes #12448 Differential Revision: D4584743 Pulled By: mkonicek fbshipit-source-id: 66e5b88134fca1980adc4cd8a2ff17c42e10022c

mkonicek · 2017-02-18T15:45:19Z

I merged this as #12448. The code is in master now, although my attempt failed to edit the commit message in Phabricator to make the code blame to @rigdern. Sorry about that! Hopefully the good news this is merged will outweigh the fact I messed up the attribution!

rigdern · 2017-02-19T00:12:16Z

@mkonicek No worries. Thanks for getting this merged!

Traubenfuchs · 2017-02-26T21:01:23Z

When will this be available via npm?

Summary: A copy of facebook#7791 because of our very imperfect tools that mirror the changes from pull requests in the fb monorepo. The internal Phabricator revision for facebook#7791 is in an 'abandoned' state (by foghina probably because of changing teams) and Phabricator doesn't allow me to claim that revision and merge it. Therefore I'm creating a new one. (It's not foghina's fault, no one probably knew about this "abandoned Phabricator revision" edge case, don't remember we hit it before.) Will try to keep attribution (git blame) to rigdern when merging. Closes facebook#12448 Differential Revision: D4584743 Pulled By: mkonicek fbshipit-source-id: 66e5b88134fca1980adc4cd8a2ff17c42e10022c

rigdern · 2017-03-01T02:47:17Z

@Traubenfuchs Here's a description of React Native's release cadence: http://facebook.github.io/react-native/blog/2017/01/07/monthly-release-cadence.html.

Based on that, my assumption is that this change will be available in the March (0.43) version of React Native. This means you can get a release candidate containing this change at the beginning of March and a stable release containing this change at the end of March.

Summary: A copy of facebook#7791 because of our very imperfect tools that mirror the changes from pull requests in the fb monorepo. The internal Phabricator revision for facebook#7791 is in an 'abandoned' state (by foghina probably because of changing teams) and Phabricator doesn't allow me to claim that revision and merge it. Therefore I'm creating a new one. (It's not foghina's fault, no one probably knew about this "abandoned Phabricator revision" edge case, don't remember we hit it before.) Will try to keep attribution (git blame) to rigdern when merging. Closes facebook#12448 Differential Revision: D4584743 Pulled By: mkonicek fbshipit-source-id: 66e5b88134fca1980adc4cd8a2ff17c42e10022c

gayancliyanage · 2017-08-01T12:42:22Z

Can any please update of the current status of this feature? @mkonicek @rigdern

rigdern · 2017-08-01T18:34:43Z

@gayancliyanage looking at this PR's history, this PR was closed in favor of PR #12448 by mkonicek. Looking at that PR, it was merged as commit 8c0e6ec. And looking at that commit, we can see the first stable release of React Native to include that commit is 0.43:

rigdern mentioned this pull request May 27, 2016

iOS: Support HTTP headers for source prop on <Image> components #7338

Closed

ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels May 27, 2016

dmmiller reviewed Jun 6, 2016
View reviewed changes

ide added GH Review: needs-revision and removed GH Review: review-needed labels Jun 6, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 12, 2016

ospfranco mentioned this pull request Aug 9, 2016

Added documentation for additional fields of Image #9304

Closed

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 30, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 6, 2016

rigdern mentioned this pull request Sep 7, 2016

Split OkHttpNetworkFetcher's fetch method to make part of it reusable facebook/fresco#1459

Closed

rigdern force-pushed the rigdern/image-headers-android branch from b888fa1 to c74001d Compare September 7, 2016 00:34

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Sep 7, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 7, 2016

Adam Comella added 3 commits February 1, 2017 20:43

Remove trailing spaces

a487dff

BUCK: Bump fresco to 1.0.0

bb6b80f

Add missing BUCK dependencies

c104038

rigdern force-pushed the rigdern/image-headers-android branch from c7428d4 to c104038 Compare February 2, 2017 04:50

Upgrade from Fresco 1.0.0 to 1.0.1

0f78dd4

facebook-github-bot added Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Feb 6, 2017

facebook-github-bot added Import Failed Import Started This pull request has been imported. This does not imply the PR has been approved. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2017

facebook-github-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 7, 2017

mkonicek mentioned this pull request Feb 18, 2017

Android: Support HTTP headers for source prop on <Image> components #12448

Closed

mkonicek closed this Feb 18, 2017

Android: Support HTTP headers for source prop on <Image> components #7791

Android: Support HTTP headers for source prop on <Image> components #7791

Uh oh!

Conversation

rigdern commented May 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 27, 2016

Uh oh!

nicklockwood commented Jun 6, 2016

Uh oh!

dmmiller Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

dmmiller commented Jun 6, 2016

Uh oh!

dmmiller commented Jun 6, 2016

Uh oh!

ghost commented Jul 6, 2016

Uh oh!

rigdern commented Jul 6, 2016

Uh oh!

ospfranco commented Aug 18, 2016

Uh oh!

ospfranco commented Aug 29, 2016

Uh oh!

rigdern commented Aug 30, 2016

Uh oh!

ospfranco commented Sep 6, 2016

Uh oh!

facebook-github-bot commented Sep 7, 2016

Uh oh!

rigdern commented Sep 7, 2016

Uh oh!

rigdern commented Feb 2, 2017

Uh oh!

mkonicek commented Feb 6, 2017

Uh oh!

facebook-github-bot commented Feb 6, 2017

Uh oh!

facebook-github-bot commented Feb 7, 2017

Uh oh!

mobinni commented Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rigdern commented Feb 13, 2017

Uh oh!

robinvw1 commented Feb 15, 2017

Uh oh!

mkonicek commented Feb 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rigdern commented Feb 19, 2017

Uh oh!

Traubenfuchs commented Feb 26, 2017

Uh oh!

rigdern commented Mar 1, 2017

Uh oh!

gayancliyanage commented Aug 1, 2017

Uh oh!

rigdern commented Aug 1, 2017

Uh oh!

Uh oh!

rigdern commented May 27, 2016 •

edited

Loading

mobinni commented Feb 13, 2017 •

edited

Loading

mkonicek commented Feb 18, 2017 •

edited

Loading