[Image] Add a way to prefetch remote images to cache with Image.prefetch #6774
Conversation
|
By analyzing the blame information on this pull request, we identified @davidaurelio, @brentvatne and @dralletje to be potential reviewers. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
cc @mkonicek for Android changes. I believe you were satisfied with the Android implementation before. I cleaned it up a bit more but it is based on the same Fresco API. cc @nicklockwood for iOS changes. I addressed your feedback you had in the previous PR, specifically removing the -Async suffix and moving the method out of the RCTNetworking class. I put the method in RCTImageLoader rather than in RCTImageViewManager for consistency with Android, because on Android the view managers can't simultaneously be bridge modules with exported methods so I had to make an ImageLoader module on Android instead. |
|
FYI Buck build is broken |
|
@ide updated the pull request. Tests pass now - ide |
| return; | ||
| } | ||
| promise.resolve(true); | ||
| dataSource.close(); |
There was a problem hiding this comment.
Should this be in a finally block just in case the code above throws an exception?
There was a problem hiding this comment.
I'll read the Fresco code more to see if this call is even necessary -- then we can either remove the unnecessary call, or will add a finally block if it is necessary.
There was a problem hiding this comment.
close() sets some internal state and I verified that the DataSource isn't automatically closed, so I believe it's correct to call close(). I'll add the finally blocks and ship this.
|
I think Nick might be too busy to review pull requests. Did he comment on the original PR? If you're using this in production might be fine to just #shipit and iterate on it. |
|
@mkonicek thanks for the review. On the previous PR, Nick did comment on it and requested two changes that are both addressed in this PR. The iOS implementation is also quite small and easy to change if we'd like to. |
Adds `Image.prefetch` to prefetch remote images before they are used in an actual `Image` component. Test Plan: - Image demo in UIExplorer on Android and iOS. Performance is better on iOS but in both cases you can see that loading a prefetched image is faster than a non-prefetched one. - Using this in production - CI
|
@ide updated the pull request. |
|
@facebook-github-bot shipit |
facebook-github-bot
added
GH Review: accepted
and removed
GH Review: review-needed
labels
|
Thanks for importing. If you are an FB employee go to Phabricator to review. |
|
@facebook-github-bot shipit |
|
Thanks for importing. If you are an FB employee go to Phabricator to review. |
|
@mkonicek could you look into why the merge failed? |
|
Buck can't see the |
|
Investigating, Sandcastle complains about CallerThreadExecutor not being found |
|
A first guess - buckconfig in fbsource is different from buckonfig in react-native. |
|
@ide @mkonicek I think I got it. define |
ghost
closed this
in
|
@bestander thank you for investigating and figuring out how to get it to build =) |
Summary:Adds `Image.prefetch` to prefetch remote images before they are used in an actual `Image` component. This is based off of facebook#4420 by sospartan and skevy's work. Closes facebook#6774 Differential Revision: D3153729 Pulled By: bestander fb-gh-sync-id: ef61412e051a49b42ae885edce7905a8ca0da23f fbshipit-source-id: ef61412e051a49b42ae885edce7905a8ca0da23f
Summary:Adds `Image.prefetch` to prefetch remote images before they are used in an actual `Image` component. This is based off of facebook#4420 by sospartan and skevy's work. Closes facebook#6774 Differential Revision: D3153729 Pulled By: bestander fb-gh-sync-id: ef61412e051a49b42ae885edce7905a8ca0da23f fbshipit-source-id: ef61412e051a49b42ae885edce7905a8ca0da23f
Adds
Image.prefetchto prefetch remote images before they are used in an actualImagecomponent. This is based off of #4420 by sospartan and skevy's work.Test Plan: