setState warnings, Broken Images, & Race Conditions

[nbolender]

This PR potentially resolves:

• setState warning #22 (setState warning)
• Broken cached image #27 (Broken cached image)

Summary of fixes:

1. Resolves all instances of setState warnings.
2. Solves an issue where an image that is being downloaded in a different component displays as blank space.
3. Solves an issue where downloads were being restarted after every parent update.
4. Fixed a bunch of race conditions related to networking and the delay between javascript & native code.

There is also a possibility that if you render many images that need to be downloaded, and then cause the jobs to be cancelled (for example, by navigating to a different screen), you may get an error from React Native of the form "Callback with id #: undefined.undefined() not found". This is caused by RNFS itself and I have a PR set up here: itinance/react-native-fs#219

Here is a walkthrough of the code changes:

1. Merged in @jayesbe’s changes in the comments of Issue setState warning #22 (setState warning), with some help from @robwalkerco.

2. downloading and jobId are not used in the render functions, and a race condition can cause these to be out of date when using this.setState() because it is asynchronous. These have been moved out of the state object.

3. _processSource() was erroneously triggering a re-download even when the source URI was identical. Any re-renders in the parent component, or change in network state, were causing the download to be restarted. Comparing the URI property (when present) saves us from accidentally downloading the same image twice.

4. I have re-added a file size check in checkImageCache(). This is necessary because on iOS, RNFS creates a zero-byte file when it begins the download. In my application, sometimes a component uses the same URI as another CacheableImage instance. There was a situation where the first component began downloading, and the second one saw the empty file and displayed a blank image. React Native does not update the image when it is changed on disk, so it rendered as transparent.

   The file size check throws an error if there is a zero-byte file, whether it is still downloading or sometime else caused the download operation to not complete. The error starts a new download.

An alternative to this functionality could be to first download the image to a random temporary location until it is completed downloading, but you still might find a race condition while the file is being moved to the final location. Also, that alternative could still result in two downloads of the same image.

   Ideally, RNFS would check to see if it is already downloading a URI, and bind the two jobs into one. But I think this is an edge case overall.

5. Fixed a typo in RNFS.mkdir().then() (_deleteFilePath())

6. Added a check to stop an in-progress download before starting a new one in the same component.

7. RNFS.downloadFile() immediately returns the jobId, so I captured that synchronously. Without this, a race condition exists where the component might unmount before imageDownloadBegin() is called, and thus won’t cancel the download job. This can cause a setState warning, not to mention a waste of bandwidth.

8. Because RNFS throws an error when you call stopDownload(), I added a check in the error handler to avoid a setState warning. (We only call stopDownload() on unmount) It is still necessary to call _deleteFilePath() on an error or cancelled download, because RNFS does not remove a partially-downloaded file when this happens.

9. _handleConnectivityChange() is now unset in componentWillUnmount(), and its status is checked in the initial NetInfo fetch. This solves an issue where a component will quickly mount and then unmount before the initial fetch is resolved. Without this, you may see setState warnings especially if you use local images and accidentally keep props.checkNetwork == true.

[robwalkerco]

@nbolender Nice work. I've started using your this branch in my project, and it is a big improvement, but there still seems to be something causing a setState warning.

I'm not sure how to figure out where the warning is being triggered though. Any tips on tracing the exact bit of code?

[nbolender]

@robwalkerco I believe there is still one possibility for a setState warning in the downloadFile completion handler (downloadFile().then()). This could occur if the download completes some time after the component unmounts, so the completion handler is called.

If you can reproduce it consistently, you might try commenting out the setState statement in the completion handler to see if it is in there.

If that's indeed the issue, it might be best to drop the imageDownloadProgress callback altogether, then perform the same check in the download completion handler as we do in its error handler.

[jayesbe]

@nbolender great work. This is very much appreciated. I will go through this soon and Im sure it will be merged by the end of the week.

[robwalkerco]

@nbolender I've isolated the warning to the setState in RNFS.stat().then() https://github.com/jayesbe/react-native-cacheable-image/pull/33/files#diff-a55ceb1e7b521b129b05093109b0da5bR64

I'll try to figure out more tomorrow, but it seems to happen when you first mount a page containing cachable images, then unmount, then mount the same page again. The images are displayed fine, but the warning occurs.

[robwalkerco]

@nbolender I've not managed to figure out the exact cause of warning for me yet, and why it consistently occurs
I have managed to prevent setState from giving any warnings by wrapping each setState in

if (this.mounted) {
    setState...
}

and using componentWillMount and componentWillUnmount to set the boolean, though I think that's an anti-pattern in this case (https://facebook.github.io/react/blog/2015/12/16/ismounted-antipattern.html)

[onchainguy-btc]

Any news about merging this one?

[robwalkerco]

I've stopped using this package now, as I was still having issues that were only fixed by my hacky work around :(

This PR was an improvement though, so I think it would be helpful if merged.

[jayesbe]

Sorry guys have been a little behind. Please create new issues if / when they come up :)

[onchainguy-btc]

Hm @robwalkerco tested your PR having the same issues as before. Had to switch to https://github.com/kfiroo/react-native-cached-image :-(