Fix Image Caching #15
Conversation
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
| 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); |
There was a problem hiding this comment.
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
nullsource 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); |
There was a problem hiding this comment.
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
| clear(requestId: number) { | ||
| const image = requests[`${requestId}`]; | ||
| if (image) { | ||
| image.onerror = null; | ||
| image.onload = null; | ||
| image = null; | ||
| ImageUriCache.remove(image.src); | ||
| image.src = ''; |
There was a problem hiding this comment.
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)
|
Requesting @marcaaron and @Beamanator for review if you have time given the comment from Kidroca about previous PR in this library |
|
Sorry, I've got to focus on some other things atm so unassigning myself as a reviewer. |
|
Bump @mountiny @Beamanator
We're introducing a small change here so it should be easy to review |
|
@kidroca just to confirm, have you tested this in the App too? do you have a screen recording? We should test it for sure before going ahead Oh I see its being pointed to the compiled code in this PR so I will go ahead and merge this |
There was a problem hiding this comment.
Tests well in the linked App PR, lets do this Expensify/App#15663
|
Yep, i confirm the following
Thanks for the review @mountiny 🫶 |
Description
This PR fixes some subtle inconsistencies around image caching and rendering the same image multiple times
Related to:
Test Strategy
Use App's PR to test the changes (Expensify/App#15663)
Why No Unit Tests
Couldn't easily add unit tests for this change, because the main change is to the
ImageLoader.loadwhich doesn't have a test suit, and is mocked in theImagetestsExisting tests cover the changes in the
Imagecomponent