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

fix: use correct prop type in BaseInvertedFlatlist component #21668

Merged
merged 2 commits into from
Jun 28, 2023
Merged

fix: use correct prop type in BaseInvertedFlatlist component #21668

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c604a4f
fix: use correct prop type in BaseInvertedFlatlist component
allroundexperts Jun 27, 2023
6893686
fix: use native flatlist for type in BaseInvertedFlatList
allroundexperts Jun 28, 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
2 changes: 1 addition & 1 deletion src/components/InvertedFlatList/BaseInvertedFlatList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const propTypes = {
initialRowHeight: PropTypes.number.isRequired,

/** Passed via forwardRef so we can access the FlatList ref */
innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.shape({current: PropTypes.instanceOf(FlatList)})]).isRequired,
innerRef: PropTypes.oneOfType([PropTypes.func, PropTypes.shape({current: PropTypes.instanceOf(React.Component)})]).isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we didn't use PropTypes.instanceOf(BaseFlatList) in the end?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we are using 'React.Component' at many of the forwarded ref component. So it should be fine to replicate the same to have consistency. I am also fine to use to this common refPropTypes created recently https://github.com/Expensify/App/blob/main/src/components/refPropTypes.js#L3

In the approval comment I had mentioned the above #20591 (comment)

It seems at multiple places in the app we are using the same behaviour. Kindly do let me know if we want to change this behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I think we should try to have our prop types as tight as possible. This reference is used here for example:

App/src/libs/ReportScrollManager/index.js

Line 19 in 70bf90d

flatListRef.current.scrollToIndex(index);

If you start passing around the type Component instead of BaseFlatList, typescript won't be able to find scrollToIndex in Component.

I'm not sure if this complicates things to much, then another option could be to leave this type problems for when we migrate to typescript 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Passing Component around when you know it is a FlatList (or BaseFlatList) feels to me like using PropTypes.any or PropTypes.object (without specifying the shape of the object)... it works... but then we are missing the purpose of types (type safety)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aldo-expensify I think the main solution proposed here should solve this issue. We should import and use Flatlist directly from RN.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it works, I think its better to be more precise about the type, so using FlatList (or BaseFlatList if FlatList is not possible) is better IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

@aldo-expensify then we can do the same way as done here from your example

App/src/components/InvertedFlatList/index.js

Lines 3 to 13 in 70bf90d

import {FlatList, StyleSheet} from 'react-native';
import _ from 'underscore';
import BaseInvertedFlatList from './BaseInvertedFlatList';
import styles from '../../styles/styles';
const propTypes = {
/** Passed via forwardRef so we can access the FlatList ref */
innerRef: PropTypes.shape({
current: PropTypes.instanceOf(FlatList),
}).isRequired,

FlatList is imported to mention it as type. I highly doubt the current variable will be of type BaseFlatList, it will be of type FlatList as it was the original ref.
If we do this way when we go to typescript it won't change AFAIK.

It is not a complex change and this was also one of the proposals on the same from @allroundexperts
It just I didn't want to add extra import for type and went with a consistent solution existing in the app.

Copy link
Contributor

@abdulrahuman5196 abdulrahuman5196 Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

OOPS. Seems there where same suggestion from @allroundexperts and I also commented the same thing. (Page didn't refresh for me 😮‍💨 )

Yes. it is possible as mentioned here #21668 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok!, yes use FlatList from react-native if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated @aldo-expensify @abdulrahuman5196!


/** Should we measure these items and call getItemLayout? */
shouldMeasureItems: PropTypes.bool,
Expand Down