Update dependencies and upgrade to RN 0.76.5 #8421
Conversation
enahum
added
2: Dev Review
| @@ -60,7 +60,7 @@ const BookmarkIcon = ({emoji, emojiSize, emojiStyle, file, genericStyle, iconSiz | |||
| name='book-outline' | |||
| size={22} | |||
| color={theme.centerChannelColor} | |||
| style={genericStyle} | |||
| style={genericStyle as StyleProp<TextStyle>} | |||
There was a problem hiding this comment.
In several files we are doing this, and the prop is only used to then be casted. Shouldn't we change the type directly in the prop?
|
|
||
| useEffect(() => { | ||
| if (files.length) { | ||
| containerHeight.value = CONTAINER_HEIGHT_MAX; | ||
| return; | ||
| } | ||
| containerHeight.value = CONTAINER_HEIGHT_MIN; | ||
| }, [files.length > 0]); | ||
| }, [containerHeight, files.length]); |
There was a problem hiding this comment.
Nitpick:
If we create a new variable hasFiles, we might not run this effect so often, and may be slightly more intention revealing. (Not that this is neither hard to understand or has any performance issue though)
const hasFiles = Boolean(files.length);
useEffect(() => {
if (hasFiles) {...}
...
}, [containerHeight, hasFiles]);
| @@ -13,7 +13,7 @@ import {typography} from '@utils/typography'; | |||
|
|
|||
| type DateSeparatorProps = { | |||
| date: number | Date; | |||
| style?: StyleProp<ViewStyle>; | |||
| style?: StyleProp<TextStyle>; | |||
There was a problem hiding this comment.
In order to make this type safe, you can use the Intersection utility type we have like this:
style?: StyleProp<Intersection<TextStyle, ViewStyle>>;
That way, we don't need the casting, and we make sure we don't pass styles that may create issues.
| @@ -130,7 +130,7 @@ const PostList = ({ | |||
| return false; | |||
| } | |||
| return posts[0]?.id !== lastPostId; | |||
| }, [posts[0]?.id, lastPostId]); | |||
| }, [lastPostId, posts]); | |||
There was a problem hiding this comment.
Probably this memo is not needed at all (it is memoizing a boolean value and doing a pretty fast operation). But 0/5 on whether it is desirable to change it in this PR.
| "cursor": undefined, | ||
| }, | ||
| ] | ||
| } |
There was a problem hiding this comment.
A bit surprised of these snapshots changing. Do you know where is this change coming from?
There was a problem hiding this comment.
this can be caused by the upgrade of react-native-gesture-handler library or the update to react-native or react, this is defo not done by us.
| afterAll(() => { | ||
| // jest.resetAllMocks(); |
There was a problem hiding this comment.
Is this no longer needed? If so, should we just remove it?
app/utils/errors.ts
Outdated
| @@ -70,15 +70,15 @@ export const getFullErrorMessage = (error: unknown, intl?: IntlShape, depth = 0) | |||
| return `${message}; ${getFullErrorMessage(error.details, intl, depth + 1)}`; | |||
| } | |||
|
|
|||
| return message; | |||
| return message!; | |||
There was a problem hiding this comment.
What is causing us to need this? Same on line 81. We should add some default value instead of using the !.
Summary
Upgrade all dependencies (the app should be tested in its entirety) and upgraded to React Native 0.76.5, still blocked to switch to the new arch by the navigation library
Ticket Link
https://mattermost.atlassian.net/browse/MM-60562
Release Note