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

Allow prefetch remote image #4420

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@sospartan
Contributor

sospartan commented Nov 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 30, 2015

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

facebook-github-bot commented Nov 30, 2015

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

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Nov 30, 2015

Contributor

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.

Contributor

nicklockwood commented Nov 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Nov 30, 2015

Contributor

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:

  1. The ImageLoader already owns the image cache, so it makes sense implementation-wise
  2. 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:

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

I think I need to discuss this with @dmmiller

Contributor

nicklockwood commented Nov 30, 2015

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:

  1. The ImageLoader already owns the image cache, so it makes sense implementation-wise
  2. 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:

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

I think I need to discuss this with @dmmiller

@sospartan

This comment has been minimized.

Show comment
Hide comment
@sospartan

sospartan Nov 30, 2015

Contributor

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 😄 ).

Contributor

sospartan commented Nov 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 30, 2015

@sospartan updated the pull request.

facebook-github-bot commented Nov 30, 2015

@sospartan updated the pull request.

@sospartan sospartan changed the title from add Networking resource moudle to add Networking resource module Dec 1, 2015

@sospartan sospartan changed the title from add Networking resource module to add Networking resource prefetch Dec 1, 2015

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 2, 2015

@sospartan updated the pull request.

facebook-github-bot commented Dec 2, 2015

@sospartan updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 2, 2015

@sospartan updated the pull request.

facebook-github-bot commented Dec 2, 2015

@sospartan updated the pull request.

@sospartan

This comment has been minimized.

Show comment
Hide comment
@sospartan

sospartan Dec 2, 2015

Contributor

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

Contributor

sospartan commented Dec 2, 2015

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

@sospartan

This comment has been minimized.

Show comment
Hide comment
@sospartan

sospartan Dec 3, 2015

Contributor

@mkonicek thanks for the suggestions.

Contributor

sospartan commented Dec 3, 2015

@mkonicek thanks for the suggestions.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 3, 2015

@sospartan updated the pull request.

facebook-github-bot commented Dec 3, 2015

@sospartan updated the pull request.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Dec 3, 2015

Collaborator

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

Collaborator

brentvatne commented Dec 3, 2015

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

@sospartan

This comment has been minimized.

Show comment
Hide comment
@sospartan

sospartan Dec 3, 2015

Contributor

@brentvatne Glad to contribute to this project .

Contributor

sospartan commented Dec 3, 2015

@brentvatne Glad to contribute to this project .

@sospartan

This comment has been minimized.

Show comment
Hide comment
@sospartan
Contributor

sospartan commented Dec 6, 2015

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 7, 2015

@sospartan updated the pull request.

facebook-github-bot commented Dec 7, 2015

@sospartan updated the pull request.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Dec 7, 2015

Collaborator

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

Collaborator

brentvatne commented Dec 7, 2015

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

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Dec 9, 2015

Contributor

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?

Contributor

mkonicek commented Dec 9, 2015

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

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Dec 9, 2015

Contributor

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?

Contributor

mkonicek commented Dec 9, 2015

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?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Dec 9, 2015

Collaborator

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?

Collaborator

ide commented Dec 9, 2015

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

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Dec 9, 2015

Contributor

👍 Agree with @ide.

Contributor

mkonicek commented Dec 9, 2015

👍 Agree with @ide.

@sospartan

This comment has been minimized.

Show comment
Hide comment
@sospartan

sospartan Dec 10, 2015

Contributor

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

Contributor

sospartan commented Dec 10, 2015

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

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

@skevy

This comment has been minimized.

Show comment
Hide comment
@skevy

skevy Mar 11, 2016

Collaborator

@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.

Collaborator

skevy commented Mar 11, 2016

@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.

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

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

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Mar 12, 2016

Contributor

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

Contributor

nicklockwood commented Mar 12, 2016

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

pglotov added a commit to pglotov/react-native that referenced this pull request Mar 15, 2016

allow use external typeface in native code
Summary:
Expose method to implement changing font family cache.  Like ide suggested in #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
uri = Uri.parse(url);
}
} catch (Exception e){
//ignore malformed url

This comment has been minimized.

@mkonicek

mkonicek Mar 15, 2016

Contributor

nit: space after //

@mkonicek

mkonicek Mar 15, 2016

Contributor

nit: space after //

//ignore malformed url
}
if (uri == null) {
FLog.w(ReactConstants.TAG,"Invalid prefetch image url.");

This comment has been minimized.

@mkonicek

mkonicek Mar 15, 2016

Contributor

space after ,

Use "Invalid prefetch image url: " + url - helps with debugging.

@mkonicek

mkonicek Mar 15, 2016

Contributor

space after ,

Use "Invalid prefetch image url: " + url - helps with debugging.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 15, 2016

Contributor

See here: exponentjs@d455cb6#diff-f8703d50cf5af4c06824e2928dddb500R504

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

Contributor

mkonicek commented Mar 15, 2016

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

This comment has been minimized.

Show comment
Hide comment
@skevy

skevy Mar 15, 2016

Collaborator

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

Collaborator

skevy commented Mar 15, 2016

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

@sospartan

This comment has been minimized.

Show comment
Hide comment
@sospartan

sospartan Mar 16, 2016

Contributor

@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.

Contributor

sospartan commented Mar 16, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 16, 2016

Contributor

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

Contributor

mkonicek commented Mar 16, 2016

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

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Mar 16, 2016

Contributor

@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).

Contributor

nicklockwood commented Mar 16, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@skevy

skevy Mar 16, 2016

Collaborator

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

Collaborator

skevy commented Mar 16, 2016

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

@skevy skevy closed this Mar 16, 2016

@skevy

This comment has been minimized.

Show comment
Hide comment
@skevy

skevy Mar 16, 2016

Collaborator

@sospartan thanks for the initial work on this!

Collaborator

skevy commented Mar 16, 2016

@sospartan thanks for the initial work on this!

@skevy skevy self-assigned this Mar 16, 2016

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

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

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

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

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

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

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

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

ghost pushed a commit that referenced this pull request Apr 13, 2016

Add a way to prefetch remote images to cache with Image.prefetch
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

rozele referenced this pull request in Microsoft/react-native-windows May 25, 2016

Add a way to prefetch remote images to cache with Image.prefetch
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

zebulgar added a commit to nightingale/react-native that referenced this pull request Jun 18, 2016

Add a way to prefetch remote images to cache with Image.prefetch
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#6774

Differential Revision: D3153729

Pulled By: bestander

fb-gh-sync-id: ef61412e051a49b42ae885edce7905a8ca0da23f
fbshipit-source-id: ef61412e051a49b42ae885edce7905a8ca0da23f

aleclarson added a commit to aleclarson/react-native that referenced this pull request Sep 6, 2016

Add a way to prefetch remote images to cache with Image.prefetch
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#6774

Differential Revision: D3153729

Pulled By: bestander

fb-gh-sync-id: ef61412e051a49b42ae885edce7905a8ca0da23f
fbshipit-source-id: ef61412e051a49b42ae885edce7905a8ca0da23f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment