From 2aa37d240e9d53069294ad5d81075c54a969c583 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 6 Nov 2023 09:32:15 +0200 Subject: [PATCH 1/4] Refactor Image for Web/Desktop with Source Headers - Introduced `BaseImage` component that branches between native and web implementations. - **Native**: Utilizes `FastImage` directly. - **Web**: Minor adjustments made to the `onLoad` event signature for compatibility. - Eliminated `Image/index.native.js` as both native and web components now leverage a unified high-level implementation for image loading and rendering. --- src/components/Image/BaseImage.js | 28 +++++++++++ src/components/Image/BaseImage.native.js | 3 ++ src/components/Image/index.js | 52 +++++++------------ src/components/Image/index.native.js | 63 ------------------------ 4 files changed, 50 insertions(+), 96 deletions(-) create mode 100644 src/components/Image/BaseImage.js create mode 100644 src/components/Image/BaseImage.native.js delete mode 100644 src/components/Image/index.native.js diff --git a/src/components/Image/BaseImage.js b/src/components/Image/BaseImage.js new file mode 100644 index 000000000000..199cbb78aab0 --- /dev/null +++ b/src/components/Image/BaseImage.js @@ -0,0 +1,28 @@ +import React, {useCallback} from 'react'; +import {Image as RNImage} from 'react-native'; +import {defaultProps, imagePropTypes} from './imagePropTypes'; + +function BaseImage({onLoad, ...props}) { + const imageLoadedSuccessfully = useCallback( + ({nativeEvent}) => { + // We override `onLoad`, so both web and native have the same signature + const {width, height} = nativeEvent.source; + onLoad({nativeEvent: {width, height}}); + }, + [onLoad], + ); + + return ( + + ); +} + +BaseImage.propTypes = imagePropTypes; +BaseImage.defaultProps = defaultProps; + +export default BaseImage; diff --git a/src/components/Image/BaseImage.native.js b/src/components/Image/BaseImage.native.js new file mode 100644 index 000000000000..a621947267a1 --- /dev/null +++ b/src/components/Image/BaseImage.native.js @@ -0,0 +1,3 @@ +import RNFastImage from 'react-native-fast-image'; + +export default RNFastImage; diff --git a/src/components/Image/index.js b/src/components/Image/index.js index ef1a69e19c12..440f7ada7348 100644 --- a/src/components/Image/index.js +++ b/src/components/Image/index.js @@ -1,51 +1,37 @@ import lodashGet from 'lodash/get'; -import React, {useEffect, useMemo} from 'react'; -import {Image as RNImage} from 'react-native'; +import React, {useMemo} from 'react'; import {withOnyx} from 'react-native-onyx'; -import _ from 'underscore'; +import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; +import BaseImage from './BaseImage'; import {defaultProps, imagePropTypes} from './imagePropTypes'; import RESIZE_MODES from './resizeModes'; function Image(props) { - const {source: propsSource, isAuthTokenRequired, onLoad, session} = props; - /** - * Check if the image source is a URL - if so the `encryptedAuthToken` is appended - * to the source. - */ + const {source: propsSource, isAuthTokenRequired, session, ...forwardedProps} = props; + + // Update the source to include the auth token if required const source = useMemo(() => { - if (isAuthTokenRequired) { - // There is currently a `react-native-web` bug preventing the authToken being passed - // in the headers of the image request so the authToken is added as a query param. - // On native the authToken IS passed in the image request headers - const authToken = lodashGet(session, 'encryptedAuthToken', null); - return {uri: `${propsSource.uri}?encryptedAuthToken=${encodeURIComponent(authToken)}`}; + if (typeof lodashGet(propsSource, 'uri') === 'number') { + return propsSource.uri; + } + if (typeof propsSource !== 'number' && isAuthTokenRequired) { + const authToken = lodashGet(session, 'encryptedAuthToken'); + return { + ...propsSource, + headers: { + [CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken, + }, + }; } + return propsSource; // The session prop is not required, as it causes the image to reload whenever the session changes. For more information, please refer to issue #26034. // eslint-disable-next-line react-hooks/exhaustive-deps }, [propsSource, isAuthTokenRequired]); - /** - * The natural image dimensions are retrieved using the updated source - * and as a result the `onLoad` event needs to be manually invoked to return these dimensions - */ - useEffect(() => { - // If an onLoad callback was specified then manually call it and pass - // the natural image dimensions to match the native API - if (onLoad == null) { - return; - } - RNImage.getSize(source.uri, (width, height) => { - onLoad({nativeEvent: {width, height}}); - }); - }, [onLoad, source]); - - // Omit the props which the underlying RNImage won't use - const forwardedProps = _.omit(props, ['source', 'onLoad', 'session', 'isAuthTokenRequired']); - return ( - { - const {width, height} = evt.nativeEvent; - dimensionsCache.set(source.uri, {width, height}); - if (props.onLoad) { - props.onLoad(evt); - } - }} - /> - ); -} - -Image.propTypes = imagePropTypes; -Image.defaultProps = defaultProps; -Image.displayName = 'Image'; -const ImageWithOnyx = withOnyx({ - session: { - key: ONYXKEYS.SESSION, - }, -})(Image); -ImageWithOnyx.resizeMode = RESIZE_MODES; -ImageWithOnyx.resolveDimensions = resolveDimensions; - -export default ImageWithOnyx; From 19b605e4e52c1c71d1515065c0ba2de7af0c61b1 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Mon, 20 Nov 2023 20:22:57 +0200 Subject: [PATCH 2/4] Add react-native-web patch for image header support --- ...-web+0.19.9+005+image-header-support.patch | 200 ++++++++++++++++++ 1 file changed, 200 insertions(+) create mode 100644 patches/react-native-web+0.19.9+005+image-header-support.patch diff --git a/patches/react-native-web+0.19.9+005+image-header-support.patch b/patches/react-native-web+0.19.9+005+image-header-support.patch new file mode 100644 index 000000000000..4652e22662f0 --- /dev/null +++ b/patches/react-native-web+0.19.9+005+image-header-support.patch @@ -0,0 +1,200 @@ +diff --git a/node_modules/react-native-web/dist/exports/Image/index.js b/node_modules/react-native-web/dist/exports/Image/index.js +index 95355d5..19109fc 100644 +--- a/node_modules/react-native-web/dist/exports/Image/index.js ++++ b/node_modules/react-native-web/dist/exports/Image/index.js +@@ -135,7 +135,22 @@ function resolveAssetUri(source) { + } + return uri; + } +-var Image = /*#__PURE__*/React.forwardRef((props, ref) => { ++function raiseOnErrorEvent(uri, _ref) { ++ var onError = _ref.onError, ++ onLoadEnd = _ref.onLoadEnd; ++ if (onError) { ++ onError({ ++ nativeEvent: { ++ error: "Failed to load resource " + uri + " (404)" ++ } ++ }); ++ } ++ if (onLoadEnd) onLoadEnd(); ++} ++function hasSourceDiff(a, b) { ++ return a.uri !== b.uri || JSON.stringify(a.headers) !== JSON.stringify(b.headers); ++} ++var BaseImage = /*#__PURE__*/React.forwardRef((props, ref) => { + var ariaLabel = props['aria-label'], + blurRadius = props.blurRadius, + defaultSource = props.defaultSource, +@@ -236,16 +251,10 @@ var Image = /*#__PURE__*/React.forwardRef((props, ref) => { + } + }, function error() { + updateState(ERRORED); +- if (onError) { +- onError({ +- nativeEvent: { +- error: "Failed to load resource " + uri + " (404)" +- } +- }); +- } +- if (onLoadEnd) { +- onLoadEnd(); +- } ++ raiseOnErrorEvent(uri, { ++ onError, ++ onLoadEnd ++ }); + }); + } + function abortPendingRequest() { +@@ -277,10 +286,78 @@ var Image = /*#__PURE__*/React.forwardRef((props, ref) => { + suppressHydrationWarning: true + }), hiddenImage, createTintColorSVG(tintColor, filterRef.current)); + }); +-Image.displayName = 'Image'; ++BaseImage.displayName = 'Image'; ++ ++/** ++ * This component handles specifically loading an image source with headers ++ * default source is never loaded using headers ++ */ ++var ImageWithHeaders = /*#__PURE__*/React.forwardRef((props, ref) => { ++ // $FlowIgnore: This component would only be rendered when `source` matches `ImageSource` ++ var nextSource = props.source; ++ var _React$useState3 = React.useState(''), ++ blobUri = _React$useState3[0], ++ setBlobUri = _React$useState3[1]; ++ var request = React.useRef({ ++ cancel: () => {}, ++ source: { ++ uri: '', ++ headers: {} ++ }, ++ promise: Promise.resolve('') ++ }); ++ var onError = props.onError, ++ onLoadStart = props.onLoadStart, ++ onLoadEnd = props.onLoadEnd; ++ React.useEffect(() => { ++ if (!hasSourceDiff(nextSource, request.current.source)) { ++ return; ++ } ++ ++ // When source changes we want to clean up any old/running requests ++ request.current.cancel(); ++ if (onLoadStart) { ++ onLoadStart(); ++ } ++ ++ // Store a ref for the current load request so we know what's the last loaded source, ++ // and so we can cancel it if a different source is passed through props ++ request.current = ImageLoader.loadWithHeaders(nextSource); ++ request.current.promise.then(uri => setBlobUri(uri)).catch(() => raiseOnErrorEvent(request.current.source.uri, { ++ onError, ++ onLoadEnd ++ })); ++ }, [nextSource, onLoadStart, onError, onLoadEnd]); ++ ++ // Cancel any request on unmount ++ React.useEffect(() => request.current.cancel, []); ++ var propsToPass = _objectSpread(_objectSpread({}, props), {}, { ++ // `onLoadStart` is called from the current component ++ // We skip passing it down to prevent BaseImage raising it a 2nd time ++ onLoadStart: undefined, ++ // Until the current component resolves the request (using headers) ++ // we skip forwarding the source so the base component doesn't attempt ++ // to load the original source ++ source: blobUri ? _objectSpread(_objectSpread({}, nextSource), {}, { ++ uri: blobUri ++ }) : undefined ++ }); ++ return /*#__PURE__*/React.createElement(BaseImage, _extends({ ++ ref: ref ++ }, propsToPass)); ++}); + + // $FlowIgnore: This is the correct type, but casting makes it unhappy since the variables aren't defined yet +-var ImageWithStatics = Image; ++var ImageWithStatics = /*#__PURE__*/React.forwardRef((props, ref) => { ++ if (props.source && props.source.headers) { ++ return /*#__PURE__*/React.createElement(ImageWithHeaders, _extends({ ++ ref: ref ++ }, props)); ++ } ++ return /*#__PURE__*/React.createElement(BaseImage, _extends({ ++ ref: ref ++ }, props)); ++}); + ImageWithStatics.getSize = function (uri, success, failure) { + ImageLoader.getSize(uri, success, failure); + }; +diff --git a/node_modules/react-native-web/dist/modules/ImageLoader/index.js b/node_modules/react-native-web/dist/modules/ImageLoader/index.js +index bc06a87..e309394 100644 +--- a/node_modules/react-native-web/dist/modules/ImageLoader/index.js ++++ b/node_modules/react-native-web/dist/modules/ImageLoader/index.js +@@ -76,7 +76,7 @@ var ImageLoader = { + var image = requests["" + requestId]; + if (image) { + var naturalHeight = image.naturalHeight, +- naturalWidth = image.naturalWidth; ++ naturalWidth = image.naturalWidth; + if (naturalHeight && naturalWidth) { + success(naturalWidth, naturalHeight); + complete = true; +@@ -102,11 +102,19 @@ var ImageLoader = { + id += 1; + var image = new window.Image(); + image.onerror = onError; +- image.onload = e => { ++ image.onload = nativeEvent => { + // avoid blocking the main thread +- var onDecode = () => onLoad({ +- nativeEvent: e +- }); ++ var onDecode = () => { ++ // Append `source` to match RN's ImageLoadEvent interface ++ nativeEvent.source = { ++ uri: image.src, ++ width: image.naturalWidth, ++ height: image.naturalHeight ++ }; ++ 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 +@@ -120,6 +128,32 @@ var ImageLoader = { + requests["" + id] = image; + return id; + }, ++ loadWithHeaders(source) { ++ var uri; ++ var abortController = new AbortController(); ++ var request = new Request(source.uri, { ++ headers: source.headers, ++ signal: abortController.signal ++ }); ++ request.headers.append('accept', 'image/*'); ++ var promise = fetch(request).then(response => response.blob()).then(blob => { ++ uri = URL.createObjectURL(blob); ++ return uri; ++ }).catch(error => { ++ if (error.name === 'AbortError') { ++ return ''; ++ } ++ throw error; ++ }); ++ return { ++ promise, ++ source, ++ cancel: () => { ++ abortController.abort(); ++ URL.revokeObjectURL(uri); ++ } ++ }; ++ }, + prefetch(uri) { + return new Promise((resolve, reject) => { + ImageLoader.load(uri, () => { From cc4920851bed2a50c5e22cea1a2861669042e258 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 24 Nov 2023 16:55:02 +0200 Subject: [PATCH 3/4] refactor(components): Exclude Auth Token for External Avatar Images This patch focuses on resolving issues encountered with avatar image loading, specifically addressing the challenges related to CORS (Cross-Origin Resource Sharing) errors. Changes: - Removed the `isAuthTokenRequired` flag from the `AttachmentModal` component in various files, including `ProfilePage.js`, `RoomHeaderAvatars.js`, and `DetailsPage.js`. This change is crucial for loading of avatar images that are hosted externally. Rationale: - The primary purpose of this modification is to streamline the loading process for avatars by removing the unnecessary inclusion of authentication tokens in requests for external images. This approach aligns with standard practices for handling externally hosted content and aims to enhance compatibility and performance. - Raised a question here as whether there are cases of avatar images that need authentication: https://github.com/Expensify/App/pull/24425/files#r1404352872 This update is expected to resolve the CORS errors associated with avatar image loading, thereby improving the overall functionality and user experience in our application. --- src/components/RoomHeaderAvatars.js | 2 -- src/pages/DetailsPage.js | 1 - src/pages/ProfilePage.js | 1 - 3 files changed, 4 deletions(-) diff --git a/src/components/RoomHeaderAvatars.js b/src/components/RoomHeaderAvatars.js index 0bb0f11d747c..da8e0b76bede 100644 --- a/src/components/RoomHeaderAvatars.js +++ b/src/components/RoomHeaderAvatars.js @@ -33,7 +33,6 @@ function RoomHeaderAvatars(props) { @@ -78,7 +77,6 @@ function RoomHeaderAvatars(props) { diff --git a/src/pages/DetailsPage.js b/src/pages/DetailsPage.js index 66345107dbb1..6d3f0198bbfe 100755 --- a/src/pages/DetailsPage.js +++ b/src/pages/DetailsPage.js @@ -134,7 +134,6 @@ function DetailsPage(props) { {({show}) => ( diff --git a/src/pages/ProfilePage.js b/src/pages/ProfilePage.js index ffe8271629f4..d21d6c82f34a 100755 --- a/src/pages/ProfilePage.js +++ b/src/pages/ProfilePage.js @@ -159,7 +159,6 @@ function ProfilePage(props) { From 1182c7d4ac0124d77bed68c2866eb0e3cfdcf7a2 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Tue, 5 Dec 2023 10:57:02 +0200 Subject: [PATCH 4/4] Align changes with project code styles --- src/components/Image/BaseImage.js | 1 + src/components/Image/index.js | 4 +--- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/components/Image/BaseImage.js b/src/components/Image/BaseImage.js index 199cbb78aab0..cd2326900c6c 100644 --- a/src/components/Image/BaseImage.js +++ b/src/components/Image/BaseImage.js @@ -24,5 +24,6 @@ function BaseImage({onLoad, ...props}) { BaseImage.propTypes = imagePropTypes; BaseImage.defaultProps = defaultProps; +BaseImage.displayName = 'BaseImage'; export default BaseImage; diff --git a/src/components/Image/index.js b/src/components/Image/index.js index 440f7ada7348..8cee1cf95e14 100644 --- a/src/components/Image/index.js +++ b/src/components/Image/index.js @@ -7,9 +7,7 @@ import BaseImage from './BaseImage'; import {defaultProps, imagePropTypes} from './imagePropTypes'; import RESIZE_MODES from './resizeModes'; -function Image(props) { - const {source: propsSource, isAuthTokenRequired, session, ...forwardedProps} = props; - +function Image({source: propsSource, isAuthTokenRequired, session, ...forwardedProps}) { // Update the source to include the auth token if required const source = useMemo(() => { if (typeof lodashGet(propsSource, 'uri') === 'number') {