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

feat: upgrade react to v18 #876

Merged
merged 3 commits into from
Sep 12, 2024
Merged

feat: upgrade react to v18 #876

merged 3 commits into from
Sep 12, 2024

Conversation

kabaros
Copy link
Contributor

@kabaros kabaros commented Sep 10, 2024

Upgrades the platform and runtime to use react 18. Implements LIBS-630.


Testing setup

Clone this repo and checkout the react-upgrade branch, then we will use yalc to publish a new version of platform and runtime to use in our projects (follow the order described):

app-platform/shell

Go to app-platform/shell (make sure you're on react-upgrade branch) then run yalc publish

image

app-platform/cli

Go to app-platform/cli and add a dependency to app-shell that we just published with yalc add @dhis2/[email protected]

Then edit the files section of package.json to include .yalc, it will look like:

image

Note

this is needed to make sure that the depdency of the dependency is also linked using yalc - see issue for context

app-runtime/runtime

Also clone the app-runtime react-upgrade, then go to app-runtime/runtime and run yalc publish

Test the upgrade of a project

Now navigate to a project to test the upgrade (I tested with data-entry-app, import-export and app-management)

  • Run yalc add @dhis2/[email protected] to replace the version of cli-app-scripts with the locally published one
  • Run yalc add @dhis2/[email protected] (this doesn't seem to be necessary as the peer depdendency is lenient and resolves to react version in the shell?)
  • if this is a project that wasn't upgrade for vite, then run ~/code/dhis/platform/app-platform/cli/bin/d2-app-scripts migrate js-to-jsx (change to the correct path to where you have app-platform cloned)

What to check?

The first sanity checks should be that when you run yarn test, you should see a log that it's using react@18
image

and similarly you should see a console.log in the running app
image

These are your sanity checks that you're running the correct version after all this depdendency madness

Then:

  • run yarn why react and ensure it resolves react@18
  • run yarn start and do a sanity check that the app works
  • run yarn build - you will likely need to run npx yarn-deduplicate before yarn build works (you will get an error about duplication in yarn.lock)
  • make sure you don't get this warning in the browser console ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it’s running React 17
  • any other exploratory testing...

Notes

  • For testing, you should also read this blog post about upgrading to react@18 as it might give clues about things that can break in different apps.
  • One major change in React@18 is automatic batching which would affect places we're doing multiple state updates inside a setTimeout for example - I had a quick look through our code base and didn't find this pattern used, but it's something to look for for people upgrading their apps.

Checklist / Known issues

  • Look at types and react-dom upgrades, and react-testing-library (jest?, anything else..)
  • RTL shell is broken false alarm
  • The UI library is generating this error Support for defaultProps will be removed from function components in a future major release. Use JavaScript default parameters instead - needs a separate update to UI
  • Have written Documentation
  • Has tests coverage

Other Questions / Discussions

  • maybe dhis/ui should be a peer dependency of app-shell? to avoid deduping
  • UI library seems to work without changes since we have react as peer depdendency
    • how come it's working?? the peer dependency is ^16.8 but it's not complaining about react@18
    • we only have a direct dependency to react at the top level @dhis2/ui-root but that's never used directly, right?
    • Shouldn't the react depdendency in app-shell be a peer depdendency?

Screenshots

image

Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

LGTM, made a few comments - mainly asking if we should either:

  1. enforce ^18 as the peerdep for app-adapter (my current preference, moving forward and breaking at v12
  2. OR specify a union of ^16.8 and ^18 for shell dependencies as well

Either is fine with me, and we can refine after it's in alpha if that's easier

@@ -44,7 +43,7 @@
"classnames": "^2",
"moment": "^2",
"prop-types": "^15",
"react": "^16.8",
"react": "^16.8 || ^18",
Copy link
Member

Choose a reason for hiding this comment

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

Should this be just ^18? If not, maybe all dependencies (including shell, which isn't a peerdep) should use the union ^16.8 || ^18? That would allow an app to specify ^16.8 and yarn would resolve to a single React version 16.8 since that would satisfy both. Otherwise if we're forcing a minimum version of 18 we could say ^18 and drop the ^16.8 in this peer dep?

Maybe easier to explain on a quick call or chat next week. It may be fine to leave this as-is, just curious what the reasoning is.

Copy link
Contributor Author

@kabaros kabaros Sep 12, 2024

Choose a reason for hiding this comment

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

yeah it should have been just ^18, updated it now - my reasoning was related to my question on why react is not a peer dependency of the shell. I thought the shell and adapter could live with either react@16 or 18, but that's not possible anyhow as the changes in the shell would only work in React@18 and it is the shell that provides react to consuming apps.

I also marked the commit as BREAKING CHANGE.

shell/src/index.jsx Outdated Show resolved Hide resolved
cli/src/commands/start.js Outdated Show resolved Hide resolved
shell/package.json Show resolved Hide resolved
BREAKING CHANGE: require react version 18
@kabaros kabaros merged commit 77ecf10 into alpha Sep 12, 2024
6 checks passed
@kabaros kabaros deleted the react-upgrade branch September 12, 2024 18:29
dhis2-bot added a commit that referenced this pull request Sep 12, 2024
# [12.0.0-alpha.14](v12.0.0-alpha.13...v12.0.0-alpha.14) (2024-09-12)

### Features

* upgrade react to v18 ([#876](#876)) ([77ecf10](77ecf10))

### BREAKING CHANGES

* require react version 18

* fix: pin react version to 18

* test: update test and test libraries for react 18
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 12.0.0-alpha.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants