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

Image caching improvements #129

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

Conversation

davidminor
Copy link

Some improvements that I suggested in #126 & #127 -

The first commit adds an optional cache key property to Image to use instead of the uri as the cache key.

The second commit avoids repeat downloads if multiple Image components reference the same uri.

Add an option to use a cache key that is different from the image
URI. This is useful for example if the image URI contains a parameter
that changes, but the image doesn't vary as a result, such as an
authentication token.

If the cache key is not present, the image URI will be used as the
cache key.
Create a single promise to handle the downloading of a cache
entry. Otherwise, multiple calls to get the path of a new cache
entry will result in multiple downloads, until one of the downloads
is successful.

If the download is unsuccesful, unset the promise so that the
next request will result in a new attempt to download the image.
@jeffreybello
Copy link

+1 to this.
This feature is sooooo good!

@davidminor
Copy link
Author

@wcandillon any chance of getting this merged soon?

@mo-hummus
Copy link

Hey man you are AMAZING!

@wcandillon
Copy link
Owner

@davidminor Thks for that, can you look into the merge conflicts?

@mo-hummus
Copy link

mo-hummus commented Dec 2, 2019

Before merging this it needs some work though, there is an edge case where if you delete a cached image then change the component's key the promise is never deleted it wont redownload/load the image, i resolved this by adding a check if the path has been resolved before

export class CacheEntry {
  uri;

  options;

  downloadPromise;

  pathResolved = false;

  constructor(uri, options) {
    this.uri = uri;
    this.options = options;
  }

  async getPath() {
    const { uri, options } = this;
    const { path, exists, tmpPath } = await getCacheEntry(uri);

    if (exists) {
      return path;
    }

    if (!this.downloadPromise) {
      this.pathResolved = false;
      this.downloadPromise = this.download(path, tmpPath);
    }

    if(this.downloadPromise && this.pathResolved) {
      this.pathResolved = false;
      this.downloadPromise = this.download(path, tmpPath);
    }

    return this.downloadPromise;
  }


  async download(path, tmpPath){
    const { uri, options } = this;
    const result = await FileSystem.createDownloadResumable(uri, tmpPath, options).downloadAsync();

    // If the image download failed, we don't cache anything
    if (result && result.status !== 200) {
      this.downloadPromise = undefined;
      return undefined;
    }
    await FileSystem.moveAsync({ from: tmpPath, to: path }).then(()=> {
      this.pathResolved = true
    });
    return path;
  }
}

@Aryk
Copy link

Aryk commented May 10, 2020

Just curious if this feature or something similar made its way into the library. I see there's been no activity for 6 months.

@davidminor
Copy link
Author

We've been using a custom build and it fell off my radar. Probably won't have time to pick it back up for awhile.

@Aryk
Copy link

Aryk commented May 11, 2020

Got it, I implemented caching of the signed s3 urls on the server side. I have it set to cache for 5 days whereas the urls are good for 7 days. The mobile app itself reloads data from the server everytime it loads a page so I don't think there is a chance for the url to get stale. Anyways, thanks for your response.

@obax
Copy link

obax commented Dec 6, 2020

Hi guys, we are using a patched version of the library to use the feature. Is there any chance that this could make its way in the mainline code anytime soon? @davidminor
Thanks for all the work guys!

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.

6 participants