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

Jump to bottom

[add] Image source headers handling #11

Merged
merged 8 commits into from
Jan 27, 2023
Merged

[add] Image source headers handling #11

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
059ab04
[add] Image source headers handling
kidroca Jan 6, 2023
4e6daca
[add] Image: Append source to `nativeEvent`
kidroca Jan 6, 2023
8f4d952
[change] Image code comments
kidroca Jan 15, 2023
1e54c64
[add] ImageLoader.loadUsingHeaders
kidroca Jan 23, 2023
93df02a
Cleanup
kidroca Jan 23, 2023
7353183
Apply suggestions from code review
kidroca Jan 25, 2023
bb25c15
Image - clean up comments
kidroca Jan 25, 2023
1f393c4
Apply suggestions from code review
kidroca Jan 26, 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
23 changes: 23 additions & 0 deletions packages/react-native-web-examples/pages/image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@ const dataBase64Svg =
'data:image/svg+xml;base64,PHN2ZyB3aWR0aD0nMjAwJyBoZWlnaHQ9JzIwMCcgZmlsbD0iIzAwMDAwMCIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIiB4bWxuczp4bGluaz0iaHR0cDovL3d3dy53My5vcmcvMTk5OS94bGluayIgdmVyc2lvbj0iMS4xIiB4PSIwcHgiIHk9IjBweCIgdmlld0JveD0iMCAwIDEwMCAxMDAiIGVuYWJsZS1iYWNrZ3JvdW5kPSJuZXcgMCAwIDEwMCAxMDAiIHhtbDpzcGFjZT0icHJlc2VydmUiPjxnPjxwYXRoIGQ9Ik0yNS44NjcsNDguODUzQzMyLjgwNiw1MC4xNzYsNDYuNDYsNTIuNSw2MS4yMTUsNTIuNWgwLjAwNWM5LjcxLDAsMTguNDAxLTEuMDU3LDI1LjkzOC0yLjkxMyAgIGMwLjE1OS0wLjA0NiwwLjM1LTAuMTM1LDAuNTY1LTAuMTg3YzAuMjgyLTAuMDcyLDAuNTY1LTAuMTY0LDAuODQ0LTAuMjM4YzMuMTg0LTAuOTY0LDIuNTc3LTMuMDUxLDIuMTk5LTMuODUyICAgYy00LjE2Ni03LjcxOS0xNS4wODYtMjMuNDE1LTM1LjAyOC0yMy40MTVjLTIyLjE2OSwwLTMwLjI2MiwxMC42MzUtMzMuMTQsMTkuNTg5QzIyLjU0NSw0Mi4zMzMsMjIuNDA3LDQ3LjEzNSwyNS44NjcsNDguODUzeiAgICBNMjguNjc2LDM4LjAzMmMwLjAxMy0wLjAzNiwwLjYxNC0xLjYyNiwxLjkyMy0xLjAwOGMxLjEzMywwLjUzNSwwLjk2MSwxLjU2MywwLjg4NywxLjg1Yy0wLjAwNywwLjAyNC0wLjAxNCwwLjA0OC0wLjAyMSwwLjA3MyAgIGMwLDAuMDAxLTAuMDAxLDAuMDA0LTAuMDAxLDAuMDA0bDAsMGMtMC4yNDksMC45MjktMC40MDQsMi4wODYtMC4wMTcsMi44NmMwLjE2LDAuMzE5LDAuNDkyLDAuNzY4LDEuNTQyLDAuOTg3bDAuMzY2LDAuMDc3ICAgYzIwLjgxNiw0LjM2LDM2LDIuOTMzLDQ1LjY3OCwwLjYyNmwtMC4wMDQsMC4wMDJjMCwwLDAuMDA1LTAuMDAyLDAuMDA3LTAuMDAzYzAuMjEyLTAuMDUsMC40MjEtMC4xMDEsMC42MjgtMC4xNTIgICBjMC41MDktMC4wNSwxLjE3MywwLjA3OCwxLjM5OSwxYzAuMzUxLDEuNDI0LTAuOTczLDEuODk1LTEuMjE3LDEuOTY5Yy01LjMyNSwxLjI3OS0xMi4yNjYsMi4zMDYtMjAuODM1LDIuMzA3ICAgYy03LjUwNSwwLTE2LjI1NS0wLjc4Ny0yNi4yNTctMi44ODJsLTAuMzY0LTAuMDc3Yy0yLjEyLTAuNDQyLTMuMTExLTEuNjMzLTMuNTY5LTIuNTU1QzI3Ljk4NSw0MS40MjEsMjguMjgxLDM5LjQxNiwyOC42NzYsMzguMDMyICAgeiI+PC9wYXRoPjxjaXJjbGUgY3g9IjEwLjQ5MyIgY3k9IjIzLjQ1NSIgcj0iMC42MTkiPjwvY2lyY2xlPjxwYXRoIGQ9Ik0yLjA4LDI4LjMwOGMwLjY3Ni0wLjE3OCwwLjk4My0wLjM1MiwxLjE3NC0wLjVDNC42OSwyNi42OSw2LjUsMjcuNDgzLDcuNSwyOC4zNTd2MC4wMDJjMCwwLDEuNzExLDEuMjM1LDAuNzM3LDIuMjAyICAgYy0wLjk3NCwwLjk2NS0yLjMxOSwwLjAwNi0yLjMxOSwwLjAwNmwwLjAzNSwwLjAxNmMtMC4zMjctMC4yMDMtMC42LTAuNTYxLTAuNzgtMC41ODRjLTAuMzcsMC4yNi0wLjg3NiwwLjUtMS40NzYsMC41SDMuNyAgIGMwLDAtMS4zNDUsMC43MDksMC4xNzgsMS42NTJjMC4wMDEsMC4wMDEsMC4wMDIsMC4wNzIsMC4wMDQsMC4wNzNjMy45MzksMi4zNDIsOC4yNzEsNS43MDEsOC4yNzEsOC44OCAgIGMwLDAuNjkxLDAuMiwxNy4wNDIsMTcuNjI2LDI0LjczOWwwLjk2NywwLjQ0MmwtMC4xLDEuMDU5Yy0wLjQyMSw0LjM5LDEuMTQ1LDEwLjE5MSwxMC45OTMsMTIuODg4bDAuMTEzLDAuMDM4ICAgYzAuMDY3LDAuMDIzLDYuNzMyLDIuNDI5LDEwLjkwNywyLjQyOWMxLjU4NCwwLDIuMTU1LTAuMzUyLDIuMjQzLTAuNTYxYzAuMDg1LTAuMjAyLDAuNjEyLTIuMTY0LTYuMzMyLTkuMzg3bDAuMDAyLTAuMTgzICAgYzAsMC0yLjQ3Ny0zLjA3LDEuNTMzLTMuMDdjMC4wMSwwLDAuMDE5LDAsMC4wMjksMGMxLjI4NSwwLDIuNjA4LDAuMjE1LDMuOTgsMC4xODRjNC43NzEtMC4xMTcsOS4zMTYtMC40MjUsMTMuNTA2LTEuMDk2ICAgbDAuNDc0LTAuMDI4bDAuNjY4LDAuMTU4YzkuNjUxLDQuOTQ4LDE2LjczOCw3LjcxNiwxOS43MzgsNy43MTZ2MC4wMDZjMCwwLDAuMTY0LDAuMDExLDAuMjMsMC4wMDQgICBjLTAuMTg5LTAuNzIzLTIuMjMtMi44LTcuMjMtOS4wNzl2MC4wMjFjMCwwLTEuNTEyLTEuNjU4LDAuNzk3LTIuNjUzYzAuMDYzLTAuMDI2LDAuMDA4LDAuMDIzLDAuMDYtMC4wMDEgICBjOC42MzktMy41MDksMTMuNTAxLTguMjA0LDE1LjQxMS0xMS43NzVjMS4xNDUtMi4xMjksMC4yMDYtMi43ODQtMC42NTktMi45NzZjLTAuMzE3LTAuMDM4LTAuNjM0LTAuMDYyLTAuOTEyLTAuMDYyICAgYy0wLjIwNSwwLTAuMzc5LDAuMDEtMC41MjgsMC4wMjdsLTMuMTQzLDEuMjE0QzgzLjczMiw1My45MjYsNzMuMjE4LDU1LjUsNjEuMjIsNTUuNWMtMC4wMDIsMC0wLjAwNSwwLTAuMDA1LDAgICBjLTE1LjEyOCwwLTI5LjEwMS0yLjQzMi0zNi4wODMtMy43NzFsLTAuMTczLTAuMTExbC0wLjE2LTAuMTI2Yy01Ljg1OC0yLjY4MS01LjEzNy0xMC4yMDItNS4xMDMtMTAuNTE5bDAuMDYtMC4zICAgYzAuODk1LTIuODM4LDIuNDY3LTYuMzUyLDUuMjEzLTkuNzE5Yy0xLjgwOC0xLjM2OS00LjU5LTQuMTg4LTQuNDMtOC40OTRjMC4wNDYtMS4yNDQtMC40ODYtMi41MDgtMS40OTgtMy41NTkgICBjLTEuNDk4LTEuNTU1LTMuNzg1LTIuNDQ2LTYuMjc0LTIuNDQ2Yy0xLjc3LDAtMy41NTMsMC40NDItNS4yOTMsMS4zMTRjLTQuMDYxLDIuMDM1LTQuODU1LDQuNzM2LTUuNjkyLDcuNTk2ICAgYy0wLjEzNiwwLjQ2OC0wLjI4NCwwLjkzOS0wLjQzOCwxLjQxYy0wLjAwNiwwLjAxOS0wLjAyMiwwLjAzNS0wLjAyOCwwLjA1NkMwLjgzMywyOC40MjMsMS42OTEsMjguMzksMi4wOCwyOC4zMDh6IE0xMC40OTMsMTkuOTA4ICAgYzEuOTU2LDAsMy41NDgsMS41OTEsMy41NDgsMy41NDdjMCwxLjk1Ny0xLjU5MiwzLjU0OC0zLjU0OCwzLjU0OGMtMS45NTcsMC0zLjU0OC0xLjU5Mi0zLjU0OC0zLjU0OCAgIEM2Ljk0NCwyMS40OTksOC41MzYsMTkuOTA4LDEwLjQ5MywxOS45MDh6Ij48L3BhdGg+PC9nPjwvc3ZnPg==';
const dataSvg =
'data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 841.9 595.3"><g fill="#61DAFB"><path d="M666.3 296.5c0-32.5-40.7-63.3-103.1-82.4 14.4-63.6 8-114.2-20.2-130.4-6.5-3.8-14.1-5.6-22.4-5.6v22.3c4.6 0 8.3.9 11.4 2.6 13.6 7.8 19.5 37.5 14.9 75.7-1.1 9.4-2.9 19.3-5.1 29.4-19.6-4.8-41-8.5-63.5-10.9-13.5-18.5-27.5-35.3-41.6-50 32.6-30.3 63.2-46.9 84-46.9V78c-27.5 0-63.5 19.6-99.9 53.6-36.4-33.8-72.4-53.2-99.9-53.2v22.3c20.7 0 51.4 16.5 84 46.6-14 14.7-28 31.4-41.3 49.9-22.6 2.4-44 6.1-63.6 11-2.3-10-4-19.7-5.2-29-4.7-38.2 1.1-67.9 14.6-75.8 3-1.8 6.9-2.6 11.5-2.6V78.5c-8.4 0-16 1.8-22.6 5.6-28.1 16.2-34.4 66.7-19.9 130.1-62.2 19.2-102.7 49.9-102.7 82.3 0 32.5 40.7 63.3 103.1 82.4-14.4 63.6-8 114.2 20.2 130.4 6.5 3.8 14.1 5.6 22.5 5.6 27.5 0 63.5-19.6 99.9-53.6 36.4 33.8 72.4 53.2 99.9 53.2 8.4 0 16-1.8 22.6-5.6 28.1-16.2 34.4-66.7 19.9-130.1 62-19.1 102.5-49.9 102.5-82.3zm-130.2-66.7c-3.7 12.9-8.3 26.2-13.5 39.5-4.1-8-8.4-16-13.1-24-4.6-8-9.5-15.8-14.4-23.4 14.2 2.1 27.9 4.7 41 7.9zm-45.8 106.5c-7.8 13.5-15.8 26.3-24.1 38.2-14.9 1.3-30 2-45.2 2-15.1 0-30.2-.7-45-1.9-8.3-11.9-16.4-24.6-24.2-38-7.6-13.1-14.5-26.4-20.8-39.8 6.2-13.4 13.2-26.8 20.7-39.9 7.8-13.5 15.8-26.3 24.1-38.2 14.9-1.3 30-2 45.2-2 15.1 0 30.2.7 45 1.9 8.3 11.9 16.4 24.6 24.2 38 7.6 13.1 14.5 26.4 20.8 39.8-6.3 13.4-13.2 26.8-20.7 39.9zm32.3-13c5.4 13.4 10 26.8 13.8 39.8-13.1 3.2-26.9 5.9-41.2 8 4.9-7.7 9.8-15.6 14.4-23.7 4.6-8 8.9-16.1 13-24.1zM421.2 430c-9.3-9.6-18.6-20.3-27.8-32 9 .4 18.2.7 27.5.7 9.4 0 18.7-.2 27.8-.7-9 11.7-18.3 22.4-27.5 32zm-74.4-58.9c-14.2-2.1-27.9-4.7-41-7.9 3.7-12.9 8.3-26.2 13.5-39.5 4.1 8 8.4 16 13.1 24 4.7 8 9.5 15.8 14.4 23.4zM420.7 163c9.3 9.6 18.6 20.3 27.8 32-9-.4-18.2-.7-27.5-.7-9.4 0-18.7.2-27.8.7 9-11.7 18.3-22.4 27.5-32zm-74 58.9c-4.9 7.7-9.8 15.6-14.4 23.7-4.6 8-8.9 16-13 24-5.4-13.4-10-26.8-13.8-39.8 13.1-3.1 26.9-5.8 41.2-7.9zm-90.5 125.2c-35.4-15.1-58.3-34.9-58.3-50.6 0-15.7 22.9-35.6 58.3-50.6 8.6-3.7 18-7 27.7-10.1 5.7 19.6 13.2 40 22.5 60.9-9.2 20.8-16.6 41.1-22.2 60.6-9.9-3.1-19.3-6.5-28-10.2zM310 490c-13.6-7.8-19.5-37.5-14.9-75.7 1.1-9.4 2.9-19.3 5.1-29.4 19.6 4.8 41 8.5 63.5 10.9 13.5 18.5 27.5 35.3 41.6 50-32.6 30.3-63.2 46.9-84 46.9-4.5-.1-8.3-1-11.3-2.7zm237.2-76.2c4.7 38.2-1.1 67.9-14.6 75.8-3 1.8-6.9 2.6-11.5 2.6-20.7 0-51.4-16.5-84-46.6 14-14.7 28-31.4 41.3-49.9 22.6-2.4 44-6.1 63.6-11 2.3 10.1 4.1 19.8 5.2 29.1zm38.5-66.7c-8.6 3.7-18 7-27.7 10.1-5.7-19.6-13.2-40-22.5-60.9 9.2-20.8 16.6-41.1 22.2-60.6 9.9 3.1 19.3 6.5 28.1 10.2 35.4 15.1 58.3 34.9 58.3 50.6-.1 15.7-23 35.6-58.4 50.6zM320.8 78.4z"/><circle cx="420.9" cy="296.5" r="45.7"/><path d="M520.5 78.1z"/></g></svg>';
const sourceWithHeaders = {
uri: placeholder,
headers: {
'x-token': '0012345'
}
};
const sourceWithHeadersAndRedirect = {
uri: source,
headers: {
'x-token': '0012345'
}
};

function Divider() {
return <View style={styles.divider} />;
Expand Down Expand Up @@ -114,6 +126,17 @@ export default function ImagePage() {
/>
</View>
</View>
<Divider />
<View style={styles.row}>
<View style={styles.column}>
<Text style={[styles.text]}>With Headers</Text>
<Image source={sourceWithHeaders} style={styles.image} />
</View>
<View style={styles.column}>
<Text style={[styles.text]}>Headers & Redirect</Text>
<Image source={sourceWithHeadersAndRedirect} style={styles.image} />
</View>
</View>
</Example>
);
}
Expand Down
104 changes: 91 additions & 13 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 @@ -8,7 +8,7 @@
* @flow
*/

import type { ImageProps } from './types';
import type { ImageProps, SourceObject } from './types';

import * as React from 'react';
import createElement from '../createElement';
Expand Down Expand Up @@ -146,6 +146,12 @@ function resolveAssetUri(source): ?string {
return uri;
}

function hasSourceDiff(a: SourceObject, b: SourceObject) {
return (
a.uri !== b.uri || JSON.stringify(a.headers) !== JSON.stringify(b.headers)
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
);
}

interface ImageStatics {
getSize: (
uri: string,
Expand All @@ -158,10 +164,12 @@ interface ImageStatics {
) => Promise<{| [uri: string]: 'disk/memory' |}>;
}

const Image: React.AbstractComponent<
type ImageComponent = React.AbstractComponent<
ImageProps,
React.ElementRef<typeof View>
> = React.forwardRef((props, ref) => {
>;

const BaseImage: ImageComponent = React.forwardRef((props, ref) => {
const {
accessibilityLabel,
blurRadius,
Expand Down Expand Up @@ -332,24 +340,94 @@ const Image: React.AbstractComponent<
);
});

Image.displayName = 'Image';
/**
* This component handles specifically loading an image source with header
kidroca marked this conversation as resolved.
Show resolved Hide resolved
*/
const ImageWithHeaders: ImageComponent = React.forwardRef((props, ref) => {
// $FlowIgnore
const nextSource: SourceObject = props.source;
const prevSource = React.useRef<SourceObject>({});
const cleanup = React.useRef<Function>(() => {});
const [blobUri, setBlobUri] = React.useState('');

const { onError, onLoadStart } = props;

React.useEffect(() => {
if (!hasSourceDiff(nextSource, prevSource.current)) return;

// When source changes we want to clean up any old/running requests
cleanup.current();

prevSource.current = nextSource;

let uri;
const abortCtrl = new AbortController();
const request = new Request(nextSource.uri, {
headers: nextSource.headers,
signal: abortCtrl.signal
});
request.headers.append('accept', 'image/*');

if (onLoadStart) onLoadStart();
kidroca marked this conversation as resolved.
Show resolved Hide resolved

fetch(request)
.then((response) => response.blob())
.then((blob) => {
uri = URL.createObjectURL(blob);
setBlobUri(uri);
})
.catch((error) => {
if (error.name !== 'AbortError' && onError) {
onError({ nativeEvent: error.message });
}
});
Copy link
Author

Choose a reason for hiding this comment

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

I've tried to write some unit tests covering the new functionality, but because of this logic a few mocks need to be setup and make the PR bigger than it has to be

I think it might be better to move this logic to ImageLoader.loadUsingHeaders so in unit tests we can stub that method and ensure it gets called when we intent to

It's easy to manually verify the fetch logic works and downloads content
Once that's clear it's easy to just verify loadUsingHeaders is called correctly with expected input

I've made a similar comment on the main repo: https://github.com/necolas/react-native-web/pull/2442/files#r1072080652

What do you think, should we refactor / write some tests now or let the mainstream repo request those changes?

Copy link

Choose a reason for hiding this comment

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

Hmm since this logic is the real meat & potatoes of the ImageWithHeaders component, if we move it to ImageLoader.loadUsingHeaders I assume we could get rid of this ImageWithHeaders component, and basically check if there's headers passed in the props - if yes, we call ImageLoader.loadUsingHeaders instead of ImageLoader.load here?

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

Lines 269 to 294 in bd1f7b8

requestRef.current = ImageLoader.load(
uri,
function load(e) {
updateState(LOADED);
if (onLoad) {
onLoad(e);
}
if (onLoadEnd) {
onLoadEnd();
}
},
function error() {
updateState(ERRORED);
if (onError) {
onError({
nativeEvent: {
error: `Failed to load resource ${uri} (404)`
}
});
}
if (onLoadEnd) {
onLoadEnd();
}
}
);
}

If that's what you're thinking, I honestly do think that logic is cleaner, even without needing to write tests for Expensify's case so I'd say the refactor sounds like a nice idea

Copy link
Author

@kidroca kidroca Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Well, the idea is purely to ease mocking but I'll give your suggestion a try

Last PR tried to solve everything inside the same component but that resulted in more logic

What a component like ImageWithHeaders gives us is a guarantee that source would always be an object with headers

BTW there's feedback on the mainstream PR: necolas#2442 (comment) that we should write tests and thumbs up for extracting ImageLoader.loadUsingHeaders

Copy link
Author

Choose a reason for hiding this comment

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

I've tried to remove the ImageWithHeaders component and use either loadUsingHeaders or load functions here: https://github.com/Expensify/react-native-web/compare/master...kidroca:react-native-web:kidroca/feat/image-loader-headers-alt-2?diff=unified

But it results in a similar amount of changes and modifies some of the original logic (and seems harder to review)

  • because now the source loading useEffect needs to account for objects
  • load and loadWithHeaders need to work in a similar way in order to be interchangeable (so they were modified to return a request.cancel function)

We might merge load and loadUsingHeaders instead of testing which one to run, though this would be similar to the first PR were load handled loading images with and without headers

Copy link

Choose a reason for hiding this comment

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

Thanks for doing that refactor! Personally I prefer the new changes because it keeps all the image loading logic in ImageLoader and there's minimal changes to Image/index.js - I don't see those new changes too difficult to review (though I am not sure where lastLoadedSource gets updated).

I'd say let's move forward with this last refactor, as I think it's pretty straightforward compared to passing / not passing specific props to the BaseImage component required in this PR

Copy link
Author

@kidroca kidroca Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IMO the refactor is more of a proof that there are more "gymnastics" necessary in order to make this work by changing the original logic inside Image component

Original logic is changed, the cleanup logic is different, ImageLoader.load is changed, people, including the mainstream maintainer would have to inspect how these used to work and whether the change is suitable

IMO the mainstream maintainer already saw the update and is fine with just moving the fetch call to ImageLoader.loadUsingHeaders. I'm still trying different things, but the most straightforward PR so far seems to be the current one

There's also a big rework planned for the Image and the original loading logic would change, it would probably be best to not write stuff that depend on it (in the alt branch loadUsingHeaders depends on load)

Copy link
Author

Choose a reason for hiding this comment

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

Let me update the current PR with loadUsingHeaders extracted to ImageLoader and then we can make one final decision which set of changes would work best for us

Copy link

Choose a reason for hiding this comment

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

IMO the mainstream maintainer already saw the update and is fine with just moving the fetch call to ImageLoader.loadUsingHeaders. I'm still trying different things, but the most straightforward PR so far seems to be the current one

Yeah this is one additional reason I really like this approach, even though there may be some additional changes in the upstream repo needed that we don't need at the moment in this fork 👍

Let me update the current PR with loadUsingHeaders extracted to ImageLoader and then we can make one final decision which set of changes would work best for us

That sounds perfect, thanks so much @kidroca 👍


// Capture a cleanup function for the current request
// The reason for using a Ref is to avoid making this function a dependency
// Because the change of a dependency would otherwise would re-trigger a hook
kidroca marked this conversation as resolved.
Show resolved Hide resolved
cleanup.current = () => {
abortCtrl.abort();
setBlobUri('');
URL.revokeObjectURL(uri);
};
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
}, [nextSource, onLoadStart, onError]);

// Run the cleanup function on unmount
React.useEffect(() => cleanup.current, []);

const propsToPass = {
...props,
// Omit `onLoadStart` because we trigger it in the current scope
Copy link

Choose a reason for hiding this comment

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

Not too sure what this comment means. Is there a different way to say this?

I think it's something like - the BaseImage onLoadStart event is not exposed to the parent. We are only interested in when the source with headers starts loading and not when the BaseImage loading starts?

Copy link

Choose a reason for hiding this comment

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

I think it's more like: ImageWithHeaders already calls onLoadStart when it starts loading the image, so we don't want the BaseImage to trigger that function a second time

Copy link
Author

Choose a reason for hiding this comment

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

Now that I look at it I see it's confusing - it's like Alex said - loading starts inside ImageWithHeaders, to prevent BaseImage to raise onLoadStart a second time we filter it out

Copy link

Choose a reason for hiding this comment

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

Ok cool I think we are all saying the same thing - BaseImage will not use an onLoadStart callback when we have headers.

kidroca marked this conversation as resolved.
Show resolved Hide resolved
onLoadStart: undefined,
// Until the current component resolves the request (using headers)
kidroca marked this conversation as resolved.
Show resolved Hide resolved
// we skip forwarding the source so the base component doesn't attempt
// to load the original source
source: blobUri ? { ...nextSource, uri: blobUri } : undefined
};

return <BaseImage ref={ref} {...propsToPass} />;
});

// $FlowIgnore: This is the correct type, but casting makes it unhappy since the variables aren't defined yet
const ImageWithStatics = (Image: React.AbstractComponent<
ImageProps,
React.ElementRef<typeof View>
> &
ImageStatics);
const Image: ImageComponent & ImageStatics = React.forwardRef((props, ref) => {
if (props.source && props.source.headers) {
return <ImageWithHeaders ref={ref} {...props} />;
}

return <BaseImage ref={ref} {...props} />;
});

Image.displayName = 'Image';

ImageWithStatics.getSize = function (uri, success, failure) {
Image.getSize = function (uri, success, failure) {
ImageLoader.getSize(uri, success, failure);
};

ImageWithStatics.prefetch = function (uri) {
Image.prefetch = function (uri) {
return ImageLoader.prefetch(uri);
};

ImageWithStatics.queryCache = function (uris) {
Image.queryCache = function (uris) {
return ImageLoader.queryCache(uris);
};

Expand Down Expand Up @@ -405,4 +483,4 @@ const resizeModeStyles = StyleSheet.create({
}
});

export default ImageWithStatics;
export default Image;
8 changes: 4 additions & 4 deletions packages/react-native-web/src/exports/Image/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import type {
TransformStyles
} from '../../types/styles';

type SourceObject = {
export type SourceObject = {
/**
* `body` is the HTTP body to send with the request. This must be a valid
* UTF-8 string, and will be sent exactly as specified, with no
Expand Down Expand Up @@ -102,8 +102,8 @@ export type ImageStyle = {
tintColor?: ColorValue
};

export type ImageProps = {
...ViewProps,
export type ImageProps = {|
...$Exact<ViewProps>,
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
blurRadius?: number,
defaultSource?: Source,
draggable?: boolean,
Expand All @@ -116,4 +116,4 @@ export type ImageProps = {
resizeMode?: ResizeMode,
source?: Source,
style?: GenericStyleProp<ImageStyle>
};
|};
4 changes: 2 additions & 2 deletions packages/react-native-web/src/exports/ImageBackground/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ import Image from '../Image';
import StyleSheet from '../StyleSheet';
import View from '../View';

type ImageBackgroundProps = {
type ImageBackgroundProps = {|
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
...ImageProps,
imageRef?: any,
imageStyle?: $PropertyType<ImageProps, 'style'>,
style?: $PropertyType<ViewProps, 'style'>
};
|};

const emptyObject = {};

Expand Down
12 changes: 10 additions & 2 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 @@ -122,9 +122,17 @@ const ImageLoader = {
id += 1;
const image = new window.Image();
image.onerror = onError;
image.onload = (e) => {
image.onload = (nativeEvent) => {
// avoid blocking the main thread
const onDecode = () => onLoad({ nativeEvent: e });
const onDecode = () => {
nativeEvent.source = {
kidroca marked this conversation as resolved.
Show resolved Hide resolved
uri: image.src,
width: image.naturalWidth,
height: image.naturalHeight
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
};

onLoad({ nativeEvent });
};
if (typeof image.decode === 'function') {
// Safari currently throws exceptions when decoding svgs.
// We want to catch that error and allow the load handler
Expand Down