Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a way to configure destination directory #68

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add a way to configure destination directory #68

wants to merge 14 commits into from

Conversation

froger
Copy link

@froger froger commented May 24, 2018

I have a use case where I need to cache permanently the images.
The application we are on will be used most of the time offline, and should not ever destroy image caches.

I just added in this pull request the ability to edit the BASE_URL configuration outside the lib.

@wcandillon
Copy link
Owner

@froger Can you put these methods part of the CacheManager instead? If you think that makes sense? 🤔

@froger
Copy link
Author

froger commented May 25, 2018

@wcandillon yes sure, when I started I though "it could have more options". But when finished writing, my thoughts were "all this code just to rewrite BASE_URL". I will find something more straight forward and write some unit-tests as well.

But I don't know if this feature is somewhat useful in the scope of the lib. The no-brainer&no-config of the lib is quiet pleasant, so at this end maybe this feature have no places in your lib.

@froger
Copy link
Author

froger commented May 25, 2018

@wcandillon I've updated my pull request, and did some tests on a project of mine.
It hasn't any unit tests for now, but still, a review of this code would be welcome.

What I've done

setBaseDir(baseDir: string): string
Setup the cache directory used by the library. Can be outside the FileSystem.cacheDirectory .

removeCacheEntry(url: string): Promise
Remove a file from cache from it's url.

What's next

Basically, introducing a setBaseDir add various edge-cases that should be considered before merging to the lib:

  • setBaseDir(baseDir: string): string Should validade that the new baseDir is an url. Currently, if the url is invalid, nothing will be cached and no warning, no error will be raised.
  • If you define a baseDir which is not in the cache directory, then the files won't be garbage-collected. It's why I added a removeCacheEntry(url: string): Promise function. But this function provoques more edges-cases (File locking ? File does not exsits?). I am not that much in RN-Expo, so I hope the Filesystem API is robust.

These updates worked good for my project, but again, maybe this lib doesn't need them.

Copy link
Owner

@wcandillon wcandillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Made two minor comments. I will integrate your branch in my own projects in order to do a bit more testing.

@@ -3,7 +3,12 @@ import * as _ from "lodash";
import {FileSystem} from "expo";
import SHA1 from "crypto-js/sha1";

const BASE_DIR = `${FileSystem.cacheDirectory}expo-image-cache/`;
let _baseDir = `${FileSystem.cacheDirectory}expo-image-cache/`;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simplicity, would it be possible to add this method to CacheManager for simplicity?

* As we can now set an uri that is not in the cacheDirectory,
* we need to be able to delete files.
*/
export const removeCacheEntry = async (uri: string): Promise => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should return be await and Promise be Promise<void>?

@froger froger mentioned this pull request Jun 1, 2018
@wcandillon
Copy link
Owner

@froger Could you rebase against the latest version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants