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

React 18 #6902

Closed
wants to merge 40 commits into from
Closed

React 18 #6902

wants to merge 40 commits into from

Conversation

tephenavies
Copy link
Member

@tephenavies tephenavies commented Sep 22, 2023

Before merging

What this PR does

Part of #6538.

Upgrade React to 18 and Typescript to 5.

Test me

Can be tested at http://ci.terria.io/react18-sd/

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@tephenavies tephenavies marked this pull request as ready for review September 27, 2023 04:30
@tephenavies
Copy link
Member Author

Tests passing!

@nf-s nf-s mentioned this pull request Oct 3, 2023
@zoran995
Copy link
Contributor

zoran995 commented Oct 4, 2023

Can we also remove import * as React from "react"; to stick to more modern syntax? It also might reduce bundle size and build speed. We can also do it as a follow-up ticket

Copy link
Contributor

@nf-s nf-s left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @steve9164 !

A few things I noticed that aren't working correctly -

Chart expand/download buttons

  • Open "All generation types"
  • Click on point
  • Click on "expand" or "download" buttons in feature info

A few other issues caused by what @zoran995 commented on - showDropdownAsModal is not defined

I agree with @zoran995 on reducing use of import * as React from "react" - it would be great to at least remove all of these from .tsx files

I think we also need to update to the new React Root API - https://react.dev/blog/2022/03/08/react-18-upgrade-guide#updates-to-client-rendering-apis

Until you switch to the new API, your app will behave as if it's running React 17

I believe there is only two usages of ReactDOM.render in https://github.com/TerriaJS/TerriaMap/blob/main/lib/Views/render.jsx#L9

lib/ReactViews/Map/Panels/DropdownPanel.jsx Outdated Show resolved Hide resolved
Comment on lines +152 to 164
"react": "^18.2.0",
"react-anything-sortable": "^1.5.2",
"react-color": "^2.19.3",
"react-datepicker": "0.53.0",
"react-dom": "^16.14.0",
"react-dom": "^18.2.0",
"react-ga4": "^2.1.0",
"react-i18next": "^11.18.0",
"react-responsive": "^5.0.0",
"react-select": "^3.1.1",
"react-swipeable": "^5.1.0",
"react-transition-group": "^4.3.0",
"react-uid": "^2.3.0",
"react-uid": "^2.3.3",
"react-virtual": "~2.3.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update any of these other react-based libraries?

Copy link
Contributor

@zoran995 zoran995 Oct 16, 2023

Choose a reason for hiding this comment

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

IMO there are some dependencies that probably should be upgraded as they list older versions of react as peer dependencies (quick check can be done via npx check-peer-dependencies --yarn)

  • @visx/* should be upgraded to at least v2.10.0, it is the first version that lists react 18 as dependency
  • html-to-react version used has peer dependency on react 16/17, newer versions are on react 18
  • react-anything-sortable
  • react-datepicker
  • react-responsive
  • react-select
  • react-swipeable
  • react-virtual (I think the only issue with the upgrade is the wrong usage of createRef in functional component and useRef should be used instead b6390c5)
  • react-shallow-testutils

Copy link
Member Author

@tephenavies tephenavies Oct 30, 2023

Choose a reason for hiding this comment

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

It's unlikely that the libraries are incompatible with React 18, and more likely that library authors are using version strings such as

"react": "^16.0 || ^17.0"

when they should use the standard:

"react": "^0.13.0 || ^0.14.0 || >=15"

but I'll check the usage of those libraries

Copy link
Member Author

Choose a reason for hiding this comment

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

A library that definitely needs updating is mobx-react

Copy link
Contributor

Choose a reason for hiding this comment

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

@steve9164 there will be issues with libraries listing react in peerDependencies as

"react": "^16.0 || ^17.0"

if we are on react 18 after upgrading to the new version of the node which comes with a new npm that automatically installs and checks peerDependencies. You can see the error occurring in #6986

package.json Show resolved Hide resolved
@tephenavies tephenavies marked this pull request as draft November 14, 2023 07:16
@nf-s nf-s mentioned this pull request Nov 15, 2023
@tephenavies tephenavies mentioned this pull request Dec 3, 2023
4 tasks
@nf-s nf-s mentioned this pull request Dec 7, 2023
@tephenavies
Copy link
Member Author

Breaking into smaller PRs. Starting with #7050

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants