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

feat:preload callbacks #995

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Conversation

nicomontanari
Copy link

@nicomontanari nicomontanari commented Jul 17, 2023

I opened another pull request because the previous one was unexpectedly closed.

@DylanVann I have been using these changes in production for 1 year and everything works fine.

doomsower and others added 25 commits August 29, 2018 09:02
# Conflicts:
#	android/src/main/java/com/dylanvann/fastimage/FastImagePreloaderModule.java
#	src/index.d.ts
#	src/index.js
Update from original repository
…-fetch-updates-2021-08-18

Feature/preload callbacks fetch updates 2021 08 18
@mathbalduino
Copy link

Hey @DylanVann, it would be nice to have this one merged. What do you think?

@lorenzpfei
Copy link

@DylanVann can you please merge this? It would be very helpful

@akeva001
Copy link

@nicomontanari would you be able to help me on adding this PR to my react native project? I have tried downloading the stable version of react-native-fast-image and applying the necessary changes to the files, but I am not able to get it working.

@akeva001
Copy link

@nicomontanari would you be able to help me on adding this PR to my react native project? I have tried downloading the stable version of react-native-fast-image and applying the necessary changes to the files, but I am not able to get it working.

I ended up being able to install it properly I believe,
Screenshot 2023-09-27 at 7 54 45 PM

But this code block is not logging anything,
Screenshot 2023-09-27 at 7 57 24 PM

@sterlingwes
Copy link

This PR is missing the runtime change needed in index.tsx, it's still calling the old native module.

Instead replace with:

FastImage.preload = (sources, onProgress, onComplete) =>
    preloaderManager.preload(sources, onProgress, onComplete)

@nicomontanari
Copy link
Author

Hi @akeva001, i'm sorry for the delay in my response.

First i added this line in the "dependecies" section in the package.json:
"react-native-fast-image": "https://github.com/nicomontanari/react-native-fast-image".

Then this is how i implemented the preload:

import FastImage from 'react-native-fast-image/src'

const preload = async (images: string[]): Promise<void> => {
    return new Promise<void>(async (resolve, reject) => {
        FastImage.preload(
            images.map((image) => ({
                uri: image
            })),
            (finished, total) => {
                if (__DEV__) {
                    console.log('Preload:', `f ${finished}/t ${total}`)
                }
            },
            (_, skipped) => {
                if (skipped > 0) {
                    reject(skipped)
                    if (__DEV__) {
                        console.log('Preload: failed with skipped ', skipped)
                    }
                }
                if (__DEV__) {
                    console.log('Preload: finish')
                }
                resolve()
            }
        )
    })
}

@nicomontanari
Copy link
Author

@sterlingwes i'm sorry but i didn't understand your comment. These changes already implement your piece of code.

@sterlingwes
Copy link

@nicomontanari if you look in the PR diff here and scroll to the very bottom you'll see that you only have typescript changes in src/index. With no runtime change, the code snippet you shared above is still calling the existing native module method which doesn't do anything with the callback params.

You need to update the index file to pass those params into the method exposed by PreloaderManager which isn't currently used in this diff.

…erListener.java

Co-authored-by: Wes Johnson <swj@wesquire.ca>
Copy link

@frg100 frg100 left a comment

Choose a reason for hiding this comment

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

It looks like src/index.tsx:260 needs some changes to use the new interface

@nicomontanari
Copy link
Author

@frg100 @sterlingwes thanks for the tip, I missed these changes along the way 😕

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.

None yet