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 #15

Merged
merged 2 commits into from
Mar 21, 2023
+16 −22
Merged

Fix Image Caching #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8aae58e
[fix] ImageLoader: ImageUriCache is not kept up to date with loaded i…
kidroca Feb 23, 2023
37264cb
[fix] Image: image LOADED state is only captured initially
kidroca Feb 24, 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
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 @@ -211,24 +211,18 @@ const BaseImage: ImageComponent = React.forwardRef((props, ref) => {
}
}

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);
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 [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 +214 to +225
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 @@ -281,7 +275,6 @@ const BaseImage: ImageComponent = React.forwardRef((props, ref) => {
}

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

Expand Down Expand Up @@ -311,7 +304,7 @@ const BaseImage: ImageComponent = React.forwardRef((props, ref) => {

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) {
kidroca marked this conversation as resolved.
Show resolved Hide resolved
const image = requests[`${requestId}`];
if (image) {
image.onerror = null;
image.onload = null;
image = null;
ImageUriCache.remove(image.src);
image.src = '';
Comment on lines +77 to +83
Copy link
Author

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

Existing functionality is unaffected (besides the rename)

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 = (nativeEvent) => {
ImageUriCache.add(uri);
// avoid blocking the main thread
const onDecode = () => {
// Append `source` to match RN's ImageLoadEvent interface
Expand Down Expand Up @@ -185,9 +187,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