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 Caching #2493

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix Image Caching #2493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
524d647
[fix] ImageLoader: ImageUriCache is not kept up to date with loaded i…
kidroca Feb 23, 2023
e77deca
[fix] Image: image LOADED state is only captured initially
kidroca Feb 24, 2023
1929474
[change] Incorrect image test name
kidroca Mar 3, 2023
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
20 changes: 10 additions & 10 deletions packages/react-native-web/src/exports/Image/__tests__/__snapshots__/index-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ exports[`components/Image prop "source" is correctly updated when missing in ini
</div>
`;

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`] = `
<div
class="css-view-175oi2r r-flexBasis-1mlwlqe r-overflow-1udh08x r-zIndex-417010"
>
Expand All @@ -272,53 +272,53 @@ exports[`components/Image prop "source" is not set immediately if the image has
</div>
`;

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>
Comment on lines -275 to 289
Copy link
Author

Choose a reason for hiding this comment

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

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

  1. Test case name was updated
  2. 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';

`;

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`] = `
<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://twitter.com/favicon.ico);"
style="background-image: url(https://yahoo.com/favicon.ico);"
/>
<img
alt=""
class="css-accessibilityImage-9pa8cd"
draggable="false"
src="https://twitter.com/favicon.ico"
src="https://yahoo.com/favicon.ico"
/>
</div>
`;

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`] = `
<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://yahoo.com/favicon.ico);"
style="background-image: url(https://google.com/not-yet-loaded-image.ico);"
/>
<img
alt=""
class="css-accessibilityImage-9pa8cd"
draggable="false"
src="https://yahoo.com/favicon.ico"
src="https://google.com/not-yet-loaded-image.ico"
/>
</div>
`;
Expand Down
4 changes: 2 additions & 2 deletions packages/react-native-web/src/exports/Image/__tests__/index-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(<Image source={source} />);
expect(container.firstChild).toMatchSnapshot();
Expand Down
21 changes: 7 additions & 14 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 @@ -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;
});
Comment on lines -208 to -217
Copy link
Author

Choose a reason for hiding this comment

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

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


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);
Comment on lines +208 to +219
Copy link
Author

Choose a reason for hiding this comment

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

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

const [flatStyle, _resizeMode, filter, _tintColor] = getFlatStyle(
style,
blurRadius,
Expand Down Expand Up @@ -277,7 +271,6 @@ const Image: React.AbstractComponent<
}

// Image loading
const uri = resolveAssetUri(source);
React.useEffect(() => {
abortPendingRequest();

Expand Down Expand Up @@ -316,7 +309,7 @@ const Image: React.AbstractComponent<

function abortPendingRequest() {
if (requestRef.current != null) {
ImageLoader.abort(requestRef.current);
ImageLoader.clear(requestRef.current);
requestRef.current = null;
}
}
Expand Down
17 changes: 9 additions & 8 deletions packages/react-native-web/src/modules/ImageLoader/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}`];
Comment on lines 76 to +78
Copy link
Author

@kidroca kidroca Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (image) {
image.onerror = null;
image.onload = null;
image = null;
ImageUriCache.remove(image.src);
image.src = '';
delete requests[`${requestId}`];
}
},
Expand All @@ -102,7 +103,7 @@ const ImageLoader = {
}
}
if (complete) {
ImageLoader.abort(requestId);
ImageLoader.clear(requestId);
clearInterval(interval);
}
}
Expand All @@ -111,7 +112,7 @@ const ImageLoader = {
if (typeof failure === 'function') {
failure();
}
ImageLoader.abort(requestId);
ImageLoader.clear(requestId);
clearInterval(interval);
}
},
Expand All @@ -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') {
Expand All @@ -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();
},
Expand Down