-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in a finally block just in case the code above throws an exception?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 |
f7bcb3e
@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.prefetch
to prefetch remote images before they are used in an actualImage
component. This is based off of #4420 by sospartan and skevy's work.Test Plan: