kfiroo / react-native-cached-image

CachedImage component for react-native
MIT License
940 stars 468 forks source link

Preload urls the old way? #82

Closed chrusart closed 7 years ago

chrusart commented 7 years ago

Hi,

in 1.4.0 there is a need to use <ImageCacheProvider> component to preload images. I download image urls not in UI and then use it in many places of the app. I can put <ImageCacheProvider> somewhere or everywhere but it was more convenient to invoke preloads just after receiving urls from backend. Static method was really convenient to use separately from UI.

Maybe expose preloadImages(...) from ImageCachePreloader.

kfiroo commented 7 years ago

@chrusart Hey, You can always use ImageCacheManager.downloadAndCacheUrl to add your urls to the cache. You might need to copy/extract some logic for handling large amount of urls.

I still strongly recommend that you use the new ImageCacheProvider though :)

ovaris commented 7 years ago

We also were using ImageCacheProvider.cacheMultipleImages(imageUris); to start image downloading as soon as possible when app starts, but I don't see how it's done easily with new HOC ImageCacheProvider?

ovaris commented 7 years ago

Here's how I did it now:

import { ImageCacheManager } from 'react-native-cached-image';
import ImageCachePreloader from 'react-native-cached-image/ImageCachePreloader';
const imageCacheManager = ImageCacheManager();

export const cacheMultipleImages = urls => {
  if (!urls || urls.length === 0) {
    return Promise.resolve();
  }
  return ImageCachePreloader.preloadImages(urls, imageCacheManager, 20);
};
kfiroo commented 7 years ago

@ovaris Do you feel a downgrade in performance when using the ImageCacheProvider as on of your root level components vs. using the Preloader when the app start?

From my testing, there wasn't any notable delay.

Using ImageCacheProvider is simple some sugar around handling the prefetch process manually and better customizing the config between the CachedImage component and the underlaying cacheing mechanism.

ovaris commented 7 years ago

I haven't notice any @kfiroo

kfiroo commented 7 years ago

@ovaris Then why not use the new way? I find it more convenient

ovaris commented 7 years ago

Because the way we have built our app, it's easier to do preloading in redux actions when fetching the data which contains all the img urls, vs. doing it in container components. It would require us to have all the img url data available in state, plus not all components that require cached images are mounted when app starts, but we still want imgs to be preloaded before mounting the components. Does that make any sense? :)

kfiroo commented 7 years ago

@ovaris Haha of course that makes sense :) I'm just trying to figure out the best and most convenient way to use this without simply adding new duplicate entry points. Thanks for your help!

First, you don't need all CachedImages to be mounted. You can render an empty view while the urls are being downloaded and the rendered CachedImages will use the ImageCacheProvider configuration to handle url requests. ImageCacheProvider was build to hide the preloading logic from the user, you simply provide a list of urls your app will use and it downloads and caches the new and expired urls while doing nothing for urls that are already in the cache.

You say you are currently "..fetching the data which contains all the img urls..". If you add a selector that would provide this list to the ImageCacheProvider you are done, and you can also delete a bunch of code :)

Note that the ImageCacheProvider should be placed as a top level component in your app (just like StyleProvider or any NavigationProvider component).

Again, I'm not against the way it was before or the way you're doing it right now :) Just trying to improve!

chrusart commented 7 years ago

Ok, we are not against anything either :)

Just doing background stuff which is pre-caching data (images here) is not something which should be connected to UI in any way I think, just to separate this is a common sense for me. I understand the idea to 'configure' cache provider for all cached image components that are descendants of it, but it was very clean to pre-download stuff and when some cached image somewhere will use already cached image it's really cool separation data from ui. What I don't like here is putting something in the top component to do some stuff for other components somewhere in the app, context of it is unknown I must say.

The idea of one shared default ImageCacheProvider is really great, then u configure this shared instance for all cached images depending on your needs and all cached images use it by default. If u want something specific then u can use configured ImageCacheProvider as a parent component or prop. But what ImageCachePreloader does is clearly not ui stuff and could/should be exposed as well, it will not break anything, just give access to the same functionality ui uses and more elastic usage of the library if it's needed.

kfiroo commented 7 years ago

@chrusart Thanks, I see where you are coming from :) So if we simply expose ImageCachePreloader it would cover your needs?

chrusart commented 7 years ago

Not only mine ;)

Could you clear up where I'm coming from? :)

kfiroo commented 7 years ago

@chrusart I meant I understand the motivation behind "preloading urls the old way".. Not where you're from geographically.. :)

kfiroo commented 7 years ago

@ovaris @chrusart Any of you want to create a quick PR for exposing the ImageCachePreloader?

chrusart commented 7 years ago

Hehe, i was rather thinking about technology roots or something like that :)

@ovaris did something that works, if he will not do PR till tomorrow, I will look at it.

ovaris commented 7 years ago

Not quite sure what you expect from my PR, but here it is, simply exporting ImageCachePreloader in index.js file. https://github.com/kfiroo/react-native-cached-image/pull/89

kfiroo commented 7 years ago

@ovaris Great! That's exactly what I needed :) I'm currently traveling so it's easier for me to take PRs then do it myself.. Thanks! :)

mik-rom commented 6 years ago

You can always use ImageCacheManager.downloadAndCacheUrl to add your urls to the cache. You might need to copy/extract some logic for handling large amount of urls.

I still strongly recommend that you use the new ImageCacheProvider though :)

@kfiroo The problem with the new changes is... I could never get CachedImage to work inside map markers. That's why I preloaded using ImageCacheProvider.getCachedImagePath and added the cached url to a regular < Image > component when it's ready.

Now after the upgrading to 1.4.3 I'm trying to replace it with ImageCacheManager.downloadAndCache but as I posted here, it gives an error.

I have tried importing the module manually and through npm install, both gives the error. Our app is broken, I would like some answer please on how to fix this. We're about to release and using the new ImageCacheProvider would be too much to refactor. Of course I could downgrade but then we'll have a bug with images loading forever in lists, so we need the latest version of this component but we need to use the old way of preloading, renaming the function used for that is fine if only it would work.