kfiroo / react-native-cached-image

CachedImage component for react-native
MIT License
938 stars 470 forks source link

Fixed images gone when fresh install #91

Closed aofeng2009 closed 6 years ago

aofeng2009 commented 6 years ago

When we freshed install app on iOS, images which with remote URLs will disappear. More infomation please see https://github.com/kfiroo/react-native-cached-image/issues/83 .

kfiroo commented 6 years ago

@aofeng2009 Thanks for this PR! I'll try to explain what's going on after you upgrade to the latest version, and maybe that will help us understand what's causing the bug.

User installs your app using v1.3.x

User upgrades to the version of your app that uses v1.4.x

The above is the expected behaviour. Can you pin point where things go wrong please?

Thank you so much for your help :)

aofeng2009 commented 6 years ago

It’s OK. “It resembles the case where the OS decided to change “cacheLocation” (as it may according to the documentation)” maybe better. Thank you.

kfiroo commented 6 years ago

@aofeng2009 Here are the references to the docs, just in case:

From the Android docs

When the device is low on internal storage space, Android may delete these cache files to recover space.

From the Apple docs

Note that the system may delete the Caches/ directory to free up disk space, so your app must be able to re-create or download these files as needed.

But I still don't understand why so many people are reporting broken/blank images :(

augustbjornberg commented 6 years ago

I'm running 1.4 and still have the blank image bug. Is there a solution that i've missed?

Steps to reproduce

  1. Install app on real device or simulator, images will load and display just fine.
  2. Update app or install app a second time on device or simulator, every cached image is blank.

I'm happy to share some code if that is helpful in any way.

kfiroo commented 6 years ago

@augustbjornberg Thanks for the info! I don't think there is a fix currently, I will try to reproduce it soon and get back to you.

aofeng2009 commented 6 years ago

@kfiroo @augustbjornberg. Well,I think this bug is not caused by low memory or storage. Maybe i do not explain clearly. In your last version the memory cache feature used an absolutely path as a key in local storage. So that the image can be found by the key. If we do not over install everything is ok.But when we fresh install app in the iOS system,the sandbox path will change by os.So the key in local storage is not available. My PR change the key to a relative path which appended the latest sandbox path as image path so that can load the image. I make a test on my app and everything is ok. Sorry for my poor English. @augustbjornberg @kfiroo you can visit my fork https://github.com/aofeng2009/react-native-cached-image make a test. Any suggestion will be appreciated.

augustbjornberg commented 6 years ago

I tried your fork @aofeng2009, it appears to be working, great stuff.

My only concern is that i had to delete the app and reinstall for the update to start working. I'm by no means an iOS development expert, how would that affect an app update that is pushed to the app store? By my logic the fix wouldn't work unless i also initially wiped/deleted the previous cached images when running my pushed app for the first time.

What are your thoughts?

aofeng2009 commented 6 years ago

@augustbjornberg I don't know if I understand . As a library who used it for the first time maybe this is well.I consider your condition seriously and make a new version in my fork. I do not delete the cached data. I redirect the unavailable image path which store in AsyncStorage to new sandbox path. you can make a test. You are right that deleted the previous cached data can fixed the bug, but It's a little bit heavy. It's just personal opinion, for your reference only.

kfiroo commented 6 years ago

@aofeng2009 @augustbjornberg Thanks for the info guys! And sorry for the late response.

The point was that the upgrade would be seamless, that is, you would not have to clear the cache in order for things to work (or do any other thing for that matter - other than adjusting your code to use the new ImageCacheProvider component).

Here is what I did to try and reproduce the issue:

  1. Completely removed the example app from my iOS simulator
  2. git checkout v1.3 && yarn to get a fresh version of the app with v1.3.5 of RNCI on it
  3. 'react-native run-ios` to get the app installed on my simulator, make sure that everything works well and the cache-info shows the correct info
  4. Close the app and the packager to make sure there are no leftovers
  5. git checkout master && yarn to get the latest version of the example app with v1.4.1 on it
  6. react-native run-ios to upgrade the example app version to the latest

Now, all the images are loaded, no blank images, and cache-info shows the correct info.

Can you guys tell me where things go wrong for you?

Thanks a lot for your patience and your help in figuring this out :)

augustbjornberg commented 6 years ago

@kfiroo, i tried your way of reproducing the issue, and i still have the same problem. 1.3.5 isn't even compatible with the latest react native. For a second i thought this bug might have to do with the ImageCachePreloader, but the bug is present regardless if i use it or not.

The things that go wrong are just the same as my response 6 days ago:

  1. Install an app that uses v1.4.1 more than once on the same device
  2. All images using the caching are blank, regardless of updated app build number or version

I can upload a screen recording showing how it breaks (if it's helpful in any way). Until then, I will stick with @aofeng2009 's version for now, it seems to be working.

kozillla commented 6 years ago

Hi guys,

For me the easiest way to reproduce issue (on iOS) is to change my app version from XCode. Example :

  1. Images are shown correctly (using v1.4.1)
  2. Change app version from XCode (General->Identity->Version) , to the new value
  3. Recompile project
  4. All cashed images are not shown anymore
kfiroo commented 6 years ago

@augustbjornberg @kozillla Thanks for the info, it's really helpful! I'll try to figure it out and update here

The fact that the cache breaks when you only change the app version is a huge help! Thanks!

aofeng2009 commented 6 years ago

@kfiroo Please see my test with screenshot and how it produced the issue. For me、 my workmate who use this library on iOS it is a obviously problem. I make a simple test which only have one image.

First, Launched app.

2017-10-14 9 44 06

Second, Rebuild and run ios app

image gone. Just easy, not related with the version.

2017-10-14 9 44 40

Have you see the problem ? The folder name changed by OS.

My opinion

At last version which has memory feature have used 'AsyncStorage' to cache image path and expire time. We can find it at bellow.

Open it where "/Documents/RCTAsyncLocalStorage_V1/manifest.json". Everything is clearly. Still used the unavailable path.

2017-10-14 9 49 51

@kozillla @augustbjornberg Have you tried my version and if it can fix your issue?

I will not explain once more. Please see above.I hope it can help person who use this library as soon as possible, because as least 25 days has passed from my issue post here.

kozillla commented 6 years ago

@aofeng2009 I've just tested your version. It works ok for me . Thanks for fix :)

chrusart commented 6 years ago

I did similar thing as @aofeng2009 , it resolves changing the application id when new container is created. Just using relative path kept in url cache.

but... the simple principle of cache is that if there is no file at specified path (error when fetching), url to path mapping should be removed for this url and downloading should be invoked again with saving new path in url cache. It will work with whatever path, relative or absolute.

The thing is that we store in CachedImage state path that doesn't exists, Image component tries to load it and fails. If we check if file exists at path returned from url cache and throw if not then in catch (CachedImageManager:35) everything will be downloaded again and stored. This must be done before we pass path to CachedImage cause it's a cache part problem not managing correctly its data.

Check a fork, i didn't want to do a second PR, just update this one maybe: https://github.com/flatfox-ag/react-native-cached-image/tree/relative_path

So:

I guess, both things doesn't impact performance as well.

PS. removing old path is missing there if we stay with absolutes paths (we shouldn't), not needed if we use relative, but some cleaning should be done regarding every entry ttl anyway, right? Ohh, it's not needed as new path will overwrite old entry in MemoryCache for the same url.

chrusart commented 6 years ago

@kfiroo , this also happen when u just click Run again in XCode, new container is created for app with the same cache, but app id is different. Everything is about path in url cache.

chrusart commented 6 years ago

@augustbjornberg, for cases like yours that aofeng2009 still not works, check for file existence described in my previous post should work.

edi commented 6 years ago

OK. Same issue where. Simplest way to reproduce this, without even opening XCode, is to open the terminal, run-ios, cache an image, close everything, then run-ios again.

Once that packager is restarted, a new AppID is generated. Old image still exists ( cached in the old folder ), and the manifest.json from the cache manager has the absolute path, therefore thinking it exists, but the new URL created contains the new AppID, hence the blank images.

I will try the solution proposed by @aofeng2009 and see where it gets me :)

chrusart commented 6 years ago

It is, if you look how it keeps mapping from url to path, u will see that it keeps absolute paths, and same issue is that changing application id from whatever reason, it will give old absolute path with old application id and will not work. Application id can change for many reasons, running from XCode, run-ios and probably updating the app, the same issue i mean that the problem lies in the same place, caching absolute paths and not re-downloading where this path is not valid anymore.

not sure, what urls u have in manifest.json and where are these new URLs created with correct appId but url cache keeps old ones and they are returned to Image component in current version of library.

And @aofeng2009 solution will work (didn't check myself, but relative path idea is correct), just missing checking if file under path in url cache exists. This will for sure resolve problems when there is no such file, even if url cache have correct path to file (appID didn't change), system can delete Caches folder and then url cache still have path for this url (valid ttl) and will return it. But file is not there anymore, this check is missing (#86 for example).

chrusart commented 6 years ago

Ok, created PR #97 just to show what I mean and easy compare.

aofeng2009 commented 6 years ago

@chrusart Good job.
Checking if file exists is thoughtful. Hard coding of "imagesCacheDir" is not recommended. At that time,I only want to resolve augustbjornberg's problem without thought too much. I will close this PR. Thank you. Maybe kfiroo is too busy and have no time to deal with this. So please wait.

chrusart commented 6 years ago

TY @aofeng2009 , I didn't want to replace your PR cause you did it first, I thought you will just update yours, but if you closed this one then @augustbjornberg @kozillla , could you check PR #97 then if it works for your issues?