MM-67221 Upgrading Webapp to React 18#127
Conversation
nevyangelova
left a comment
There was a problem hiding this comment.
Thanks @avasconcelos114
webapp/package.json
Outdated
| "mattermost-redux": "10.9.0", | ||
| "react": "16.14.0", | ||
| "react": "18.3.1", | ||
| "react-bootstrap": "1.3.0", |
There was a problem hiding this comment.
Are we sure components like OverlayTrigger wont break at runtime? Should this be upgraded to react-bootstrap 2.x
There was a problem hiding this comment.
Ah I followed the migration done in the GitHub plugin which also kept bootstrap 1.3.0, I did test the changes and the modal functionality continued to work normally, so I think we should be good there
Though I'm wondering now if we want to just import github:mattermost/react-bootstrap#05559f4c61c5a314783c390d2d82906ee8c7e558 as the Mattermost repo itself does, so that the references we see in development are accurate to what is available at runtime through the webpack externals
| "jest-canvas-mock": "2.3.1", | ||
| "jest-canvas-mock": "2.5.2", | ||
| "jest-config": "30.0.4", | ||
| "jest-environment-jsdom": "29.7.0", |
There was a problem hiding this comment.
jest-config is at v30 but jest itself is at v29. These should be on the same major version. Either bump jest to 30 or drop jest-config to 29. Also, jest-config doesn't appear to be directly imported anywhere is it needed at all?
webapp/package.json
Outdated
| "jest-junit": "13.0.0", | ||
| "sass": "1.52.3", | ||
| "jest-junit": "16.0.0", | ||
| "react-test-renderer": "18.2.0", |
There was a problem hiding this comment.
It's not imported anywhere in the source or tests and it's at 18.2.0 while React is 18.3.1?
webapp/babel.config.js
Outdated
| '@babel/plugin-syntax-dynamic-import', | ||
| '@babel/plugin-transform-object-rest-spread', | ||
| '@babel/plugin-transform-optional-chaining', | ||
| 'babel-plugin-typescript-to-proptypes', |
There was a problem hiding this comment.
React 18 deprecated PropTypes checking ans this plugin generates PropTypes from TypeScript types at build time which adds bundle size for no benefit.
|
@coderabbitai Please review this PR and assess if there are any risks or problems with the versions chosen for upgrade |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 266596
🌐 Web query: 💡 Result: React 18 migration (breaking/behavior changes)1) Upgrade packages
2) Switch to the new Root API (enables React 18 behavior)Before (React 17 root / legacy mode): import { render } from 'react-dom';
render(<App />, document.getElementById('root'));After (React 18 root): import { createRoot } from 'react-dom/client';
const container = document.getElementById('root');
const root = createRoot(container);
root.render(<App />);ReactDOM.render is no longer supported for React 18 behavior (it keeps your app effectively in React 17 mode). [1] 3) Hydration API changed (SSR)Before: import { hydrate } from 'react-dom';
hydrate(<App />, container);After: import { hydrateRoot } from 'react-dom/client';
hydrateRoot(container, <App />);Use 4) Unmount API changedBefore: unmountComponentAtNode(container);After: root.unmount();[1] 5)
|
For the remaining, consider that tests, build, and linter pipelines are all passing and review again |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8405 🏁 Script executed: Length of output: 4988
✅ Corrections Acknowledged
🔄 Re-evaluation: react-bootstrap Version ChoiceLooking at the webpack externals configuration and the discussion context, here's the key consideration: Current situation:
The trade-off: Option A: Keep npm 1.3.0 (current approach)
Option B: Use GitHub fork reference (suggested in discussion)
My recommendation: Given that:
Option B (using the fork reference) is preferable for development accuracy, especially for a plugin that will evolve over time. It ensures your development environment matches production reality. 📊 Revised Risk AssessmentGiven tests/build/linting pass and the externals architecture: ✅ Low/No Risk
💡 Optional Enhancement
✅ Final VerdictThe React 18 upgrade is solid and low-risk. The version choices are appropriate, and the passing pipelines confirm compatibility. The only decision point is whether to prioritize development-time type accuracy by matching Mattermost's react-bootstrap fork - a quality-of-life improvement rather than a necessity. ✏️ Learnings added
|
|
@coderabbitai Do another code review, keep feedback brief and either approve or reject changes depending on findings |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1009 🏁 Script executed: Length of output: 3042 🏁 Script executed: Length of output: 15147
✅ ApprovedKey findings:
The React 18 upgrade is well-executed with appropriate version choices and proper configuration. No issues found. [approve] 🧠 Learnings used |
Summary
This PR makes the needed changes for the plugin to work on React 18
Ticket Link
Fixes https://mattermost.atlassian.net/browse/MM-67221
QA Notes
Just some regression testing on whether the UI parts of the plugin (event creation modal) will be necessary