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.

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

Upgrade to React 18 #8095

Merged
merged 13 commits into from
Jul 4, 2024
Merged

Upgrade to React 18 #8095

merged 13 commits into from
Jul 4, 2024

Conversation

vrigal
Copy link
Collaborator

@vrigal vrigal commented Jun 12, 2024

The migration of React itself is rather straightforward.
The application seems to work as usal (tested with a new frontend build using BACKEND=https://treeherder.allizom.org/).

However dependencies would require a much larger effort to remain maintainable:

@vrigal
Copy link
Collaborator Author

vrigal commented Jun 20, 2024

@Archaeopteryx I replaced react-hot-loader by @pmmmwh/react-refresh-webpack-plugin to test the dev server. I'm not sure if we need to keep the hot loading, it could probably be removed afterwards.

I also had issues with routes (connected-react-router is not compatible).
I can build, but cannot run the dev server for now (no route match).
I'll look at ESlint and the CI jobs later on.

@vrigal
Copy link
Collaborator Author

vrigal commented Jun 20, 2024

I finally decided not to upgrade webpack-dev-server for now (nor react-hot-loader which is not maitained anymore).
Upgrading those is a real topic and is not essential to test react upgrade yet.
This causes a deprecation warning because of a dependecy to webpack-dev-server during build, and a lot of warnings in the console (e.g. Support for defaultProps will be removed from function components in a future major release).

To make ESLint work, I upgraded to 9.x (with configuration file), but had to drop support for eslint-config-airbnb and eslint-plugin-extensions plugins. It works and is sync with pre-commit hook (note that this hook is archived too).

I hope the build works as expected.

@vrigal
Copy link
Collaborator Author

vrigal commented Jun 21, 2024

@Archaeopteryx I updated the tests to pass with React 18. It was quite a while, but all CI jobs should be green now.
Please note that tests currently use the legacyRoot rendering option, which is deprecated and will not work with React >18. Updating the tests to use render could either be a follow up.

Next step on my side is to look at console warnings I suppose.

@vrigal vrigal mentioned this pull request Jun 21, 2024
@La0 La0 requested a review from Archaeopteryx June 24, 2024 09:08
Copy link
Collaborator

@Archaeopteryx Archaeopteryx left a comment

Choose a reason for hiding this comment

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

Thank you. During testing, no issues have been noticed.

The upgrade to ESLint 9 and the removal of the airbnb js style guide causes some reformatting, e.g. puts multiple function arguments into a single line which makes history annotation per line ('blame') noisy. On the other hand it's unknown when then the style guide will support ESLint 9.

Is it a dependency on a different package (which depends on React 18) which ties the upgrade to ESLint 9? Could ESLint 8 and packages which depend on it be kept?

@vrigal vrigal force-pushed the react-18 branch 3 times, most recently from 442d3f5 to f4791fb Compare July 2, 2024 14:04
@vrigal vrigal mentioned this pull request Jul 2, 2024
@vrigal
Copy link
Collaborator Author

vrigal commented Jul 2, 2024

I'm not sure why I upgraded ESLint initially, probably because eslint-config-airbnb depends on eslint-plugin-react.
However it seems to work fine keeping it at version 8.21.0. I moved all the changes related to ESLint on a new PR: #8119.

@Archaeopteryx Archaeopteryx merged commit c227e84 into mozilla:master Jul 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants