Allow prefetch remote image #4420

sospartan · 2015-11-30T05:56:56Z

Allow prefetch remote image to disk cache for later use .
This will be useful for prepare important image before render (e.g. background image )

facebook-github-bot · 2015-11-30T05:58:24Z

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

nicklockwood · 2015-11-30T13:19:19Z

Can we add this facility to the existing RCTNetworking module? I don't really see the need to add a second module for making network requests when we already have one.

Also, we should be providing JS wrappers for all native module functions. Accessing native modules directly makes it hard to manage cross-platform differences.

nicklockwood · 2015-11-30T13:25:47Z

I like the idea of this, but I don't think it makes sense to add a prefetch property to . You'll mostly want to prefetch images before rendering them, so having to render an image in order to prefetch the uri so you can render the image doesn't make sense to me as a workflow. - never mind, I didn't read the code carefully enough.

On iOS I've been toying with adding an ImageLoader.preload() method. I might still add this, but it wouldn't be applicable to loading font files, or any non-images, which would also be useful.

The advantages of making this a method of the ImageLoader module (at least on iOS) are:

The ImageLoader already owns the image cache, so it makes sense implementation-wise
The method can return useful image metadata, such as the pixel dimensions (which is one of the main reasons I'm adding the function, in addition to preloading)

The disadvantages are:

Doesn't apply to other data types (although we don't currently have a cache for those on iOS anyway)
ImageLoader isn't a cross-platform module (yet)

I think I need to discuss this with @dmmiller

sospartan · 2015-11-30T16:45:26Z

It's a good idea to use the 'RCTNetworking' native module. I'll fix that.
Can you can give an example of other common data types may need this facility ? Otherwise I think a 'File Operation' module working with fetch is more reasonable (I'm waiting this from day one 😄 ).

facebook-github-bot · 2015-11-30T18:15:35Z

@sospartan updated the pull request.

facebook-github-bot · 2015-12-02T11:26:18Z

@sospartan updated the pull request.

facebook-github-bot · 2015-12-02T14:14:25Z

@sospartan updated the pull request.

sospartan · 2015-12-02T14:16:03Z

@nicklockwood @dmmiller I've added ios version. Like to hear your opinions on this pull request.

mkonicek · 2015-12-02T23:14:51Z

ReactAndroid/src/main/java/com/facebook/react/views/text/ReactFontManager.java

+        mFontCache.put(fontFamilyName, fontFamily);
+        fontFamily.setTypeface(style, typeface);
+      }
+    } catch (RuntimeException e) {


Consider logging.

sospartan · 2015-12-03T02:29:43Z

@mkonicek thanks for the suggestions.

facebook-github-bot · 2015-12-03T02:30:16Z

@sospartan updated the pull request.

brentvatne · 2015-12-03T02:38:16Z

@sospartan - thanks for your work here! looking forward to this landing 😄

sospartan · 2015-12-03T02:57:51Z

@brentvatne Glad to contribute to this project .

sospartan · 2015-12-06T06:33:10Z

@brentvatne @mkonicek @nicklockwood Any updates?

facebook-github-bot · 2015-12-07T03:09:17Z

@sospartan updated the pull request.

brentvatne · 2015-12-07T06:32:30Z

@sospartan - I'll leave this one up to @nicklockwood and @mkonicek, they are smarter than me

mkonicek · 2015-12-09T20:16:34Z

The font loading logic seems quite complex. Would you mind splitting this into two PRs? One for images, one for fonts, to make it easier to review?

mkonicek · 2015-12-09T20:18:03Z

How does Text.loadFont work? What font do all the Text elements display until the font is fetched? Are all the Text elements refreshed automatically once the font is fetched?

mkonicek · 2015-12-09T20:19:28Z

Examples/UIExplorer/ImageExample.js

-
+        {this.state.startLoadPrefetched?
+          <Image
+            source={this.props.sourcePrefetch}


nit: name this prefetchedSource

ide · 2015-12-09T20:21:41Z

I agree that this font loading approach looks complicated. What we do in our own projects is implement this entirely in JS except for downloading and installing the font (on iOS at least). This way the behavior of the font loader is something that we can customize instead of having it be something arbitrarily chosen by RN core. We try to keep code in user space.

Could the font loader be published as its own package to npm?

mkonicek · 2015-12-09T22:36:38Z

👍 Agree with @ide.

sospartan · 2015-12-10T02:45:16Z

There is no API provided to implement 'downloading' in pure JS as far as i known, because the lacking of 'File' API. All the behaviors behind this pull request are same as the original Text did: Font file loading failed(or render before download finish) will act same as the assets' font file is missing.

I'll reconsider the font part. It's uncompleted as your said and not necessary related to the image part.
Thanks for the review. @ide @mkonicek

facebook#4420

skevy · 2016-03-11T20:14:58Z

@nicklockwood I think it does actually.

See here: expo@d455cb6#diff-f8703d50cf5af4c06824e2928dddb500R504

Am I doing this wrong? It seems to work for our use case.

facebook#4420

nicklockwood · 2016-03-12T02:18:38Z

@skevy oh, hmm. I guess I was mistaken.

Summary: Expose method to implement changing font family cache. Like ide suggested in facebook#4420 , this will helpful for using remote font file (use `Typeface#createFromFile` to load downloaded font file). iOS's CoreText already allow this in native code. Closes facebook#4696 Reviewed By: bestander Differential Revision: D2911762 Pulled By: andreicoman11 fb-gh-sync-id: a931e2e711dd94fa0df6fdd066827756d862a4ba

mkonicek · 2016-03-15T17:49:32Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

+        uri = Uri.parse(url);
+      }
+    } catch (Exception e){
+      //ignore malformed url


nit: space after //

mkonicek · 2016-03-15T17:51:31Z

See here: exponentjs@d455cb6#diff-f8703d50cf5af4c06824e2928dddb500R504

@skevy Should this PR use that? Or do you want to submit your implementation instead of this PR?

skevy · 2016-03-15T20:20:20Z

Seems reasonable for it to use my implementation. Any thoughts @sospartan ?

sospartan · 2016-03-16T02:13:08Z

@skevy @mkonicek I'm glad someone like this PR and make a better implementation. Just go ahead.
This is created nearly 4 months ago ... I'm almost forget these code.

mkonicek · 2016-03-16T17:23:40Z

@sospartan Should we close this PR then? I hope you'll continue contributing in the future :)

nicklockwood · 2016-03-16T17:44:09Z

@skevy I'd be happy to accept your implementation; The only changes I'd ask for are 1) not to use the Async suffix, and 2) Move the method to RCTImageViewManager, as it doesn't really belong in the Networking module (the image modules depends on networking, not the other way around).

skevy · 2016-03-16T20:31:26Z

@nicklockwood sweet. Thanks. I will submit a new PR sometime in the very near future.

skevy · 2016-03-16T20:31:45Z

@sospartan thanks for the initial work on this!

facebook#4420

Summary:Adds `Image.prefetch` to prefetch remote images before they are used in an actual `Image` component. This is based off of #4420 by sospartan and skevy's work. Closes #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 #4420 by sospartan and skevy's work. Closes facebook/react-native#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

facebook-github-bot 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 Nov 30, 2015

sospartan changed the title ~~add Networking resource moudle~~ add Networking resource module Dec 1, 2015

sospartan changed the title ~~add Networking resource module~~ add Networking resource prefetch Dec 1, 2015

mkonicek reviewed Dec 2, 2015
View reviewed changes

mkonicek reviewed Dec 9, 2015
View reviewed changes

ide pushed a commit to expo/react-native that referenced this pull request Mar 11, 2016

[Exp] Support prefetch image.

718f08f

facebook#4420

ide pushed a commit to expo/react-native that referenced this pull request Mar 12, 2016

[Exp] Support prefetch image.

a488164

facebook#4420

ide pushed a commit to expo/react-native that referenced this pull request Mar 12, 2016

[Exp] Support prefetch image.

e32131c

facebook#4420

mkonicek reviewed Mar 15, 2016
View reviewed changes

skevy closed this Mar 16, 2016

skevy self-assigned this Mar 16, 2016

skevy pushed a commit to expo/react-native that referenced this pull request Mar 24, 2016

[Exp] Support prefetch image.

8483cd4

facebook#4420

skevy pushed a commit to expo/react-native that referenced this pull request Mar 24, 2016

[Exp] Support prefetch image.

b8081c7

facebook#4420

skevy pushed a commit to expo/react-native that referenced this pull request Mar 24, 2016

[Exp] Support prefetch image.

5395fad

facebook#4420

skevy pushed a commit to expo/react-native that referenced this pull request Mar 25, 2016

[Exp] Support prefetch image.

258dae4

facebook#4420

skevy pushed a commit to expo/react-native that referenced this pull request Mar 25, 2016

[Exp] Support prefetch image.

ffc6ce2

facebook#4420

skevy pushed a commit to expo/react-native that referenced this pull request Mar 31, 2016

[Exp] Support prefetch image.

4e7c0a1

facebook#4420

skevy pushed a commit to expo/react-native that referenced this pull request Apr 1, 2016

[Exp] Support prefetch image.

37b6850

facebook#4420

ide pushed a commit to expo/react-native that referenced this pull request Apr 1, 2016

[Exp] Support prefetch image.

3f51035

facebook#4420

ide mentioned this pull request Apr 2, 2016

[Image] Add a way to prefetch remote images to cache with Image.prefetch #6774

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow prefetch remote image #4420

Allow prefetch remote image #4420

sospartan commented Nov 30, 2015

facebook-github-bot commented Nov 30, 2015

nicklockwood commented Nov 30, 2015

nicklockwood commented Nov 30, 2015

sospartan commented Nov 30, 2015

facebook-github-bot commented Nov 30, 2015

facebook-github-bot commented Dec 2, 2015

facebook-github-bot commented Dec 2, 2015

sospartan commented Dec 2, 2015

mkonicek Dec 2, 2015

sospartan commented Dec 3, 2015

facebook-github-bot commented Dec 3, 2015

brentvatne commented Dec 3, 2015

sospartan commented Dec 3, 2015

sospartan commented Dec 6, 2015

facebook-github-bot commented Dec 7, 2015

brentvatne commented Dec 7, 2015

mkonicek commented Dec 9, 2015

mkonicek commented Dec 9, 2015

mkonicek Dec 9, 2015

ide commented Dec 9, 2015

mkonicek commented Dec 9, 2015

sospartan commented Dec 10, 2015

skevy commented Mar 11, 2016

nicklockwood commented Mar 12, 2016

mkonicek Mar 15, 2016

mkonicek commented Mar 15, 2016

skevy commented Mar 15, 2016

sospartan commented Mar 16, 2016

mkonicek commented Mar 16, 2016

nicklockwood commented Mar 16, 2016

skevy commented Mar 16, 2016

skevy commented Mar 16, 2016

Allow prefetch remote image #4420

Allow prefetch remote image #4420

Conversation

sospartan commented Nov 30, 2015

facebook-github-bot commented Nov 30, 2015

nicklockwood commented Nov 30, 2015

nicklockwood commented Nov 30, 2015

sospartan commented Nov 30, 2015

facebook-github-bot commented Nov 30, 2015

facebook-github-bot commented Dec 2, 2015

facebook-github-bot commented Dec 2, 2015

sospartan commented Dec 2, 2015

mkonicek Dec 2, 2015

Choose a reason for hiding this comment

sospartan commented Dec 3, 2015

facebook-github-bot commented Dec 3, 2015

brentvatne commented Dec 3, 2015

sospartan commented Dec 3, 2015

sospartan commented Dec 6, 2015

facebook-github-bot commented Dec 7, 2015

brentvatne commented Dec 7, 2015

mkonicek commented Dec 9, 2015

mkonicek commented Dec 9, 2015

mkonicek Dec 9, 2015

Choose a reason for hiding this comment

ide commented Dec 9, 2015

mkonicek commented Dec 9, 2015

sospartan commented Dec 10, 2015

skevy commented Mar 11, 2016

nicklockwood commented Mar 12, 2016

mkonicek Mar 15, 2016

Choose a reason for hiding this comment

mkonicek commented Mar 15, 2016

skevy commented Mar 15, 2016

sospartan commented Mar 16, 2016

mkonicek commented Mar 16, 2016

nicklockwood commented Mar 16, 2016

skevy commented Mar 16, 2016

skevy commented Mar 16, 2016