Component renders twice
Closed this issue · 13 comments
Hey,
there is an unnecessary addtional rerender happening on the component and caused by the useEffect
specifically by uri in deps array
the problem is when my propsSource is an https for image, the uri will never be equal to propsSource because the uri is the local path while propsSource is an http url, therefore entering to the first if and redo the work again
it would cause an infinite loop if
react-native-image-cache/src/CachedImage.tsx
Line 112 in 5367b6f
setUrl, the url will be the same as the previous one so it won't rerun the useEffect again.
Also please notice the performance difference between the first log and the second one, if we could make it like the second one it will be super fast and unnoticable.
@saif-o99 thanks for spotting this, please feel free to open a PR that solves it! Thanks
@efstathiosntonas i can do that, can you just tell if the uri in the deps array is needed? if yes, what for.
also is this code still needed?
https://github.com/georstat/react-native-image-cache/blob/5367b6f5cb6f552a04a1c8359e555bfdacef0133/src/CachedImage.tsx#L31C14-L31C14
if these were added for the component unmounted warning then we no longer need them
@saif-o99 I really can't remember 😄 , it's kindof translation of this one: https://github.com/wcandillon/react-native-expo-image-cache/blob/b81b99b01918558c7680595980d76003d507bc6f/src/Image.tsx#L53. I'm rereading the flow now to remember it.
About the useStateIfMounted it's needed for react < 18 or Paper builds with react > 17.
@saif-o99 I really can't remember 😄 , it's kindof translation of this one: https://github.com/wcandillon/react-native-expo-image-cache/blob/b81b99b01918558c7680595980d76003d507bc6f/src/Image.tsx#L53. I'm rereading the flow now to remember it.
i traced the code but couldnt find a case where we need to recall load when the url changes so i think we dont need it. also the first if statement in the useEffect will never match because propsSource is http url while uri is local path so this needs to be removed too
i think this should be the proper flow:
useEffect(() => {
load(props).catch();
if (propsSource !== currentSource) {
setCurrentSource(propsSource);
setUri(undefined);
resetAnimations();
}
/* eslint-disable react-hooks/exhaustive-deps */
}, [propsSource, propsOptions]);I believe this line setUri(undefined); is the reason of the check if (propsSource !== uri) {
maybe the propsSource should be currentSource, running some tests now.
i think this should be the proper flow:
useEffect(() => { load(props).catch(); if (propsSource !== currentSource) { setCurrentSource(propsSource); setUri(undefined); resetAnimations(); } /* eslint-disable react-hooks/exhaustive-deps */ }, [propsSource, propsOptions]);
100%, i'll make a PR for that
I'm checking if propsSource should be currentSource, give me 5 mins, check it your self too
I believe this line
setUri(undefined);is the reason of the checkif (propsSource !== uri) {maybe the
propsSourceshould becurrentSource, running some tests now.
i think you meant url to be currentSource
because
url is something like /data/user/0/com.app/cache/images_cache/80f56239124b6e6fa6505bc87e88cc801da0a195.jpg
while currentSource and propsSource is something like https://app/api/6253388ccdf378256ede6aae/picture
so url would never be equal to currentSource or propsSource... correct me if im wrong
@saif-o99 can you test this out:
const prevSource = useRef<string | null>(propsSource);
useEffect(() => {
if (prevSource.current !== uri) {
load(props).catch();
}
if (propsSource !== currentSource) {
setCurrentSource(propsSource);
setUri(undefined);
resetAnimations();
}
/* eslint-disable react-hooks/exhaustive-deps */
}, [propsSource, uri, propsOptions, prevSource]);
const load = async ({
maxAge,
noCache = false,
onError,
options = {},
source
}: ImageProps): Promise<void> => {
if (source) {
try {
const path = await CacheManager.get(source, options, noCache, maxAge).getPath();
if (path) {
setUri(path);
prevSource.current = path; <---- ADDED
setError(false);
} else {
setError(true);
prevSource.current = null; <----- ADDED
onError({
nativeEvent: { error: new Error("Could not load image") }
});
}
} catch (e: any) {
setError(true);
onError({ nativeEvent: { error: e } });
}
}
};@efstathiosntonas there is a unneeded complexity with this code, here is the flow
1- enters useEffect then enters the load function
2- load function will setUri and prevSource
3- prevSource won't trigger rerender on useEffect so no need for it there
4- uri will trigger a rerender on useEffect but not the IF statement because prevSource.current == uri
5- but then setUri(undefined) will run and trigger rerender on useEffect and now it will enter the IF statement because uri changed
my observation is this:
1- if propsOptions changed but the source still the same the prevSource.current !== uri will prevent enter to load function again
2- prevSource won't trigger useEffect rerender
3-still not sure we need uri as deps array item, i think we dont need it
4-currentSource should be useRef not useState
and this is my latest code change please check it
const currentSource = React.useRef<string>(propsSource);
useEffect(() => {
load(props).catch();
if (propsSource !== currentSource.current) {
currentSource.current = propsSource;
resetAnimations();
}
/* eslint-disable react-hooks/exhaustive-deps */
}, [propsSource, propsOptions]);
const load = async ({
maxAge,
noCache = false,
onError,
options = {},
source,
}: ImageProps): Promise<void> => {
if (source) {
try {
const path = await CacheManager.get(
source,
options,
noCache,
maxAge
).getPath();
if (path) {
setUri(path);
setError(false);
} else {
setUri(undefined); <=== ADDED
setError(true);
onError({
nativeEvent: { error: new Error('Could not load image') },
});
}
} catch (e: any) {
setUri(undefined); <=== ADDED
setError(true);
onError({ nativeEvent: { error: e } });
}
}
};
@saif-o99 looks good to me, submit it 😄
closed via 2.8.0