jayesbe / react-native-cacheable-image

An Image Component for React Native that will cache itself to disk.
MIT License
304 stars 78 forks source link

Broken cached image #27

Open bitcoinvsalts opened 7 years ago

bitcoinvsalts commented 7 years ago

Any idea why do I get broken cached images sometimes?

broken_image

nbolender commented 7 years ago

Are the images within a loop or map function?

This may be a RN issue: https://github.com/facebook/react-native/issues/9893

bitcoinvsalts commented 7 years ago

I am using a ListView and a renderRow function

nbolender commented 7 years ago

Are you using a unique key prop for the root component in your renderRow function? Just thinking it could be some sort of referencing issue.

Or maybe something corrupted the image when it was being cached.

bitcoinvsalts commented 7 years ago

here is my render function:

enderRow = (data) => {
    console.log("--- _renderRow ---")
    const timeString = moment(data.timestamp).fromNow()
    return (
      <Post
        postTitle={data.title}
        posterName={data.username}
        postTime={timeString}
        postContent={data.text}
        imagePath={data.image}
        imageWidth={data.imageWidth}
        imageHeight={data.imageHeight}
      />
    )
  }

and

<CacheableImage
          source={{ uri:this.props.imagePath }}
          resizeMode='contain'
          style={{
            height: height,
            width: screenWidth,
            alignSelf: 'center',
            marginBottom: 10,
          }}
        />
jayesbe commented 7 years ago

@jsappme I think your component needs a <Post key={{'#ID'}}> prop.

bitcoinvsalts commented 7 years ago

it still breaks some of the images sometimes

jayesbe commented 7 years ago

@jsappme Do any of the images break without CacheableImage ? meaning using regular Image ?

bitcoinvsalts commented 7 years ago

no broken image with regular Image component

On Thu, Nov 24, 2016 at 2:16 PM, Jesse notifications@github.com wrote:

@jsappme https://github.com/jsappme Do any of the images break without CacheableImage ? meaning using regular Image ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jayesbe/react-native-cacheable-image/issues/27#issuecomment-262840003, or mute the thread https://github.com/notifications/unsubscribe-auth/AM0R9TZqZf5CkH98ZbY2rEep2Bd9mW5sks5rBfCcgaJpZM4K4fqf .

-- Hervé Fulchiron JSapp.me

jayesbe commented 7 years ago

Hmm.. too many files maybe being written to storage at once ? maybe they need to be queued up for download.

bitcoinvsalts commented 7 years ago

yes I was actually thinking about this probable cause.

Any simple way to queue up those downloads?

bitcoinvsalts commented 7 years ago

I might have find a solution to download the images one by one for my case. I will let you know if it works

bitcoinvsalts commented 7 years ago

I am still struggling with this issue. is there a way to know how much of the image is downloaded during the loading?

bitcoinvsalts commented 7 years ago

I had to go back to the RN Image component for now :-(

jayesbe commented 7 years ago

@jsappme I think it could be that the image is destroyed even before it's had a chance to finish downloading. I believe if that occurs the cached file just needs to be removed since it was incomplete. I will create a patch can you test it?

jayesbe commented 7 years ago

@jsappme can you try the following please


diff --git a/image.js b/image.js
index c53e479..4b13ff3 100644
--- a/image.js
+++ b/image.js
@@ -18,11 +18,12 @@
         this.state = {
             isRemote: false,
             cachedImagePath: null,
+            downloadingImagePath: null,
             downloading: false,
             cacheable: true,
             jobId: null,
             networkAvailable: false
-        };
+        };  
     };

     componentWillReceiveProps(nextProps) {
@@ -89,23 +90,25 @@
                     begin: this.imageDownloadBegin,
                     progress: this.imageDownloadProgress
                 };
-
+                
+                this.setState({downloadingImagePath: filePath});
+                
                 // directory exists.. begin download
                 RNFS
                 .downloadFile(downloadOptions)
                 .promise
                 .then(() => {
-                    this.setState({cacheable: true, cachedImagePath: filePath});
+                    this.setState({cacheable: true, cachedImagePath: filePath, downloading: false, jobId: null, downloadingImagePath: null});
                 })
                 .catch((err) => {
                     // error occurred while downloading or download stopped.. remove file if created
                     this._deleteFilePath(filePath);
-                    this.setState({cacheable: false, cachedImagePath: null});
+                    this.setState({cacheable: false, cachedImagePath: null, downloading: false, jobId: null, downloadingImagePath: null});
                 });
             })
             .catch((err) => {
                 this._deleteFilePath(filePath);
-                this.setState({cacheable: false, cachedImagePath: null});
+                this.setState({cacheable: false, cachedImagePath: null, downloading: false, jobId: null, downloadingImagePath: null});
             })
         });
     }
@@ -156,19 +159,20 @@

     componentWillMount() {
         NetInfo.isConnected.addEventListener('change', this._handleConnectivityChange);
-        // initial
-        NetInfo.isConnected.fetch().then(isConnected => {
-            this.setState({networkAvailable: isConnected});
-       });
+        
+        if (this.props.checkNetwork) {
+            NetInfo.isConnected.fetch().done(this._handleConnectivityChange);
+        }

         this._processSource(this.props.source);
     }
-
+    
     componentWillUnmount() {
         NetInfo.isConnected.removeEventListener('change', this._handleConnectivityChange);

         if (this.state.downloading && this.state.jobId) {
             RNFS.stopDownload(this.state.jobId);
+            this._deleteFilePath(this.state.downloadingImagePath);
         }
     }
bitcoinvsalts commented 7 years ago

Hi Jesse can you send me the new file to herve76 @ gmail.com?

jayesbe commented 7 years ago

@jsappme if you can test the latest version.

wcandillon commented 7 years ago

@jsappme I implemented a cache system similar to this module using react native fetch blob to download/save the images and I'm getting the exact same issue. Not sure why.

I'm also wondering if Image.prefetch could be used to do the job? And if yes how? Or I'm misunderstanding the semantic of Image.prefetch completely?

MossP commented 7 years ago

I think you're right @jayesbe. In my tests with another component it seems to occur if I have the image inside a cell of a Flatlist which destroys the cells as they move outside of the render window. I'll try this component with that fix you've mentioned to see if it fixes the issue.

MossP commented 7 years ago

~I very quickly ran into this error using this component. Not sure if it's related.~ May be my error

React Native : 0.45.1 Component Version : 1.5.1

Used inside a flatlist and scrolling quickly. If I throttle the connection then many images (~60%) never show even though I can see through the proxy that they have finished.

image uploaded from ios 1

MossP commented 7 years ago

That previous error may have been due to some remaining elements of the last cache component clashing with this one. It seems to be much more stable now but I still get the frequent missed images as mentioned above.

I'd hazard a guess that the missing image are the ones that would have been incomplete, so now it's not showing them at all instead of showing half an image.

jayesbe commented 7 years ago

@MossP have you tried the dev branch ?

The component has to be restructured so that data logic can be extracted into its own module that facilitates loading and cache file management. I haven't been able to get back to updating this component in some time.

MossP commented 7 years ago

Ah, no, I'm back on another component now but I'll try to take a look soon. I just thought I would test as @jsappme had gone quiet.

MossP commented 7 years ago

@jayesbe I just tried on the Dev branch. I get about 60% failure rate still when throttling the connection. I can see all images requests are complete via proxy but complete image was not received. App still shows activity indicator.

Even if I disable throttling and force close/restart App, some images do not recover although the requests to them are complete via the proxy and I can see the full image has come through.

DavitVosk commented 6 years ago

The same problem here. Could anyone fix this bug?

jayesbe commented 6 years ago

Sorry guys.. I haven't been able to maintain this package as much as I need to.

I believe for the most part this package is now obsolete though may be worthwhile as a convenience.

RN now provides an ImageStore API that can be used to cache images. Adding this into the package would allow the ImageStore to maintain the cache rather than using react-native-fs. RNFS would still be used to download the file and convert to base64 encoded which would be required for ImageStore.

This requires dev.. react-native-cacheable-image doesn't work well with list components.. this is a known issue.. especially lists with a lot of elements.