-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
[TS Migration] Migrate Image to TypeScript #31296
Changes from 48 commits
f67e753
ba4eda8
98bd9e1
1e95421
ed90b35
c1d0c2c
b423c06
46c9f92
0041ba8
b446609
21c7980
f115914
e2508f9
3f260bc
3a76fb0
247dfff
02daaf5
0e11fb8
31aa36a
06c374c
e9451f0
a695adb
ee81360
9ceb2eb
50726c7
4c69801
3f5550f
1056c9f
b13fce2
611227e
7b6ce98
6cc06a2
e24eef6
3b958ba
7ba47c8
403dc95
080dbf4
747f7f5
4851245
2f780c6
9dcfc34
0ef3bf5
171e45a
5d125cf
d1e55cc
1e582b6
065c27c
05919f2
cc78940
c0f10cd
3476ef1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,35 +1,26 @@ | ||
import {Image as ImageComponent} from 'expo-image'; | ||
import lodashGet from 'lodash/get'; | ||
import React from 'react'; | ||
import {withOnyx} from 'react-native-onyx'; | ||
import CONST from '@src/CONST'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import {defaultProps, imagePropTypes} from './imagePropTypes'; | ||
import RESIZE_MODES from './resizeModes'; | ||
import type {ImageOnyxProps, ImageProps} from './types'; | ||
|
||
const dimensionsCache = new Map(); | ||
|
||
function resolveDimensions(key) { | ||
return dimensionsCache.get(key); | ||
} | ||
|
||
function Image(props) { | ||
// eslint-disable-next-line react/destructuring-assignment | ||
const {source, isAuthTokenRequired, session, ...rest} = props; | ||
|
||
function Image({source, isAuthTokenRequired = false, session, onLoad, ...rest}: ImageProps) { | ||
let imageSource = source; | ||
if (source && source.uri && typeof source.uri === 'number') { | ||
if (typeof source === 'object' && 'uri' in source && typeof source.uri === 'number') { | ||
imageSource = source.uri; | ||
} | ||
if (typeof imageSource !== 'number' && isAuthTokenRequired) { | ||
const authToken = lodashGet(props, 'session.encryptedAuthToken', null); | ||
if (typeof imageSource === 'object' && typeof source === 'object' && isAuthTokenRequired) { | ||
const authToken = session?.encryptedAuthToken ?? null; | ||
imageSource = { | ||
...source, | ||
headers: authToken | ||
? { | ||
[CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken, | ||
} | ||
: null, | ||
: undefined, | ||
}; | ||
} | ||
|
||
|
@@ -41,23 +32,20 @@ function Image(props) { | |
onLoad={(evt) => { | ||
const {width, height, url} = evt.source; | ||
dimensionsCache.set(url, {width, height}); | ||
if (props.onLoad) { | ||
props.onLoad({nativeEvent: {width, height}}); | ||
if (onLoad) { | ||
onLoad({nativeEvent: {width, height}}); | ||
} | ||
}} | ||
/> | ||
); | ||
} | ||
|
||
Image.propTypes = imagePropTypes; | ||
Image.defaultProps = defaultProps; | ||
Image.displayName = 'Image'; | ||
const ImageWithOnyx = withOnyx({ | ||
|
||
const ImageWithOnyx = withOnyx<ImageProps, ImageOnyxProps>({ | ||
session: { | ||
key: ONYXKEYS.SESSION, | ||
}, | ||
})(Image); | ||
ImageWithOnyx.resizeMode = RESIZE_MODES; | ||
ImageWithOnyx.resolveDimensions = resolveDimensions; | ||
Comment on lines
-60
to
-61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Safe to remove these? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some time ago we decided to remove these as they are not used anywhere (here) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not going to block on this because @kidroca is addressing the |
||
|
||
export default ImageWithOnyx; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,24 @@ | ||
import lodashGet from 'lodash/get'; | ||
import React, {useEffect, useMemo} from 'react'; | ||
import {Image as RNImage} from 'react-native'; | ||
import {withOnyx} from 'react-native-onyx'; | ||
import _ from 'underscore'; | ||
import useNetwork from '@hooks/useNetwork'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import {defaultProps, imagePropTypes} from './imagePropTypes'; | ||
import RESIZE_MODES from './resizeModes'; | ||
import type {ImageOnyxProps, ImageOwnProps, ImageProps} from './types'; | ||
|
||
function Image(props) { | ||
const {source: propsSource, isAuthTokenRequired, onLoad, session} = props; | ||
function Image({source: propsSource, isAuthTokenRequired = false, onLoad, session, ...forwardedProps}: ImageProps) { | ||
const {isOffline} = useNetwork(); | ||
|
||
/** | ||
* Check if the image source is a URL - if so the `encryptedAuthToken` is appended | ||
* to the source. | ||
*/ | ||
const source = useMemo(() => { | ||
if (isAuthTokenRequired) { | ||
const authToken = session?.encryptedAuthToken ?? null; | ||
if (isAuthTokenRequired && typeof propsSource === 'object' && 'uri' in propsSource && authToken) { | ||
// 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)}`}; | ||
return {uri: `${propsSource?.uri}?encryptedAuthToken=${encodeURIComponent(authToken)}`}; | ||
} | ||
JKobrynski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. | ||
|
@@ -39,13 +35,13 @@ function Image(props) { | |
if (onLoad == null) { | ||
return; | ||
} | ||
RNImage.getSize(source.uri, (width, height) => { | ||
onLoad({nativeEvent: {width, height}}); | ||
}); | ||
}, [onLoad, source, isOffline]); | ||
|
||
// Omit the props which the underlying RNImage won't use | ||
const forwardedProps = _.omit(props, ['source', 'onLoad', 'session', 'isAuthTokenRequired']); | ||
if (typeof source === 'object' && 'uri' in source && source.uri) { | ||
JKobrynski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
RNImage.getSize(source.uri, (width, height) => { | ||
onLoad({nativeEvent: {width, height}}); | ||
}); | ||
} | ||
}, [onLoad, source, isOffline]); | ||
|
||
return ( | ||
<RNImage | ||
|
@@ -56,21 +52,19 @@ function Image(props) { | |
); | ||
} | ||
|
||
function imagePropsAreEqual(prevProps, nextProps) { | ||
function imagePropsAreEqual(prevProps: ImageOwnProps, nextProps: ImageOwnProps) { | ||
return prevProps.source === nextProps.source; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB but idk why this function isn't just inlined as an arrow function since it's so simple |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is unrelated to this PR, but the Image will not re-render if styles or other props change. @kowczarz, Could you share your insights on comparing only the source in the |
||
|
||
Image.propTypes = imagePropTypes; | ||
Image.defaultProps = defaultProps; | ||
|
||
const ImageWithOnyx = React.memo( | ||
withOnyx({ | ||
withOnyx<ImageProps, ImageOnyxProps>({ | ||
session: { | ||
key: ONYXKEYS.SESSION, | ||
}, | ||
})(Image), | ||
imagePropsAreEqual, | ||
); | ||
ImageWithOnyx.resizeMode = RESIZE_MODES; | ||
|
||
ImageWithOnyx.displayName = 'Image'; | ||
|
||
export default ImageWithOnyx; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import type {ImageSource} from 'expo-image'; | ||
import type {ImageRequireSource, ImageResizeMode, ImageStyle, ImageURISource, StyleProp} from 'react-native'; | ||
import type {OnyxEntry} from 'react-native-onyx'; | ||
import type {Session} from '@src/types/onyx'; | ||
|
||
type ExpoImageSource = ImageSource | number | ImageSource[]; | ||
|
||
type ImageOnyxProps = { | ||
/** Session info for the currently logged in user. */ | ||
session: OnyxEntry<Session>; | ||
}; | ||
|
||
type ImageOnLoadEvent = { | ||
nativeEvent: { | ||
width: number; | ||
height: number; | ||
}; | ||
}; | ||
|
||
type ImageOwnProps = { | ||
/** Styles for the Image */ | ||
style?: StyleProp<ImageStyle>; | ||
|
||
/** The static asset or URI source of the image */ | ||
source: ExpoImageSource | Omit<ImageURISource, 'cache'> | ImageRequireSource | undefined; | ||
|
||
/** Should an auth token be included in the image request */ | ||
isAuthTokenRequired?: boolean; | ||
|
||
/** How should the image fit within its container */ | ||
resizeMode?: ImageResizeMode; | ||
|
||
/** Event for when the image begins loading */ | ||
onLoadStart?: () => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB but wouldn't |
||
|
||
/** Event for when the image finishes loading */ | ||
onLoadEnd?: () => void; | ||
|
||
/** Error handler */ | ||
onError?: () => void; | ||
|
||
/** Event for when the image is fully loaded and returns the natural dimensions of the image */ | ||
onLoad?: (event: ImageOnLoadEvent) => void; | ||
|
||
/** Progress events while the image is downloading */ | ||
onProgress?: () => void; | ||
}; | ||
|
||
type ImageProps = ImageOnyxProps & ImageOwnProps; | ||
|
||
export type {ImageOwnProps, ImageOnyxProps, ImageProps, ExpoImageSource, ImageOnLoadEvent}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safe change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and it didn't cause any issues, and also null is not supported