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.

Sign up for GitHub

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

fix(Image): keep function identiy accross renders #2638

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+34 −11
Open

fix(Image): keep function identiy accross renders #2638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions packages/react-native-web/src/exports/Image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import StyleSheet from '../StyleSheet';
import TextAncestorContext from '../Text/TextAncestorContext';
import View from '../View';
import { warnOnce } from '../../modules/warnOnce';
import useCallbackRef from '../../vendor/react-native/Utilities/useCallbackRef';

export type { ImageProps };

Expand Down Expand Up @@ -230,6 +231,12 @@ const Image: React.AbstractComponent<
const backgroundImage = displayImageUri ? `url("${displayImageUri}")` : null;
const backgroundSize = getBackgroundSize();

const onErrorRef = useCallbackRef(onError);
const onLoadRef = useCallbackRef(onLoad);
const onLoadEndRef = useCallbackRef(onLoadEnd);
const onLoadStartRef = useCallbackRef(onLoadStart);


// Accessibility image allows users to trigger the browser's image context menu
const hiddenImage = displayImageUri
? createElement('img', {
Expand Down Expand Up @@ -276,32 +283,32 @@ const Image: React.AbstractComponent<

if (uri != null) {
updateState(LOADING);
if (onLoadStart) {
onLoadStart();
if (onLoadStartRef.current) {
onLoadStartRef.current();
}

requestRef.current = ImageLoader.load(
uri,
function load(e) {
updateState(LOADED);
if (onLoad) {
onLoad(e);
if (onLoadRef.current) {
onLoadRef.current(e);
}
if (onLoadEnd) {
onLoadEnd();
if (onLoadEndRef.current) {
onLoadEndRef.current();
}
},
function error() {
updateState(ERRORED);
if (onError) {
onError({
if (onErrorRef.current) {
onErrorRef.current({
nativeEvent: {
error: `Failed to load resource ${uri} (404)`
}
});
}
if (onLoadEnd) {
onLoadEnd();
if (onLoadEndRef.current) {
onLoadEndRef.current();
}
}
);
Expand All @@ -315,7 +322,7 @@ const Image: React.AbstractComponent<
}

return abortPendingRequest;
}, [uri, requestRef, updateState, onError, onLoad, onLoadEnd, onLoadStart]);
}, [uri, requestRef, updateState, onErrorRef, onLoadRef, onLoadEndRef, onLoadStartRef]);

return (
<View
Expand Down
16 changes: 16 additions & 0 deletions packages/react-native-web/src/vendor/react-native/Utilities/useCallbackRef.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Helper to keep the same ref of a function accross renders
*
*/
import { useLayoutEffect, useEffect, useRef } from 'react';

export default function useCallbackRef(callback) {
const callbackRef = useRef(callback);
const useIsomorphicLayoutEffect = typeof window !== 'undefined' ? useLayoutEffect : useEffect;

useIsomorphicLayoutEffect(() => {
Copy link

Choose a reason for hiding this comment

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

Is there any point delaying the ref update through an effect?

Copy link
Author

Choose a reason for hiding this comment

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

initial ref is set with const callbackRef = useRef(callback);

it then updates in the effect

Copy link

Choose a reason for hiding this comment

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

I understand that. I mean, you can simply update the ref without any effects. Simpler code and it guarantees that the most recent value of callback is assigned to the ref and will be called by Image - you don't know when exactly the effect will be executed and Image may stop loading between the render and the effect.

Copy link
Author

Choose a reason for hiding this comment

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

That's true. I'm wondering why swr does it this way though? (They also don't have the config in the dependency array). There's also the experimental_useEffectEvent from react, so I guess there are edge cases with both solutions

callbackRef.current = callback;
}, [callback]);

return callbackRef;
}