Major fixes #59

iegik · 2017-07-18T10:08:32Z

Fixes 304 Not modified and Cache-Control #56
Breaks Feature: Resolve headers #16 (Discussing in What is resolveHeaders property? #58)
Fixes Is there any way to set expiration policy for cache? #40
Fixes TypeError: RNFatchBlob.config(...).fetch(...).then(...).catch(...).finally is not a function #33

kfiroo · 2017-07-18T10:47:50Z

CachedImage.js

    },

    getDefaultProps() {
        return {
-            renderImage: props => (<Image ref={CACHED_IMAGE_REF} {...props}/>),
+            renderImage: props => (<ImageBackground ref={CACHED_IMAGE_REF} {...props}/>),


Why do you prefer ImageBackground over Image?

React recommends to use ImageBackground.

To confirm, using ImageBackground would require implementors of react-native-cached-image to update to 0.46? If so and @kfiroo is happy with this, this should probably be denoted as a peerDependency in the package.json.

Added alternative in "progressive enhancement" style for ImageBackground facebook/react-native@9637dd4

kfiroo · 2017-07-18T10:51:01Z

CachedImage.js

@@ -211,6 +219,16 @@ const CachedImage = React.createClass({
                      style={activityIndicatorStyle}/>
            )
        });
+    },
+
+    renderError() {


We would probably want to give the users a way to render their own errors.
What do you think? A prop for renderError?

kfiroo · 2017-07-18T10:58:58Z

@iegik WOW! Thanks for all the good things! I really appreciate your help! :)

I absolutely loved the use MemoryCache to manage url expiry! Great Job!

Any chance you could update the README to reflect your changes?
I think its an amazing improvement to the previous version, thanks again.

As a side note, this would have been a lot better as separate PRs with a descriptive title so we could reason about it better :)

iegik · 2017-07-18T11:05:47Z

MemoryCache - is not the best solution. It uses AsyncStorage for saving relative data (we could save some info about image).

We can cache images as Blobs in AsyncStorage, to reduce calls to fs.
Save expiration date somewhere else?
Save expiration date in memory on fly while application runs, but in that case wi will not clean our cache after app relaunch.

iegik · 2017-07-18T11:06:33Z

It`s hard for me split job. I just learning to do that.

mattvot

Thanks for the work @iegik. Just a few things I've seen to be looked at but this is much appreciated!

mattvot · 2017-07-18T12:04:35Z

ImageCacheProvider.js

+                //     'Response': {
+                //         Raw: toFile
+                //     }
+                // });


Is this commented code required, and could it be removed before merging?

mattvot · 2017-07-18T12:05:48Z

ImageCacheProvider.js

+                //         Error: err
+                //     }
+                // });
+            })


Same with this catch that will swallow all errors, unless that is the intention?

I agree, looks like the comment was supposed to include the catch clause?

mattvot · 2017-07-18T12:11:44Z

package.json

    "crypto-js": "3.1.9-1",
    "lodash": "4.17.4",
+    "react-native-clcasher": "git+ssh://git@bitbucket.org/clcorpdevelopment/react-native-casher.git#aa8ead1",


This should be a version number to the npm package.

@kfiroo I'm weary of this dependency addition.

It's a private repo

The NPM package has no readme https://www.npmjs.com/package/react-native-clcasher

It would be good if the required caching functions are included in this library.

I added following documentation. Will be available in next release.
Currently, we do not expected, that package will available as private. We will fix this later. It`s free.

Also, I think, we should provide flexible caching provider.

MemoryCache

Extended AsyncStorage with expiration check

AsyncStorage can only save data forever. If you want save data for some period of time and clean outdated data -
use following API:

set(key: string, value?: mixed, expires?: seconds) - Stores data by key and expiration time in seconds

get(key: string) - Returns stored data by key

remove(key: string) - Clear data by key

multiGet(keys: array) - Get data by keys

multiSet(values: object, expires?: seconds) - Store multiple data with expiration time in seconds

multiRemove(keys: array) - Clears storage by specified keys

flush() - Clear storage

isExpired(key: string) - Checks of data expiration

getAllKeys() - Returns all stored keys

getAllValues() - Returns all stored serialized values

Installation

npm install --save react-native-clcasher

Usage

const MemoryCache = require('react-native-clcasher/MemoryCache').default; MemoryCache.set(url, headers, maxAge)

mattvot · 2017-07-18T12:13:02Z

ImageCacheProvider.js

@@ -1,7 +1,8 @@
 'use strict';

 const _ = require('lodash');
-
+// const debug = require('console-network');


I use my own console-network tool for debugging Image HTTP request (print in console).
There is delemma:

RNFetchBlob.fetch() .then(/* Here we can get HTTP response*/) .catch(/* Here we can get fetch error */) .then(/* finally */) // ... .catch(/* Here we can get file move error */)

How we can provide normal interface for catching fs / fetch errors?

iegik · 2017-07-18T16:48:37Z

Tested on RN v0.39.2

kfiroo · 2017-07-19T09:18:16Z

@mattvot Thanks for the review! :)
@iegik Don't worry about splitting the PR, it was just a friendly advice for the future :)

In response to #59 (comment) :

Why don't you like AsyncStorage for keeping images metadata (such as ts, ttl, size, hash? I don't see any other valid option. Keeping Image data only in memory would mean that you can't tell if it is expired, or how long it's in the cache
Saving images as blobs in AsyncStorage doesn't sound like a solid plan to me, I'm afraid I'm not sure about the limits of such thing. (Is there a limit on a single entry? Is there a size limit to the storage? etc.) + it seems like the fs calls are fast enough (right?)
It feels like all metadata about a url should be kept in the AsyncStorage

Also, if I got that right, react-native-clcasher is your repo? If so, you could publish it and change the version number to the npm one. (I'm happy to help if you need anything)

So, to conclude, I think keeping URL Metadata in some sort of persistent storage (e.g. AsyncStorage or MemoryCache) is a great addition to this lib, but I'd like to think about the API some more and maybe add some tests as the logic seems to be getting more complex.

iegik · 2017-07-19T09:55:33Z

@kfiroo I think is too much requests to fs (AsyncStorage, saving to TMP, moving), but it`s seems to be ok.
Here, I save headers to AsyncStorage. https://github.com/kfiroo/react-native-cached-image/pull/59/files#diff-684d138fd2e5cef7a2e12df1792166e1R258 TTL (or expiration date) stores automatically by react-native-clcashier. Other info can be included later.

I think we can merge now, or I missed something?

…-image # Conflicts: # CachedImage.js # CachedImageExample/index.js

kfiroo · 2017-07-20T10:50:16Z

CachedImage.js

+            renderImage: props => {
+                return (<Image ref={CACHED_IMAGE_REF} {...props}/>)
+            },
+            renderImageBackground: props => {


@iegik Why do we need another prop for renderImageBackground?
I'd rather have the original renderImage test for ImageBackground and fallback to Image for older versions

renderImageBackground aka ImageBackground is needed only when we have children, otherwise we should use old Image.
children for images we use for placeholders only.

torbjorn-kvist · 2017-09-14T06:00:14Z

Is there any status on this PR to be merged?

kfiroo · 2017-09-14T16:31:50Z

@Dvidan I think all issues were resolved with v1.4.0
Let's try to create new issues with a more specific topic so we can discuss them properly :)

aofeng2009 · 2017-09-27T09:33:56Z

@iegik When I cover install my app, Images gone. see issues #83

iegik · 2017-09-28T11:29:22Z

@aofeng2009 Please, read carefuly https://github.com/kfiroo/react-native-cached-image/releases/tag/v1.4.0

iegik added 4 commits July 14, 2017 14:54

Upgrade to RN 0.46

6da64f0

Add console-network, fix Promise

4c5c8b2

Add preload for specific time

adeee0e

Add preload for specific time

66147b4

iegik force-pushed the master branch from 964b29b to 66147b4 Compare July 18, 2017 10:21

iegik mentioned this pull request Jul 18, 2017

activityIndicator doesn't show on Android #57

Open

kfiroo reviewed Jul 18, 2017

View reviewed changes

Fix source may be string

83286b7

mattvot suggested changes Jul 18, 2017

View reviewed changes

Prefer ImageBackground in RN 0.46<

949f96e

Add _clearDepricated on evry 1000 ms

8d3d00f

iegik mentioned this pull request Jul 19, 2017

Children inside Image will be deprecated #60

Open

Update react-native-clcashier to 1.0.0

674a65f

iegik added 6 commits July 19, 2017 13:00

Clean

f8afff4

Fix FS File exists check

3b0a7d0

Merge branch 'master' into master

8137cff

Add error message

5be0bcd

Merge conflicts

bfe1438

Merge branch 'master' of https://github.com/iegik/react-native-cached…

d9c8f7f

…-image # Conflicts: # CachedImage.js # CachedImageExample/index.js

kfiroo mentioned this pull request Jul 19, 2017

Is there any way to set expiration policy for cache? #40

Closed

kfiroo reviewed Jul 20, 2017

View reviewed changes

kfiroo closed this Sep 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major fixes #59

Major fixes #59

iegik commented Jul 18, 2017

kfiroo Jul 18, 2017

iegik Jul 18, 2017

mattvot Jul 18, 2017

iegik Jul 19, 2017

kfiroo Jul 18, 2017

iegik Jul 18, 2017

kfiroo commented Jul 18, 2017

iegik commented Jul 18, 2017

iegik commented Jul 18, 2017

mattvot left a comment

mattvot Jul 18, 2017

mattvot Jul 18, 2017

kfiroo Jul 19, 2017

mattvot Jul 18, 2017

iegik Jul 18, 2017

mattvot Jul 18, 2017

iegik Jul 18, 2017

iegik commented Jul 18, 2017

kfiroo commented Jul 19, 2017

iegik commented Jul 19, 2017

kfiroo Jul 20, 2017

iegik Jul 20, 2017

torbjorn-kvist commented Sep 14, 2017

kfiroo commented Sep 14, 2017

aofeng2009 commented Sep 27, 2017 •

edited

Loading

iegik commented Sep 28, 2017

Major fixes #59

Major fixes #59

Conversation

iegik commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfiroo commented Jul 18, 2017

iegik commented Jul 18, 2017

iegik commented Jul 18, 2017

mattvot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Also, I think, we should provide flexible caching provider.

MemoryCache

Installation

Usage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iegik commented Jul 18, 2017

kfiroo commented Jul 19, 2017

iegik commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torbjorn-kvist commented Sep 14, 2017

kfiroo commented Sep 14, 2017

aofeng2009 commented Sep 27, 2017 • edited Loading

iegik commented Sep 28, 2017

aofeng2009 commented Sep 27, 2017 •

edited

Loading