-
-
Notifications
You must be signed in to change notification settings - Fork 20
fix: clear state in useImage
hook
#44
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
base: main
Are you sure you want to change the base?
Conversation
// clear state each time we will load a new image | ||
if (image.image || image.error) { | ||
setImage({ image: undefined, error: undefined }); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I put it in the cleanup function but I moved this, as setting new state from it is not safe. We check if state is "dirty " to skip this in "initial" call
I guess with #41 we could check if source is same and skip it in that case const isSameSource = (a: AsyncImageSource, b: AsyncImageSource) => {
if (!b) return false;
if (isHybridObject(a) && isHybridObject(b)) {
return a.equals(b);
}
return JSON.stringify(a) === JSON.stringify(b);
};
const shouldClearState = (source: AsyncImageSource, result: Result) => {
const { image, error } = result;
// If there was an error, we need to clear the state
if (error) return true;
// If there is an image, we check if the source has changed
// if not, we don't need to clear the state
if (image && "__source" in image && image.__source) {
// @ts-expect-error - We save the source on the JS side so we can diff it
return !isSameSource(source, image.__source);
}
return false;
}
...
// in hook
if (shouldClearState(source, image)) {
setImage({ image: undefined, error: undefined });
} But this can lead to unpredictable behavior - eg. what if one endpoint returns random image? imo this should be handled be cache that users can control but let know what do you think about this |
1692a58
to
8a0ca27
Compare
hey - is this PR ready as is? |
8a0ca27
to
cfbd545
Compare
Hey! It seems I forgot to commit my changes 😅 - now it's ready! |
Summary
Clear state in
useImage
hook, each time when loading new image.As documented in file, we should go back to "loading state" while we are loading new source
fixes #37