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 onProgress and onComplete to preload. #268

Closed
wants to merge 7 commits into from

Conversation

doomsower
Copy link

The title is pretty self-descriptive. Here is the feature request topic: #144

Hopefully will add Android counterpart on Saturday.

@doomsower doomsower changed the title [WIP] Add onProgress and onComplete to preload Add onProgress and onComplete to preload Aug 24, 2018
@doomsower
Copy link
Author

Ok, now on Android too!

@doomsower
Copy link
Author

#266

This can be easily added on JS side (e.g. when onProgress and onComplete are both undefined, make JS method return promise).
But first I want maintaineer to review this PR at least. Btw I already use this code in a small production app, no complaints and crashes so far.

@aaalive
Copy link

aaalive commented Nov 1, 2018

Hi
It's very useful, could you please merge?

@alirezarastegarfard
Copy link

how can i use this feature ?

@elan
Copy link

elan commented Dec 3, 2018

Any plans on getting this merged? Would be awesome to not have to fork the repo to use it 😅

@ChristianTucker
Copy link

Please get this merged.

@retyui @DylanVann

@luco
Copy link

luco commented Jan 2, 2019

@DylanVann Merge this please.

@xzilja
Copy link

xzilja commented Jan 13, 2019

Is there anything preventing this PR from being merged @DylanVann ? Would love to help :)

# Conflicts:
#	android/src/main/java/com/dylanvann/fastimage/FastImagePreloaderModule.java
#	src/index.d.ts
#	src/index.js
@huang-xiao-jian
Copy link

Is there any plan for the PR?

@franciscomdelgado
Copy link

@DylanVann willing to resolve CI failure to get this merged into the package. Some guidance would be appreciated though.

@DylanVann
Copy link
Owner

Sorry for the lack of response. The reason I did not implement this feature, and the reason I'd be hesitant to merge this is that the API SDWebImage provides for prefetching images is not very good.

It does not tell you which images fail and succeed, it only gives you the number of successful loads and failed loads, which seems silly to me.

If someone did a PR to improve their API I'd be much more likely to merge something like this.

@franciscomdelgado
Copy link

Sorry for the lack of response. The reason I did not implement this feature, and the reason I'd be hesitant to merge this is that the API SDWebImage provides for prefetching images is not very good.

It does not tell you which images fail and succeed, it only gives you the number of successful loads and failed loads, which seems silly to me.

If someone did a PR to improve their API I'd be much more likely to merge something like this.

Awesome, I was wondering why it hadn't been merged; seems like a good feature. But that is a valid reason not to merge this feature.

# Conflicts:
#	README.md
#	ReactNativeFastImageExample/yarn.lock
#	android/src/main/java/com/dylanvann/fastimage/FastImagePreloaderModule.java
#	ios/FastImage.xcodeproj/project.pbxproj
#	react-native-fast-image-example/src/PreloadExample.js
# Conflicts:
#	android/src/main/java/com/dylanvann/fastimage/FastImagePreloaderModule.java
#	src/index.d.ts
#	src/index.js
@codecov
Copy link

codecov bot commented Aug 31, 2019

Codecov Report

Merging #268 into master will decrease coverage by 38.3%.
The diff coverage is 34.48%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #268       +/-   ##
===========================================
- Coverage   94.11%   55.81%   -38.31%     
===========================================
  Files           1        2        +1     
  Lines          17       43       +26     
===========================================
+ Hits           16       24        +8     
- Misses          1       19       +18
Impacted Files Coverage Δ
src/preloaderManager.js 33.33% <33.33%> (ø)
src/index.js 93.75% <50%> (-0.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b8c0c8...1a395c1. Read the comment docs.

@doomsower
Copy link
Author

doomsower commented Aug 31, 2019

@DylanVann and everyone interested

I've update this PR. Now iOS progress handler checks if the url it receives is cached on disk. Based on this JS callbacks will now return array of cached urls as first argument.

I also merged master, so it's up to date with v7 of this module, and I updated example project to RN 0.60.5. Example project works fine on iOS and Android. However, I haven't tried this in a real project, as I have yet to migrate my app that utilizes preloading to RN 0.60

@DylanVann DylanVann changed the title Add onProgress and onComplete to preload Add onProgress and onComplete to preload. Oct 20, 2019
@DylanVann
Copy link
Owner

Closing this until we can resolve the blocking issue of SDWebImage not giving enough info.

I wrote a roadmap.md file to keep track of this kind of thing, with a link to this PR.

https://github.com/DylanVann/react-native-fast-image/blob/master/docs/roadmap.md#add-onprogress-and-oncomplete-to-preload

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.