Allow changing the cache root directory #38

sladkoff · 2017-05-09T15:55:15Z

This PR addresses what I already mentioned in #36:

Background

Files in the default cache directory may be purged by the Android OS automatically to free up space. While this is fine in most cases, you may want to make sure that the cached images are permanent (when implementing an offline-mode for example).

API changes

This PR introduces a cacheLocation prop in CachedImage and a cacheLocation option in ImageCacheProvider. All utility functions like clearCache now take an optional parameter as well.

Possible values:

ImageCacheProvider.LOCATION.CACHE (='cache') -> fs.dirs.CacheDir
ImageCacheProvider.LOCATION.BUNDLE (='bundle') -> fs.dirs.MainBundleDir

Everything falls back to 'cache' so there should be no breaking changes.

Feedback appreciated

I tested the CachedImage component and some of the caching functions in ImageCacheProvider. I did not get to test all of the utility functions like collectFilesInfo, clearCache, etc.
I verified that the images are stored in the respective directories.
I updated the README.
I only tested on Android

It'd be cool if your could look over the changes and test if you have the time.

Let me know what you think. I can fix stuff if needed.

- Adds cacheLocation (cache|bundle) options to CachedImage and ImageCacheProvider - Uses respective directories for caching

kfiroo · 2017-05-10T15:49:34Z

@sladkoff Thanks! This look awesome! :)
I'd want to take a closer look just to make sure nothing breaks and that iOS works as expected but other then that it looks great!

mattvot · 2017-05-23T16:04:07Z

@sladkoff Hey, looks good. Do you have time to add Document directory support too? Thanks

sladkoff · 2017-05-23T19:56:35Z

@mattvot It would be very easy to add support for the document directory, yes.
I'd like to hear back from @kfiroo before implementing it.

PS. We're currently using my forked version in our app and everything's working out great so far.

kfiroo · 2017-06-05T12:08:38Z

CachedImage.js

@@ -116,7 +118,9 @@ const CachedImage = React.createClass({
    processSource(source) {
        const url = _.get(source, ['uri'], null);
        if (ImageCacheProvider.isCacheable(url)) {
-            const options = _.pick(this.props, ['useQueryParamsInCacheKey', 'cacheGroup']);
+            let options = _.pick(this.props, ['useQueryParamsInCacheKey', 'cacheGroup']);


why the change to let?

No reason, will revert to const. I think I missed that during cleanup.

kfiroo · 2017-06-05T12:09:26Z

@sladkoff Hey! Thanks for this!
So sorry for the delay, haven't been around my computer for a while..

I'd really like to merge this ASAP, this is a great PR :)

kfiroo · 2017-06-05T12:11:13Z

CachedImage.js

@@ -46,15 +46,17 @@ const CachedImage = React.createClass({
            React.PropTypes.bool,
            React.PropTypes.array
        ]).isRequired,
-        resolveHeaders: React.PropTypes.func
+        resolveHeaders: React.PropTypes.func,
+        cacheLocation: React.PropTypes.oneOf(Object.values(ImageCacheProvider.LOCATION)).isRequired


I'd consider making this a string value instead of an enum to allow more flexibility.
We could expose the LOCATION enum with some convenience values, but still allow the user to input what ever he wants
What do you think?

I concur with this. A future change I would like to add is to support falling back to multiple cache locations.

One of our applications would be bundling a set of content to the app on for release. We'd like to directly reference the bundled cached files if avaliable (rather than seeding by duplicating or pulling a fresh cache down).

By accepting cacheLocation as string, this may be one step closer to our requirement.

kfiroo · 2017-06-05T12:59:31Z

@sladkoff I left some comments in the code, basically I'd rather have the cache prop as a string for flexibility
Other then that it looks good to me :)

sladkoff · 2017-06-09T19:54:51Z

@kfiroo Thanks for the feedback. I'm currently on vacation, will fix this up next week B)

The `cacheLocation` option can now be specified as a string. `ImageCacheProvider.LOCATION` contains default / convenience values. Updated readme.

sladkoff · 2017-06-14T23:57:38Z

CachedImage.js

@@ -47,7 +47,7 @@ const CachedImage = React.createClass({
            React.PropTypes.array
        ]).isRequired,
        resolveHeaders: React.PropTypes.func,
-        cacheLocation: React.PropTypes.oneOf(Object.values(ImageCacheProvider.LOCATION)).isRequired
+        cacheLocation: React.PropTypes.string


Made cacheLocation path fully customizable via string option.

sladkoff · 2017-06-15T00:01:09Z

ImageCacheProvider.js

+                    .then(() => fs.exists(dirPath).then(exists => {
+                        // Check if dir has indeed been created because
+                        // there's no exception on incorrect user-defined paths (?)...
+                        if (!exists) throw new Error('Invalid cacheLocation');


Had to add this workaround in order to be able to properly throw an exception when a user-defined cacheLocation path could not be created. Let me know if there's a better way.

sladkoff · 2017-06-15T00:04:10Z

ImageCacheProvider.js

-    CACHE: 'cache',
-    BUNDLE: 'bundle'
+    CACHE: fs.dirs.CacheDir + '/imagesCacheDir',
+    BUNDLE: fs.dirs.MainBundleDir + '/imagesCacheDir'
 };


The LOCATION enum still contains some defaults / convenience values.

sladkoff · 2017-06-15T00:04:58Z

@kfiroo The latest version supports custom cacheLocation paths. Let me know what you think!

kfiroo · 2017-07-09T13:11:57Z

@sladkoff Sorry for the delay! I was away for a while.
I see @mattvot created a PR (#52) that includes the functionality you suggested here.
I wanted to get you opinion on this, if you think this PR covers both issues I can merge it alone, otherwise I'll merge both.
What do you think?

sladkoff · 2017-07-09T18:48:39Z

@kfiroo The other PR includes this one, so it should be fine if you only merge #52.

kfiroo · 2017-07-10T14:59:04Z

@sladkoff Cool, thanks.
So I'm closing this one

sladkoff · 2017-07-18T21:09:06Z

@kfiroo Just a proposal - since #52 seems to require some more time, would you consider merging this separately?
I'd like to stop using our fork (which I need to keep up to date separately) as dependency.

kfiroo · 2017-07-19T09:47:55Z

@sladkoff Hey, I agree, I think we should move forward with this PR.
I just have some comments here #38 (comment) and I'd like to go over the changes and see if we can do this a bit better.
I'll try to do it today!

mattvot · 2017-07-19T10:29:57Z

Hey, I'll try and put some time into resolving the issues with #52 today and tomorrow. Hopefully you won't be waiting much longer @sladkoff 😀

sladkoff · 2017-07-19T10:35:02Z

@kfiroo Cool. Just let me know what you have in mind and I'll get on it.
I went for a straight-forward approach, I think. There's always room for improvements.

@mattvot Didn't mean to rush you :)

sladkoff added 2 commits May 8, 2017 21:48

Add 'cacheLocation' option

7b1e07a

- Adds cacheLocation (cache|bundle) options to CachedImage and ImageCacheProvider - Uses respective directories for caching

Update readme

a4fb10d

kfiroo reviewed Jun 5, 2017

View reviewed changes

Made cacheLocation fully customizable

cfc8e60

The `cacheLocation` option can now be specified as a string. `ImageCacheProvider.LOCATION` contains default / convenience values. Updated readme.

sladkoff commented Jun 14, 2017

View reviewed changes

sladkoff commented Jun 15, 2017

View reviewed changes

mattvot mentioned this pull request Jun 21, 2017

Add read-only directory fallbacks #52

Closed

kfiroo closed this Jul 10, 2017

kfiroo reopened this Jul 19, 2017

Merge branch 'master' into master

50fbb5b

kfiroo merged commit 6aac317 into kfiroo:master Jul 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow changing the cache root directory #38

Allow changing the cache root directory #38

sladkoff commented May 9, 2017

kfiroo commented May 10, 2017

mattvot commented May 23, 2017

sladkoff commented May 23, 2017

kfiroo Jun 5, 2017

sladkoff Jun 9, 2017

kfiroo commented Jun 5, 2017

kfiroo Jun 5, 2017

mattvot Jun 5, 2017

kfiroo commented Jun 5, 2017

sladkoff commented Jun 9, 2017

sladkoff Jun 14, 2017

sladkoff Jun 15, 2017

sladkoff Jun 15, 2017

sladkoff commented Jun 15, 2017

kfiroo commented Jul 9, 2017

sladkoff commented Jul 9, 2017

kfiroo commented Jul 10, 2017

sladkoff commented Jul 18, 2017

kfiroo commented Jul 19, 2017

mattvot commented Jul 19, 2017

sladkoff commented Jul 19, 2017

Allow changing the cache root directory #38

Allow changing the cache root directory #38

Conversation

sladkoff commented May 9, 2017

Background

API changes

Feedback appreciated

kfiroo commented May 10, 2017

mattvot commented May 23, 2017

sladkoff commented May 23, 2017

kfiroo Jun 5, 2017

Choose a reason for hiding this comment

sladkoff Jun 9, 2017

Choose a reason for hiding this comment

kfiroo commented Jun 5, 2017

kfiroo Jun 5, 2017

Choose a reason for hiding this comment

mattvot Jun 5, 2017

Choose a reason for hiding this comment

kfiroo commented Jun 5, 2017

sladkoff commented Jun 9, 2017

sladkoff Jun 14, 2017

Choose a reason for hiding this comment

sladkoff Jun 15, 2017

Choose a reason for hiding this comment

sladkoff Jun 15, 2017

Choose a reason for hiding this comment

sladkoff commented Jun 15, 2017

kfiroo commented Jul 9, 2017

sladkoff commented Jul 9, 2017

kfiroo commented Jul 10, 2017

sladkoff commented Jul 18, 2017

kfiroo commented Jul 19, 2017

mattvot commented Jul 19, 2017

sladkoff commented Jul 19, 2017