- 
                Notifications
    You must be signed in to change notification settings 
- Fork 169
Update dependencies for React 18 #2114
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
base: master
Are you sure you want to change the base?
Conversation
NPM seems to have a hard time applying these overrides without first removing these and then re-adding them afterwards.
``` npm add [email protected] [email protected] @types/[email protected] @types/[email protected] react-bootstrap@github:mattermost/react-bootstrap#05559f4c61c5a314783c390d2d82906ee8c7e558 @types/[email protected] [email protected] [email protected] [email protected] mattermost-redux @types/[email protected] [email protected] [email protected] [email protected] @testing-library/[email protected] [email protected] ```
ActionFuncAsync shouldn't be be used in these places because the functions passed through these props had already been dispatched, so the code using them would only see the results of the action. This is similar to a change we made a couple years ago in the web app to standardize the types used for Redux actions.
I have no idea why, but having these be interfaces prevented us from dispatching them because they couldn't be used as an UnknownAction. For some reason, you can't pass an arbitrary interface as an UnknownAction because it has an index signature, but TS will accept an arbitrary object just fine.
| "lodash": "4.17.21", | ||
| "luxon": "3.6.1", | ||
| "mattermost-redux": "10.9.0", | ||
| "mattermost-redux": "11.1.0-2", | 
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 updated mattermost-redux and the client/type packages because I forgot that mattermost-redux had a dependency on a specific major version of Redux. I cut these adhoc releases off of the latest 11.1 branch of the monorepo, and we can replace them with proper ones after 11.1.0 releases
| "qs": "6.10.5", | ||
| "react": "^17.0.2", | ||
| "react": "18.2.0", | ||
| "react-beautiful-dnd": "13.1.1", | 
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.
This moved from being a devDependency despite being bundled with Playbooks since it's not exposed by the web app
| "jest-canvas-mock": "2.3.1", | ||
| "jest-environment-jsdom": "29.7.0", | ||
| "jest-junit": "13.0.0", | ||
| "patch-package": "8.0.1", | 
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.
This is temporary until I either merge mattermost/react-bootstrap#6 or figure out a better way to modify its dependency without it
| "process": "0.11.10", | ||
| "react-beautiful-dnd": "13.1.0", | ||
| "react-bootstrap": "1.6.1", | ||
| "react-bootstrap": "github:mattermost/react-bootstrap#05559f4c61c5a314783c390d2d82906ee8c7e558", | 
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.
As mentioned elsewhere, this is only used during development, but it'll now match the web app's version instead of accidentally depending on a newer version of React Bootstrap
| "webpack-dev-server": "4.15.1" | ||
| }, | ||
| "overrides": { | ||
| "@testing-library/react-hooks": { | 
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 didn't upgrade a few of these dependencies because newer versions required some further changes.
This library doesn't support React 18, but it seems to work fine for now. It needs to be replaced with @testing-library/react which has some breaking changes to renderHook
| "react": "$react", | ||
| "react-dom": "$react-dom" | 
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.
react-custom-scrollbars doesn't support React 17 or higher, so it will need to be replaced down the line. In the web app, I replaced it with simplebar-react (mattermost/mattermost#33783)
| "react-input-autosize": { | ||
| "react": "$react" | ||
| }, | ||
| "react-select": { | ||
| "react": "$react", | ||
| "react-dom": "$react-dom" | 
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.
The web app uses a newer version of React Select (5.9.0) which supports React 18 and has some accessibility improvements, but upgrading requires some breaking changes.
React Select 5 also removes the dependency on react-input-autosize, so we can remove that when React Select is upgraded
| delayShow={0} | ||
| delayHide={1000} | 
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.
These might be a functional change because the API used before wouldn't have worked with the provided version of React Bootstrap as best as I can tell.
We've stopped using these components instead of Floating UI in the web app though because we suspected that OverlayTrigger had a memory leak
| const getProfiles = useMemo(() => makeGetProfilesByIdsAndUsernames(), []); | ||
| const usersInSubset = useSelector((state: GlobalState) => getProfiles(state, {allUserIds: props.userGroups?.subsetUserIds || [], allUsernames: []})); | 
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 did a bit extra here to fix an incorrectly memoized selector
| export const PUBLISH_TEMPLATES = (manifest.id + '_PUBLISH_TEMPLATES').toUpperCase(); | ||
|  | ||
| export interface ReceivedToggleRHSAction { | ||
| export type ReceivedToggleRHSAction = { | 
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've got absolutely no idea why, but these types weren't able to be passed into dispatch previously because of some weird behaviour where interfaces aren't compatible with UnknownAction unless those interfaces supported arbitrary fields
Summary
Along with updating the version of React, we also have to update a bunch of libraries that depend on it because the versions we previously used only work on React 18. For the ones that we upgraded in the web app repo, I updated them to match the web app, but for some other ones, I just overrode the dependencies of those dependencies because that seemed to be all that prevented them from working with React 18.
Along with that, there's some minor changes to some types and some small fixes which I'll note below.
Ticket Link
Checklist
Gated by experimental feature flag