Fix Image Caching #2493

kidroca · 2023-03-03T11:10:36Z

Description

This PR fixes some subtle inconsistencies around image caching and rendering the same image multiple times

Test Strategy

Created a sandbox sample from the pull request, where the bug can no longer happens
https://codesandbox.io/s/rnw-image-flicker-bug-fix-s93ri0

Steps:

Go to https://codesandbox.io/s/rnw-image-flicker-bug-fix-s93ri0
Press the "Add More" button at the top a few time
Images are no longer flickering

Why No Unit Tests

Couldn't easily add unit tests for this change, because the main change is to the ImageLoader.load which doesn't have a test suit, and is mocked in the Image tests

Existing tests cover the changes in the Image component

codesandbox-ci · 2023-03-03T11:11:39Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1929474:

Sandbox	Source
react-native-web-examples	Configuration
RNW-image-flicker-bug-fix	PR
RNW-image-flicker-bug-fix	PR
RNW-image-flicker-bug-sample	Issue #2492

kidroca · 2023-03-03T11:24:53Z

packages/react-native-web/src/exports/Image/__tests__/__snapshots__/index-test.js.snap

-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`] = `
 <div
  class="css-view-175oi2r r-flexBasis-1mlwlqe r-overflow-1udh08x r-zIndex-417010"
 >
  <div
    class="css-view-175oi2r r-backgroundColor-1niwhzg r-backgroundPosition-vvn4in r-backgroundRepeat-u6sd8q r-bottom-1p0dtai r-height-1pi2tsx r-left-1d2f490 r-position-u8s1d r-right-zchlnj r-top-ipm5af r-width-13qz1uu r-zIndex-1wyyakw r-backgroundSize-4gszlv"
-    style="background-image: url(https://google.com/favicon.ico);"
+    style="background-image: url(https://twitter.com/favicon.ico);"
  />
  <img
    alt=""
    class="css-accessibilityImage-9pa8cd"
    draggable="false"
-    src="https://google.com/favicon.ico"
+    src="https://twitter.com/favicon.ico"
  />
 </div>


The diff is confusing, it does not represent the change correctly

Test case name was updated

This resulted in 1 new snapshot created and 1 obsolete snapshot removed

If we inspect the snapshot (number 2) we see that it correctly renders uriTwo

react-native-web/packages/react-native-web/src/exports/Image/__tests__/index-test.js

Line 281 in 38bb9fa

const uriTwo = 'https://twitter.com/favicon.ico';

packages/react-native-web/src/exports/Image/__tests__/index-test.js

kidroca · 2023-03-03T11:32:40Z

packages/react-native-web/src/exports/Image/index.js

-  const [state, updateState] = React.useState(() => {
-    const uri = resolveAssetUri(source);
-    if (uri != null) {
-      const isLoaded = ImageLoader.has(uri);
-      if (isLoaded) {
-        return LOADED;
-      }
-    }
-    return IDLE;
-  });


This is a state initializer, it runs only once, because of that the component does not behave correctly when we switch source, cases like

component is mounted with null source then changed to actual source that was already loaded - state was already initialized and is not updated to LOADED

component changes source dynamically to some other source that was loaded - same problem as above

This causes flickers on some occasions

kidroca · 2023-03-03T11:36:31Z

packages/react-native-web/src/exports/Image/index.js

+  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);


We can capture the old "state initializer" in a hook and run it on source change, so that if already loaded source is passed we can directly switch to LOADED state, but I find it simpler to always start from IDLE state and capture isCached like the proposed change. This way we've evaluated the correct result on the same render, while an use effect would only update state on the next render

kidroca · 2023-03-03T11:40:16Z

packages/react-native-web/src/modules/ImageLoader/index.js

 const ImageLoader = {
-  abort(requestId: number) {
-    let image = requests[`${requestId}`];
+  clear(requestId: number) {
+    const image = requests[`${requestId}`];


renamed this to clear to represent more closely that it's not just for aborting but actually a necessary cleanup, as it also updates the ImageUriCache now

The change doesn't alter existing behavior

kidroca · 2023-03-22T16:53:52Z

BTW This PR also fixes Image.queryCache - I don't think we have an issue for this, but right now Image.queryCache does not work as intended (nothing already loaded is considered cached)
@necolas do you want me to open an issue for tracking purposes?

necolas · 2023-03-29T18:39:35Z

Hi, please could you rebase this. Now that 0.19 is done, the 0.20 release will be focused on Image changes.

…mages

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

kidroca · 2023-04-04T07:24:50Z

✅ rebased

kidroca · 2023-04-04T07:43:20Z

packages/react-native-web/src/exports/Image/__tests__/index-test.js

-    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 images is loading and there is no default source', () => {
+      const uri = 'https://google.com/not-yet-loaded-image.ico';
      const source = { uri };
      const { container } = render(<Image source={source} />);


Perhaps this was originally true, and source was not rendered until loaded, but if you inspect the snapshot you can see that <img /> with the source was rendered, so I renamed the test

We do render the source while loading, unless there's a default source

react-native-web/packages/react-native-web/src/exports/Image/index.js

Lines 210 to 211 in e8098fd

const shouldDisplaySource =

state === LOADED || (state === LOADING && defaultSource == null);

necolas · 2023-04-05T17:31:41Z

One unit test snapshot is failing

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

kidroca · 2023-04-06T16:47:22Z

One unit test snapshot is failing

Sorry, should be fine now

…mages Close #2493

kidroca force-pushed the kidroca/fix/image-caching branch from 38bb9fa to 216e758 Compare March 3, 2023 11:45

kidroca commented Mar 3, 2023

View reviewed changes

kidroca marked this pull request as ready for review March 3, 2023 11:57

This was referenced Mar 3, 2023

[HOLD for payment 2023-04-03] Clean-up react-native-web workaround and fix it upstream Expensify/App#14278

Closed

Fix Image Caching Expensify/react-native-web#15

Merged

necolas added this to the 0.20: Image milestone Mar 20, 2023

kidroca added 2 commits April 4, 2023 10:18

[fix] ImageLoader: ImageUriCache is not kept up to date with loaded i…

524d647

…mages

kidroca force-pushed the kidroca/fix/image-caching branch from 216e758 to 8991b58 Compare April 4, 2023 07:21

kidroca force-pushed the kidroca/fix/image-caching branch from 8991b58 to fa50f4f Compare April 4, 2023 07:42

kidroca commented Apr 4, 2023

View reviewed changes

kidroca force-pushed the kidroca/fix/image-caching branch from fa50f4f to 491b728 Compare April 4, 2023 07:44

[change] Incorrect image test name

1929474

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

kidroca force-pushed the kidroca/fix/image-caching branch from 491b728 to 1929474 Compare April 6, 2023 16:45

necolas pushed a commit that referenced this pull request Apr 10, 2023

[fix] ImageLoader: ImageUriCache is not kept up to date with loaded i…

7f77017

…mages Close #2493

necolas added the has: pr Subject of a pull request label Apr 10, 2023

necolas pushed a commit that referenced this pull request Apr 10, 2023

[fix] ImageLoader: ImageUriCache is not kept up to date with loaded i…

160f50f

…mages Close #2493

necolas pushed a commit that referenced this pull request Apr 12, 2023

[fix] ImageLoader: ImageUriCache is not kept up to date with loaded i…

e722282

…mages Close #2493

roryabraham mentioned this pull request Dec 9, 2023

Tracker to follow up with react-native-web patches Expensify/App#32544

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Image Caching #2493

Fix Image Caching #2493

kidroca commented Mar 3, 2023 •

edited

codesandbox-ci bot commented Mar 3, 2023 •

edited

kidroca Mar 3, 2023

kidroca Mar 3, 2023

kidroca Mar 3, 2023

kidroca Mar 3, 2023 •

edited

kidroca commented Mar 22, 2023

necolas commented Mar 29, 2023

kidroca commented Apr 4, 2023

kidroca Apr 4, 2023

necolas commented Apr 5, 2023

kidroca commented Apr 6, 2023

	const shouldDisplaySource =
	state === LOADED \|\| (state === LOADING && defaultSource == null);

Fix Image Caching #2493

Are you sure you want to change the base?

Fix Image Caching #2493

Conversation

kidroca commented Mar 3, 2023 • edited

Description

Test Strategy

Steps:

Why No Unit Tests

codesandbox-ci bot commented Mar 3, 2023 • edited

kidroca Mar 3, 2023

Choose a reason for hiding this comment

kidroca Mar 3, 2023

Choose a reason for hiding this comment

kidroca Mar 3, 2023

Choose a reason for hiding this comment

kidroca Mar 3, 2023 • edited

Choose a reason for hiding this comment

kidroca commented Mar 22, 2023

necolas commented Mar 29, 2023

kidroca commented Apr 4, 2023

kidroca Apr 4, 2023

Choose a reason for hiding this comment

necolas commented Apr 5, 2023

kidroca commented Apr 6, 2023

kidroca commented Mar 3, 2023 •

edited

codesandbox-ci bot commented Mar 3, 2023 •

edited

kidroca Mar 3, 2023 •

edited