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

Make it possible to obtain the cache path of an image #228

Closed
wants to merge 1 commit into from

Conversation

andriichernenko
Copy link

@andriichernenko andriichernenko commented Jun 22, 2018

This pull request adds the FastImage.loadImage(source) method which fetches the image and returns the path to its copy in the disk cache.

This may not be useful for everybody, but I figured I'd open a PR anyway.

@andriichernenko
Copy link
Author

@DylanVann any comments or thoughts?

@n1ru4l
Copy link
Contributor

n1ru4l commented Jul 10, 2018

@andriichernenko Could you please give a reason why this should be added to the API? Also this is a Android only implementation. Have you planned to also add the ios counterpart?

@andriichernenko
Copy link
Author

@n1ru4l In some (admittedly rare) cases it is useful to have path to the local copy of the image. An example of such use case would be #220.

The iOS counterpart is implemented too (see FFFastImageViewManager.m).

@n1ru4l
Copy link
Contributor

n1ru4l commented Jul 10, 2018

@andriichernenko I overlooked that file sorry

@compojoom
Copy link

@andriichernenko - can you give an example on how to use this?
So, I take it - if the image is already visible to the user a call to FastImage.loadImage(source) - would just return the local path without re-downloading the image?

@andriichernenko
Copy link
Author

Here's an example usage:

const imagePath = await FastImage.loadImage({ uri: imageURI });

If this PR is accepted, I will update the docs.

The idea is that if the image has been downloaded before (for example, if it has been displayed somewhere), the path to its copy in the cache will be returned. Otherwise, it will be downloaded first. There might be a catch though. As I understand, Glide uses things like target image size to generate the cache key. These things are not possible to determine if the target view is not known, so this might mean it will be downloaded again (because it was not found in the cache). I am not really familiar enough with Glide to say whether this is the case or not.

@compojoom
Copy link

This sounds exciting! I'll try to test this.

What do you mean with a "target image size". The way I understand it the image is downloaded based on the url? So, if it is downloaded once, it won't be downloaded again.

@andriichernenko
Copy link
Author

andriichernenko commented Jul 11, 2018

I read somewhere in the Glide's GitHub issues (can't seem to find the relevant issue now) that they may store multiple resized copies of an image in the cache. But, of course, this should not affect downloads, so never mind.

@compojoom
Copy link

@andriichernenko amazing! Just added this to iOS and the difference between - manually fetching the image and using this to get the path to it is enourmous! Previously it took like 10s to fetch the image, convert it to base64 and display the share screen. Now it is nearly instant...

A question though - how are we supposed to test this pull request? I tried doing:

yarn add react-native-fast-image@https://github.com/softwerkab/react-native-fast-image\#cache-path

but this was failing with

[4/4] 📃  Building fresh packages...
$ git submodule update --init --recursive
fatal: Not a git repository (or any of the parent directories): .git
error Command failed with exit code 128.

So I manually added the changes to the files... But I would like to use this pull request till it gets merged.

@DylanVann could you merge this?

@andriichernenko
Copy link
Author

@compojoom I ran into this issue as well, it is caused by yarnpkg/yarn#1488. I "fixed" it by removing the script that fetches git submodules (see softwerkab@2d7b399). Since all dependencies in git submodules are added via Cocoapods anyway, everything seems to work.

@compojoom
Copy link

compojoom commented Jul 12, 2018

Thanks! Tested now on android as well. It works. One thing to note on android is that the returned URI doesn't have a file: prefix and if you try to process this further one gets an error.

@andriichernenko
Copy link
Author

andriichernenko commented Jul 12, 2018

The intention is to return the path, so that's what the method does on both Android and iOS. Is there a reason we need a URI?

if you try to process this further one gets an error.

Can you elaborate?

@compojoom
Copy link

doesn't matter. I added a file:// to it and it seems to work both on ios and android.

this is how I use it together with react-native-share

    return FastImage.loadImage({ uri: imageURL })
      .then(imagePath => {
        return getBase64ForImage('file://' + imagePath).then((imageData) => {
          shareOptions.url = imageData

          Share.open(shareOptions)
            .then(() => {

            }).catch((err) => {
            // do nothing for now
          })
        }).catch( (reason) => Alert.alert('sharing failed', reason.message))
      })
  }

@andriyslyusar
Copy link

andriyslyusar commented Aug 1, 2018

Hey, can we proceed with merging this pull request?

@Riant
Copy link

Riant commented Aug 17, 2018

I think put the path attribute on the onLoad callback e.nativeEvent argument is a better idea. So we can get the path like bellow:

  <FastImage ... onLoad={(e) => this.onImageLoad(e)}
  />
  ...

  onImageLoad(e) {
    console.log(e.nativeEvent.width, e.nativeEvent.height, e.nativeEvent.path)
  }

@retyui
Copy link

retyui commented Aug 23, 2018

@compojoom
This realization is very necessary, how soon will you be able to merge this PR?

@compojoom
Copy link

@retyui - I'm not the maintainer of the project and don't have commit access. @DylanVann would have to merge this.

@nihp
Copy link

nihp commented Oct 9, 2018

@DylanVann kindly merge this as soon as possible.

@maxencehenneron
Copy link

maxencehenneron commented Oct 25, 2018

I really need this feature as well.

A few use cases:

  • using previously cached images with any native library that requires an image as an input
  • sharing a photo on a distant server without having to redownload the photo again

@andreyslyusar can you rebase this ?

@compojoom
Copy link

hey guys! I've made a new pull request #351 that resolves the current conflicts this branch has. Please review it and let's get this merged.

@andriyslyusar
Copy link

Sorry, I was busy with another project. @compojoom thanks for taking care of resolving conflicts. @DylanVann can you help us merge #351 and close this pull request as duplicate?

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.

8 participants