From 524d647efbe1023fde73e96ea07bf17f88ba2002 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 23 Feb 2023 13:56:58 +0200 Subject: [PATCH 1/3] [fix] ImageLoader: ImageUriCache is not kept up to date with loaded images --- .../react-native-web/src/exports/Image/index.js | 2 +- .../src/modules/ImageLoader/index.js | 17 +++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/react-native-web/src/exports/Image/index.js b/packages/react-native-web/src/exports/Image/index.js index bd69e5e844..7e810bf054 100644 --- a/packages/react-native-web/src/exports/Image/index.js +++ b/packages/react-native-web/src/exports/Image/index.js @@ -316,7 +316,7 @@ const Image: React.AbstractComponent< function abortPendingRequest() { if (requestRef.current != null) { - ImageLoader.abort(requestRef.current); + ImageLoader.clear(requestRef.current); requestRef.current = null; } } diff --git a/packages/react-native-web/src/modules/ImageLoader/index.js b/packages/react-native-web/src/modules/ImageLoader/index.js index 892db99292..5671cb3456 100644 --- a/packages/react-native-web/src/modules/ImageLoader/index.js +++ b/packages/react-native-web/src/modules/ImageLoader/index.js @@ -74,12 +74,13 @@ let id = 0; const requests = {}; const ImageLoader = { - abort(requestId: number) { - let image = requests[`${requestId}`]; + clear(requestId: number) { + const image = requests[`${requestId}`]; if (image) { image.onerror = null; image.onload = null; - image = null; + ImageUriCache.remove(image.src); + image.src = ''; delete requests[`${requestId}`]; } }, @@ -102,7 +103,7 @@ const ImageLoader = { } } if (complete) { - ImageLoader.abort(requestId); + ImageLoader.clear(requestId); clearInterval(interval); } } @@ -111,7 +112,7 @@ const ImageLoader = { if (typeof failure === 'function') { failure(); } - ImageLoader.abort(requestId); + ImageLoader.clear(requestId); clearInterval(interval); } }, @@ -123,6 +124,7 @@ const ImageLoader = { const image = new window.Image(); image.onerror = onError; image.onload = (e) => { + ImageUriCache.add(uri); // avoid blocking the main thread const onDecode = () => onLoad({ nativeEvent: e }); if (typeof image.decode === 'function') { @@ -143,9 +145,8 @@ const ImageLoader = { ImageLoader.load( uri, () => { - // Add the uri to the cache so it can be immediately displayed when used - // but also immediately remove it to correctly reflect that it has no active references - ImageUriCache.add(uri); + // load() adds the uri to the cache so it can be immediately displayed when used, + // but we also immediately remove it to correctly reflect that it has no active references ImageUriCache.remove(uri); resolve(); }, From e77decaafb54c2559e0af1ec256f1fa1b04ac25e Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 24 Feb 2023 20:15:10 +0200 Subject: [PATCH 2/3] [fix] Image: image LOADED state is only captured initially If the Image component is rendered with a `null` source, and consecutively updated with actual source url that was already loaded, it would fail to pic kup the change - `state` would be `IDLE` for a brief moment and this would cause a small flicker when the image renders Let's always start from IDLE state, and update `shouldDisplaySource` condition to be based on `ImageLoader.has` cache or not --- .../src/exports/Image/index.js | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/packages/react-native-web/src/exports/Image/index.js b/packages/react-native-web/src/exports/Image/index.js index 7e810bf054..d84eb2a7a4 100644 --- a/packages/react-native-web/src/exports/Image/index.js +++ b/packages/react-native-web/src/exports/Image/index.js @@ -205,24 +205,18 @@ const Image: React.AbstractComponent< } } - const [state, updateState] = React.useState(() => { - const uri = resolveAssetUri(source); - if (uri != null) { - const isLoaded = ImageLoader.has(uri); - if (isLoaded) { - return LOADED; - } - } - return IDLE; - }); - + const [state, updateState] = React.useState(IDLE); const [layout, updateLayout] = React.useState({}); const hasTextAncestor = React.useContext(TextAncestorContext); const hiddenImageRef = React.useRef(null); const filterRef = React.useRef(_filterId++); const requestRef = React.useRef(null); + const uri = resolveAssetUri(source); + const isCached = uri != null && ImageLoader.has(uri); const shouldDisplaySource = - state === LOADED || (state === LOADING && defaultSource == null); + state === LOADED || + isCached || + (state === LOADING && defaultSource == null); const [flatStyle, _resizeMode, filter, _tintColor] = getFlatStyle( style, blurRadius, @@ -277,7 +271,6 @@ const Image: React.AbstractComponent< } // Image loading - const uri = resolveAssetUri(source); React.useEffect(() => { abortPendingRequest(); From 192947497fc2fbb352b0aa6db3e3649c92aae7b9 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 3 Mar 2023 12:49:25 +0200 Subject: [PATCH 3/3] [change] Incorrect image test name As we can see by the created snapshot - the image is rendered This is because there's no default source. In such case we render the Image source while it loads --- .../__snapshots__/index-test.js.snap | 20 +++++++++---------- .../src/exports/Image/__tests__/index-test.js | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/react-native-web/src/exports/Image/__tests__/__snapshots__/index-test.js.snap b/packages/react-native-web/src/exports/Image/__tests__/__snapshots__/index-test.js.snap index 55e2d30ac5..b3e04407ac 100644 --- a/packages/react-native-web/src/exports/Image/__tests__/__snapshots__/index-test.js.snap +++ b/packages/react-native-web/src/exports/Image/__tests__/__snapshots__/index-test.js.snap @@ -255,7 +255,7 @@ exports[`components/Image prop "source" is correctly updated when missing in ini `; -exports[`components/Image prop "source" is not set immediately if the image has not already been loaded 1`] = ` +exports[`components/Image prop "source" is set immediately if the image has already been loaded 1`] = `
@@ -272,53 +272,53 @@ exports[`components/Image prop "source" is not set immediately if the image has
`; -exports[`components/Image prop "source" is set immediately if the image has already been loaded 1`] = ` +exports[`components/Image prop "source" is set immediately if the image has already been loaded 2`] = `
`; -exports[`components/Image prop "source" is set immediately if the image has already been loaded 2`] = ` +exports[`components/Image prop "source" is set immediately if the image was preloaded 1`] = `
`; -exports[`components/Image prop "source" is set immediately if the image was preloaded 1`] = ` +exports[`components/Image prop "source" is set immediately while image is loading and there is no default source 1`] = `
`; diff --git a/packages/react-native-web/src/exports/Image/__tests__/index-test.js b/packages/react-native-web/src/exports/Image/__tests__/index-test.js index 42392d973c..eba876b929 100644 --- a/packages/react-native-web/src/exports/Image/__tests__/index-test.js +++ b/packages/react-native-web/src/exports/Image/__tests__/index-test.js @@ -248,8 +248,8 @@ describe('components/Image', () => { }); }); - test('is not set immediately if the image has not already been loaded', () => { - const uri = 'https://google.com/favicon.ico'; + test('is set immediately while image is loading and there is no default source', () => { + const uri = 'https://google.com/not-yet-loaded-image.ico'; const source = { uri }; const { container } = render(); expect(container.firstChild).toMatchSnapshot();