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.

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

Merged
merged 51 commits into from
Mar 1, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
f67e753
migrate resizeModes to TypeScript
JKobrynski Nov 13, 2023
ba4eda8
start migrating Image to TypeScript
JKobrynski Nov 13, 2023
98bd9e1
migrate index.js to TypeScript
JKobrynski Nov 13, 2023
1e95421
migrate index.native.js to TypeScript
JKobrynski Nov 14, 2023
ed90b35
remove unnecessary js files
JKobrynski Nov 14, 2023
c1d0c2c
extract map type
JKobrynski Nov 14, 2023
b423c06
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Nov 14, 2023
46c9f92
bring back optional props
JKobrynski Nov 14, 2023
0041ba8
destructure props, add return types
JKobrynski Nov 14, 2023
b446609
extract source type, improve if statements
JKobrynski Nov 15, 2023
21c7980
change Image prop types names
JKobrynski Nov 15, 2023
f115914
bring back imageSource to if statement
JKobrynski Nov 15, 2023
e2508f9
remove resolveDimensions from native Image
JKobrynski Nov 17, 2023
3f260bc
remove references to Image.resizeModes
JKobrynski Nov 17, 2023
3a76fb0
remove RESIZE_MODES from Image
JKobrynski Nov 20, 2023
247dfff
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Nov 27, 2023
02daaf5
remove unnecessary comment
JKobrynski Nov 27, 2023
0e11fb8
remove Image.resizeMode
JKobrynski Nov 27, 2023
31aa36a
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Jan 15, 2024
06c374c
adjust types to expo-image
JKobrynski Jan 15, 2024
e9451f0
fix source type
JKobrynski Jan 16, 2024
a695adb
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Jan 18, 2024
ee81360
bring back sourcePropTypes directory
JKobrynski Jan 18, 2024
9ceb2eb
fix type errors in Avatar.tsx
JKobrynski Jan 18, 2024
50726c7
remove unnecessary comment
JKobrynski Jan 18, 2024
4c69801
simplify source related if statements
JKobrynski Jan 18, 2024
3f5550f
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 2, 2024
1056c9f
fix headers type issue
JKobrynski Feb 5, 2024
b13fce2
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 5, 2024
611227e
migrate index.native.js to TypeScript
JKobrynski Feb 5, 2024
7b6ce98
fix onLoad method typing
JKobrynski Feb 6, 2024
6cc06a2
fix source type mismatches
JKobrynski Feb 6, 2024
e24eef6
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 6, 2024
3b958ba
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 6, 2024
7ba47c8
add default props
JKobrynski Feb 7, 2024
403dc95
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 7, 2024
080dbf4
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 8, 2024
747f7f5
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 19, 2024
4851245
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 20, 2024
2f780c6
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 21, 2024
9dcfc34
fix image source bug
JKobrynski Feb 21, 2024
0ef3bf5
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 22, 2024
171e45a
destructure props in function declaration
JKobrynski Feb 22, 2024
5d125cf
fix types
JKobrynski Feb 22, 2024
d1e55cc
revert ios related changes
JKobrynski Feb 23, 2024
1e582b6
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 23, 2024
065c27c
revert tmp.xcconfig
JKobrynski Feb 23, 2024
05919f2
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Feb 29, 2024
cc78940
fix type errors
JKobrynski Feb 29, 2024
c0f10cd
fill the dependency array in MoneyRequestCOnfirmationList
JKobrynski Feb 29, 2024
3476ef1
Merge branch 'main' into migrateImageToTypeScript
JKobrynski Mar 1, 2024
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
51 changes: 0 additions & 51 deletions src/components/Image/imagePropTypes.js

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,35 +1,35 @@
import lodashGet from 'lodash/get';
import React from 'react';
import RNFastImage from 'react-native-fast-image';
import {ImageRequireSource, ImageURISource} from 'react-native';
import RNFastImage, {Source} from 'react-native-fast-image';
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 {DimensionsCacheValue, ImageOnyxProps, ImagePropsWithOnyx} from './types';

const dimensionsCache = new Map();
const dimensionsCache = new Map<string, DimensionsCacheValue>();

function resolveDimensions(key) {
function resolveDimensions(key: string) {
return dimensionsCache.get(key);
JKobrynski marked this conversation as resolved.
Show resolved Hide resolved
}

function Image(props) {
function Image(props: ImagePropsWithOnyx) {
// eslint-disable-next-line react/destructuring-assignment
const {source, isAuthTokenRequired, session, ...rest} = props;

JKobrynski marked this conversation as resolved.
Show resolved Hide resolved
let imageSource = source;
if (source && source.uri && typeof source.uri === 'number') {
let imageSource: Omit<ImageURISource, 'cache'> | ImageRequireSource | Source = source;
if (typeof source !== 'number' && typeof source.uri === 'number') {
JKobrynski marked this conversation as resolved.
Show resolved Hide resolved
imageSource = source.uri;
JKobrynski marked this conversation as resolved.
Show resolved Hide resolved
}
if (typeof imageSource !== 'number' && isAuthTokenRequired) {
const authToken = lodashGet(props, 'session.encryptedAuthToken', null);
if (typeof source !== 'number' && isAuthTokenRequired) {
const authToken = props.session?.encryptedAuthToken ?? null;
JKobrynski marked this conversation as resolved.
Show resolved Hide resolved
imageSource = {
...source,
headers: authToken
? {
[CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken,
}
: null,
: undefined,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Safe change?

Copy link
Contributor Author

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

}

Expand All @@ -39,6 +39,9 @@ function Image(props) {
{...rest}
source={imageSource}
onLoad={(evt) => {
if (typeof source === 'number' || !source.uri) {
return;
}
const {width, height} = evt.nativeEvent;
dimensionsCache.set(source.uri, {width, height});
if (props.onLoad) {
Expand All @@ -49,15 +52,14 @@ function Image(props) {
);
}

Image.propTypes = imagePropTypes;
Image.defaultProps = defaultProps;
Image.displayName = 'Image';
const ImageWithOnyx = withOnyx({
Image.resizeMode = RESIZE_MODES;
Image.resolveDimensions = resolveDimensions;
JKobrynski marked this conversation as resolved.
Show resolved Hide resolved

JKobrynski marked this conversation as resolved.
Show resolved Hide resolved
const ImageWithOnyx = withOnyx<ImagePropsWithOnyx, ImageOnyxProps>({
session: {
key: ONYXKEYS.SESSION,
},
})(Image);
ImageWithOnyx.resizeMode = RESIZE_MODES;
ImageWithOnyx.resolveDimensions = resolveDimensions;

Comment on lines -60 to -61
Copy link
Contributor

Choose a reason for hiding this comment

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

Safe to remove these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)

Copy link
Contributor

Choose a reason for hiding this comment

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

const dimensionsCache = new Map(); should also be removed - it's not exposed externally and it's only used to set items internally, but never get them. I've raised this subject before and it seems the code and related functionality are leftovers.
I've removed the same blocks of code in my RP as well (#36560)

Copy link
Contributor

Choose a reason for hiding this comment

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

not going to block on this because @kidroca is addressing the dimensionsCache in a separate PR

export default ImageWithOnyx;
35 changes: 15 additions & 20 deletions src/components/Image/index.js → src/components/Image/index.tsx
Original file line number Diff line number Diff line change
@@ -1,25 +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 ONYXKEYS from '@src/ONYXKEYS';
import {defaultProps, imagePropTypes} from './imagePropTypes';
import RESIZE_MODES from './resizeModes';
import {ImageOnyxProps, ImageProps, ImagePropsWithOnyx} from './types';

function Image(props) {
const {source: propsSource, isAuthTokenRequired, onLoad, session} = props;
function Image(props: ImagePropsWithOnyx) {
const {source: propsSource, isAuthTokenRequired, onLoad, session, ...forwardedProps} = props;
/**
JKobrynski marked this conversation as resolved.
Show resolved Hide resolved
* Check if the image source is a URL - if so the `encryptedAuthToken` is appended
* to the source.
*/
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)}`};
// 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 = session?.encryptedAuthToken ?? null;

if (isAuthTokenRequired && authToken && typeof propsSource !== 'number') {
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.
Expand All @@ -33,17 +32,14 @@ function Image(props) {
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) {
if (onLoad == null || typeof source === 'number' || !source.uri) {
return;
JKobrynski marked this conversation as resolved.
Show resolved Hide resolved
}
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']);

JKobrynski marked this conversation as resolved.
Show resolved Hide resolved
return (
<RNImage
// eslint-disable-next-line react/jsx-props-no-spreading
Expand All @@ -53,21 +49,20 @@ function Image(props) {
);
}

function imagePropsAreEqual(prevProps, nextProps) {
function imagePropsAreEqual(prevProps: ImageProps, nextProps: ImageProps) {
return prevProps.source === nextProps.source;
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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 imagePropsAreEqual function?


Image.propTypes = imagePropTypes;
Image.defaultProps = defaultProps;
Image.resizeMode = RESIZE_MODES;
Image.displayName = 'Image';
JKobrynski marked this conversation as resolved.
Show resolved Hide resolved

const ImageWithOnyx = React.memo(
withOnyx({
withOnyx<ImagePropsWithOnyx, ImageOnyxProps>({
session: {
key: ONYXKEYS.SESSION,
},
})(Image),
imagePropsAreEqual,
);
ImageWithOnyx.resizeMode = RESIZE_MODES;

export default ImageWithOnyx;
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ const RESIZE_MODES = {
cover: 'cover',
stretch: 'stretch',
center: 'center',
};
} as const;

export default RESIZE_MODES;
11 changes: 0 additions & 11 deletions src/components/Image/sourcePropTypes/index.js

This file was deleted.

10 changes: 0 additions & 10 deletions src/components/Image/sourcePropTypes/index.native.js

This file was deleted.

48 changes: 48 additions & 0 deletions src/components/Image/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import {ImageRequireSource, ImageResizeMode, ImageStyle, ImageURISource, StyleProp} from 'react-native';
import {ImageStyle as FastImageStyle, OnLoadEvent, ResizeMode, Source} from 'react-native-fast-image';
import {OnyxEntry} from 'react-native-onyx';
import {Session} from '@src/types/onyx';

type ImageOnyxProps = {
/* Onyx Props */
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment unnecessary, the name of type speaks for itself.

Suggested change
/* Onyx Props */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/** Session info for the currently logged in user. */
session: OnyxEntry<Session>;
};

type ImageProps = {
JKobrynski marked this conversation as resolved.
Show resolved Hide resolved
/** Styles for the Image */
style?: StyleProp<ImageStyle & FastImageStyle>;

/** The static asset or URI source of the image */
source: Omit<ImageURISource, 'cache'> | ImageRequireSource | Omit<Source, 'cache'>;

/** Should an auth token be included in the image request */
isAuthTokenRequired?: boolean;

/** How should the image fit within its container */
resizeMode?: ImageResizeMode & ResizeMode;

/** Event for when the image begins loading */
onLoadStart?: () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB but wouldn't unknown make more sense than void here (and for onLoadEnd, onError, onLoad, onProgress) since we just don't know or care about the return type?


/** 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: OnLoadEvent) => void;

/** Progress events while the image is downloading */
onProgress?: () => void;
};

type ImagePropsWithOnyx = ImageOnyxProps & ImageProps;

type DimensionsCacheValue = {
width: number;
height: number;
};

export type {ImageProps, ImageOnyxProps, ImagePropsWithOnyx, DimensionsCacheValue};
Loading