{"action":"created","issue":{"url":"https://api.github.com/repos/jayesbe/react-native-cacheable-image/issues/33","repository_url":"https://api.github.com/repos/jayesbe/react-native-cacheable-image","labels_url":"https://api.github.com/repos/jayesbe/react-native-cacheable-image/issues/33/labels{/name}","comments_url":"https://api.github.com/repos/jayesbe/react-native-cacheable-image/issues/33/comments","events_url":"https://api.github.com/repos/jayesbe/react-native-cacheable-image/issues/33/events","html_url":"https://github.com/jayesbe/react-native-cacheable-image/pull/33","id":195441226,"number":33,"title":"setState warnings, Broken Images, & Race Conditions","user":{"login":"nbolender","id":4635368,"avatar_url":"https://avatars.githubusercontent.com/u/4635368?v=3","gravatar_id":"","url":"https://api.github.com/users/nbolender","html_url":"https://github.com/nbolender","followers_url":"https://api.github.com/users/nbolender/followers","following_url":"https://api.github.com/users/nbolender/following{/other_user}","gists_url":"https://api.github.com/users/nbolender/gists{/gist_id}","starred_url":"https://api.github.com/users/nbolender/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nbolender/subscriptions","organizations_url":"https://api.github.com/users/nbolender/orgs","repos_url":"https://api.github.com/users/nbolender/repos","events_url":"https://api.github.com/users/nbolender/events{/privacy}","received_events_url":"https://api.github.com/users/nbolender/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2016-12-14T05:02:50Z","updated_at":"2016-12-15T13:53:02Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/jayesbe/react-native-cacheable-image/pulls/33","html_url":"https://github.com/jayesbe/react-native-cacheable-image/pull/33","diff_url":"https://github.com/jayesbe/react-native-cacheable-image/pull/33.diff","patch_url":"https://github.com/jayesbe/react-native-cacheable-image/pull/33.patch"},"body":"**This PR potentially resolves:**\r\n* https://github.com/jayesbe/react-native-cacheable-image/issues/22 (setState warning)\r\n* https://github.com/jayesbe/react-native-cacheable-image/issues/27 (Broken cached image)\r\n\r\n**Summary of fixes:**\r\n1. Resolves all instances of `setState` warnings.\r\n2. Solves an issue where an image that is being downloaded in a different component displays as blank space.\r\n3. Solves an issue where downloads were being restarted after every parent update.\r\n4. Fixed a bunch of race conditions related to networking and the delay between javascript & native code.\r\n\r\nThere 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: https://github.com/johanneslumpe/react-native-fs/pull/219\r\n\r\n**Here is a walkthrough of the code changes:**\r\n1. Merged in @jayesbe’s changes in the comments of Issue #22 (setState warning), with some help from @robwalkerco.\r\n2. `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.\r\n3. `_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.\r\n4. 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.\r\n\r\n   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.\r\n\r\n   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.\r\n5. Fixed a typo in `RNFS.mkdir().then()` (`_deleteFilePath()`)\r\n6. Added a check to stop an in-progress download before starting a new one in the same component.\r\n7. `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.\r\n8. 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.\r\n9. `_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`."},"comment":{"url":"https://api.github.com/repos/jayesbe/react-native-cacheable-image/issues/comments/267332206","html_url":"https://github.com/jayesbe/react-native-cacheable-image/pull/33#issuecomment-267332206","issue_url":"https://api.github.com/repos/jayesbe/react-native-cacheable-image/issues/33","id":267332206,"user":{"login":"robwalkerco","id":6524830,"avatar_url":"https://avatars.githubusercontent.com/u/6524830?v=3","gravatar_id":"","url":"https://api.github.com/users/robwalkerco","html_url":"https://github.com/robwalkerco","followers_url":"https://api.github.com/users/robwalkerco/followers","following_url":"https://api.github.com/users/robwalkerco/following{/other_user}","gists_url":"https://api.github.com/users/robwalkerco/gists{/gist_id}","starred_url":"https://api.github.com/users/robwalkerco/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/robwalkerco/subscriptions","organizations_url":"https://api.github.com/users/robwalkerco/orgs","repos_url":"https://api.github.com/users/robwalkerco/repos","events_url":"https://api.github.com/users/robwalkerco/events{/privacy}","received_events_url":"https://api.github.com/users/robwalkerco/received_events","type":"User","site_admin":false},"created_at":"2016-12-15T13:53:02Z","updated_at":"2016-12-15T13:53:02Z","body":"@nbolender I've not managed to figure out the exact cause of warning for me yet, and why it consistently occurs\r\nI have managed to prevent `setState` from giving any warnings by wrapping each `setState` in \r\n```\r\nif (this.mounted) {\r\n    setState...\r\n}\r\n```\r\nand 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)\r\n"}}
{"actor":{"display_login":"robwalkerco"}}