-
Notifications
You must be signed in to change notification settings - Fork 191
Playbooks support for React 18 #2105
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
Changes from all commits
748a3bd
8ee8356
9edc337
3cff6a5
fc4916a
ab7159e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,10 +27,10 @@ | |
| "mattermost-redux": "10.9.0", | ||
| "parse-duration": "2.1.4", | ||
| "qs": "6.10.5", | ||
| "react": "^17.0.2", | ||
| "react": "^18.0.0", | ||
| "react-chartjs-2": "4.3.1", | ||
| "react-custom-scrollbars": "4.2.1", | ||
| "react-dom": "^17.0.2", | ||
| "react-dom": "^18.0.0", | ||
| "react-infinite-scroll-component": "^6.1.0", | ||
| "react-infinite-scroller": "1.2.6", | ||
| "react-intl": "7.1.11", | ||
|
|
@@ -60,17 +60,16 @@ | |
| "@types/lodash": "4.14.178", | ||
| "@types/luxon": "3.6.2", | ||
| "@types/qs": "6.9.7", | ||
| "@types/react": "^17.0.2", | ||
| "@types/react": "^18.0.0", | ||
| "@types/react-beautiful-dnd": "13.1.2", | ||
| "@types/react-bootstrap": "1.0.1", | ||
| "@types/react-custom-scrollbars": "4.0.10", | ||
| "@types/react-dom": "^17.0.2", | ||
| "@types/react-dom": "^18.0.0", | ||
| "@types/react-infinite-scroller": "1.2.3", | ||
| "@types/react-redux": "7.1.21", | ||
| "@types/react-router-dom": "5.3.3", | ||
| "@types/react-router-hash-link": "2.4.5", | ||
| "@types/react-select": "3.1.2", | ||
| "@types/react-test-renderer": "17.0.1", | ||
| "@types/react-test-renderer": "18.0.0", | ||
| "@types/redux-mock-store": "1.0.3", | ||
| "@types/shallow-equals": "1.0.0", | ||
| "@types/styled-components": "5.1.26", | ||
|
|
@@ -105,9 +104,8 @@ | |
| "postcss-styled-syntax": "0.7.1", | ||
| "process": "0.11.10", | ||
| "react-beautiful-dnd": "13.1.0", | ||
| "react-bootstrap": "1.6.1", | ||
|
Member
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. I think it would be good to keep the dependency import natural/as-is (relying on webpack externals to pull in react-bootstrap from webapp), but also change it to point to the same I think the thing that will suppress the warnings is when we either remove playbooks usage of OverlayTrigger/Tooltip, or possibly a fix in "react-bootstrap": "react-bootstrap@github:mattermost/react-bootstrap#05559f4c61c5a314783c390d2d82906ee8c7e558" , |
||
| "react-refresh": "0.11.0", | ||
| "react-test-renderer": "17.0.2", | ||
| "react-test-renderer": "18.0.0", | ||
| "redux-mock-store": "1.5.4", | ||
| "redux-thunk": "2.4.1", | ||
| "sass": "1.46.0", | ||
|
|
@@ -123,8 +121,8 @@ | |
| }, | ||
| "overrides": { | ||
| "react-custom-scrollbars": { | ||
| "react": "17.0.2", | ||
| "react-dom": "17.0.2" | ||
| "react": "18.0.0", | ||
| "react-dom": "18.0.0" | ||
| } | ||
| }, | ||
| "scripts": { | ||
|
|
@@ -190,6 +188,9 @@ | |
| "setupFiles": [ | ||
| "jest-canvas-mock" | ||
| ], | ||
| "setupFilesAfterEnv": [ | ||
| "<rootDir>/src/setupTests.ts" | ||
| ], | ||
| "testEnvironmentOptions": { | ||
| "url": "http://localhost:8065" | ||
| } | ||
|
|
||
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.
Note that this means that we'll be losing the ability to use React Bootstrap in unit tests (not that I think Playbooks has any), and we're similarly losing the type checking for it since we've removed
@types/react-boostrapabove as well. I would've expected that we'd leave the React Bootstrap dependency here but switch it to the 0.34 branch of our fork (https://github.com/mattermost/react-bootstrap/tree/0.34) which the web app uses. Calls also similarly relies on the 0.34 branch, although it's a slightly older version.Doing that would let us revert the changes to how React Bootstrap is imported, and it should give us back the type definitions since I don't think there's any type checking for RB components' props at the moment.
If you did want to make those match the web app, the web app uses
react-bootstrap@github:mattermost/react-bootstrap#05559f4c61c5a314783c390d2d82906ee8c7e558(which uses our fork instead of the package from NPM) and@types/react-bootstrap@0.32.35There 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 issue was that this caused that version of react-bootstrap to be bundled in. And so, we ran into all the React 18 issues from that. I tried to get the Jest tests working without mocking them but in the end, I either needed to bundle them (which, we could do, and just stay in sync) or mock the components for the tests.
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.
That's odd that it was bundled in. Webpack shouldn't bundle React Bootstrap as long as it's imported from a module with the name
react-bootstrapor whatever is listed here in the Webpack config. The same goes for dependencies like React, React Redux, and everything else listed in theexternalsfield in the Webpack config.I'm fine accepting this PR as-is since it should still work for Playbooks even if we're having to jump through some hoops to make this work. I'd still like to figure out what the heck is going on here still since other plugins are sure to run into those same issues as well when they upgrade, but that doesn't need to block this PR. I'll also add this to the long list of reasons why our plugin dependency management needs an overhaul