wagslane / react-native-expo-cached-image

Cached image component for Expo's managed workflow
https://qvault.io/2020/02/04/how-to-cache-images-react-native-expo-managed
MIT License
44 stars 8 forks source link

Fixed corrupted caching issue and lag on navigation animations #21

Closed yoeven closed 3 years ago

yoeven commented 3 years ago

Hi there, the pull request fixes two key issues:

  1. Previously when the component gets unmounted, it causes the image download to break and save the file regardless of its completion status. Instead, now I use a downloadResumable with a callback to track the progress of its completion and if the image doesn't completely download at the point of unmounting, it would delete the half downloaded file.

  2. The component starts the load process of the image on componentDidMount which causes lag to JS-based animations and react-navigation transition. The solution was to use the InteractionManager.runAfterInteractions function that would start the load process only after any interaction or JS animations are done.

wagslane commented 3 years ago

Hey @yoeven! Thanks for the PR! A couple things, looks like this PR does more than just those two things:

Can you scope down your PR to ONLY address the scope of what you want to change?

yoeven commented 3 years ago

Hi @lane-c-wagner, Thanks for the feedback!

  1. I fixed the styling by adding prettier support and also added expo's eslint config for future PR's so that styling will always stay consistent

  2. I'm a little unsure on what you mean by your second point, not really sure what is being removed. You stated that "The ability to use an instead of a is being removed.", would be great if you could elaborate a little more or provide an example so I can make the right changes. :)

wagslane commented 3 years ago

Hey sorry, I didn't escape point number two properly! It was this: The ability to use an <Image> instead of a <BackgroundImage> is being removed., but now I'm seeing the order was switched, it wasn't removed (I'm on a phone right now haha).

So the only thing I'll ask at this point is that you remove all the styling changes (including prettier). It's typically unwise to try to make styling AND functionality changes in one PR. If you want to make styling changes because you intend to do further work on the project, just open up a separate pull request please :)

yoeven commented 3 years ago

Hey there, reverted the prettier changes accordingly and only applied the functionality changes while keeping the styling consistent to the initial repo. I'll send another PR after this gets merge for styling and maybe adding typescript :) Hope its all good now!

Also, recently I'm participating in the hacktoberfest and was hoping if you could label this PR with hacktoberfest-accepted for it to be counted as a contribution :)

yoeven commented 3 years ago

alright, removed the unnecessary logs

yoeven commented 3 years ago

Also if possible could you label this fork with hacktoberfest-accepted so that it counts against the participation of the hacktoberfest event :)

wagslane commented 3 years ago

Absolutely. I'll get that done and published