Skip to content

Conversation

@tkajtoch
Copy link
Member

@tkajtoch tkajtoch commented May 12, 2023

Summary

This is the first PR upgrading to React 18 and bumping all dependency versions that need it in order to support React 18.

Please note that the unit and e2e tests are not 100% passing. This is expected and will be addressed in further PRs to the feature/react-18 feature branch.

  • Upgrade React and ReactDOM to version 18.2.0 and other related packages
  • Switch to using the hacky @cfaester/enzyme-adapter-react-18 enzyme adapter for React 18
  • Update component types to support React 18
  • Update jest emotion serializer to fix class name transformation
  • Migrate EuiPortal tests to RTL
  • Integrate Redux DevTools for easy debugging and remove react-router-redux as it doesn't seem to be used or maintained anymore
  • Upgrade Cypress renderer to @cypress/react18
  • Rewrite EuiPortal to be a function component and use hooks to manage the container element

QA

  1. Pull the changes and perform a clean yarn install (rm -rf node_modules && yarn)
  2. Run yarn start and check if the development environments starts up and is accessible in browser
  3. Open http://localhost:8030 and verify the application is (more or less) working as intended - there are no hard crashes and components are behaving as they would normally do
  4. Run yarn build and check if the production build is passing
  5. Run yarn lint to confirm there are no typing or code style issues

Ignore failing unit and e2e tests for now. They will be fixed in the following PRs to feature/react-18.

General checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for accessibility including keyboard-only and screenreader modes

Changelog and possible breaking changes will be listed when merging the feature branch (feature/react-18) to main

@tkajtoch tkajtoch added dependencies PRs that update a dependency file tech debt labels May 12, 2023
@tkajtoch tkajtoch self-assigned this May 12, 2023
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6772/

1 similar comment
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6772/

@tkajtoch tkajtoch changed the base branch from main to react-18 May 16, 2023 13:30
@cdolek
Copy link
Contributor

cdolek commented May 18, 2023

is there any timeline on this?

@tkajtoch tkajtoch marked this pull request as ready for review May 30, 2023 15:41
@cee-chen cee-chen added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label May 31, 2023
@cee-chen
Copy link
Contributor

Hey @tkajtoch, just saw this was marked ready for review for merge into a feature branch - should I go ahead and start reviewing/QAing?

@cdolek Please be patient - we're actively working on this 😃 Thanks!

@tkajtoch
Copy link
Member Author

tkajtoch commented Jun 1, 2023

@cee-chen I cleaned up this branch to contain mostly the major pieces that make react 18 work with our code base. It's ready to review. After we merge this one, I'll rebase the feature branch and open next PRs for the remaining work waiting on branches on my fork :)

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6772/

1 similar comment
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6772/

Comment on lines +14 to +15
["@babel/react", { runtime: 'classic' }],
["@babel/typescript", { isTSX: true, allExtensions: true, allowDeclareFields: true }],
Copy link
Contributor

Choose a reason for hiding this comment

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

This reminds me - I'm going to go ahead and merge latest main into the upstream feature branch. It's missing all the latest babel upgrades which is almost certainly going to affect yarn.lock at the very minimum

Copy link
Contributor

Choose a reason for hiding this comment

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

alrighty, main is in - @tkajtoch please rebase/merge main into this PR/branch as well

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm on it!

Copy link
Contributor

@cee-chen cee-chen Jun 1, 2023

Choose a reason for hiding this comment

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

Iiin hindsight, I realize is actually A Lot of conflicts/changes, especially with the #6809 (comment). 😅

@tkajtoch, if we're going to have to do major merge conflict resolution in any case, I'd like to talk about splitting this PR significantly more (especially since it's going into a feature branch and CI does not technically have to pass). Realistically, 120+ files touched and a 1-3k line diff is way too much for a single code review.

Can we split it up like so:

  • PR 1:
    • Initial React 18 package upgrade
    • Any related or required dependency upgrades (e.g. redux devtools)
    • Types fixes only, no test fixes, nothing has to be working
  • PR 2:
    • Non-portal Jest test fixes
    • Enyzme adapter workaround
    • Jest Emotion serializer update
  • PR 3:
    • Portal-specific workarounds/upgrades and test fixes
  • PR 4:
    • Cypress test fixes
  • I believe you have a few more PRs queued up after this

Having more atomic PRs will make it significantly easier for us to progress with confidence that our code is consistent and thoroughly reviewed rather than trying to wait until the end to QA it all together.

Comment on lines +231 to +237
"react-router": "^5",
"react-router-dom": "^5",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have these packages (and history up above on line 205) only state a major instead of clearly delineating a minor/patch? Personally, I strongly prefer to use package.json minors as a way of keeping track of when we've deliberately upgraded to latest releases, rather than having to dig through yarn.lock to figure out what we're actually on. Can we update this to be the case?

@tkajtoch tkajtoch force-pushed the feature/react-18 branch from 71fc316 to 92d3c9f Compare June 1, 2023 21:52
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6772/

@tkajtoch tkajtoch mentioned this pull request Jun 2, 2023
1 task
@tkajtoch
Copy link
Member Author

tkajtoch commented Jun 2, 2023

The work is now divided into multiple PRs to make reviewing easier. See #6827.

@tkajtoch tkajtoch closed this Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies PRs that update a dependency file skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants