Skip to content

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jan 31, 2022

Summary

closes #4800
closes #4250

This PR upgrades React 16.12 to 17.0, as well as upgrading various other React-based 3rd-party component dependencies that now also support React 17.

Thanks to React's new policy of easy upgrades, the actual impact on our components and docs is minimal (barring QA on React's 17 changelog).

QA

  • Breaking event changes - we should test components with effects such as drag & drop, page-wide listeners, etc.
  • Kibana compatibility - Kibana is currently pinned at 16.12.0. We need to test if EUI on React 17 will work with Kibana and start a conversation with them on their planned upgrade timelines.
  • Enzyme does not support React 17 OOTB and requires a forked adapter to work (2d17e06).

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked Code Sandbox works for any docs examples
- [ ] Added or updated jest and cypress tests

  • Checked for breaking changes and labeled appropriately

- [ ] Checked for accessibility including keyboard-only and screenreader modes

  • A changelog entry exists and is marked appropriately

@cee-chen cee-chen added breaking change PRs with breaking changes. (Don't delete - used for automation) tech debt labels Jan 31, 2022
@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 1, 2022

Just tested this with yarn build-pack and a local Kibana pointed at the .tgz and everything seems to be working in QA with no bootstrap or console errors! The only change I had to make was also upgrading Kibana's react-beautiful-dnd to 13.1.0 (from 13.0.0). I don't think I can run Kibana's many tests/checks without a CI instance though.

Nevertheless this is still pretty exciting - could React 17 possibly be this easy? 💦

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 1, 2022

Spencer also came back re: Kibana's plans for React 17:

Pretty sure we don't have any timeline for it, but it should be a pretty easy upgrade. For react we usually let other teams lead there

Which I would take as an implicit greenlight to upgrade ahead of Kibana itself (rather than pinning to match them). Our "^16.12 || ^17.0" peerDependency should cover us for both Kibana and other apps that choose to use React 17.

@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 2, 2022

Once #5554 and #5591 both land, I'll pull this out of draft (so that we don't have too much yarn lock shuffling going on at once). Also worth noting that the major item left to QA is portals/focus-traps (e.g. popovers, modals) across all components.

+ update snapshots that changed as a result
- Remove directly unused react-test-renderer dependency (it's used a sub-dependency of other dependencies, but we shouldn't have to manually keep it updated since we don't import it directly)

- Fix `@emotion/react` semver

- Fix unnecessary `react-is` import
@cee-chen cee-chen removed the breaking change PRs with breaking changes. (Don't delete - used for automation) label Feb 7, 2022
@cee-chen cee-chen marked this pull request as ready for review February 7, 2022 16:33
@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 7, 2022

This is ready for review. Unlike #5591, I think this is a non-breaking change since applications still on React 16.12+ should still fully work with EUI, it's just that now React 17+ is supported as well. Am I incorrect in thinking that @thompsongl @chandlerprall?

Once staging is updated, I'll do a quick QA of all components and another test against local Kibana (to confirm the combined Typescript upgrade works as well).

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

I think this is a non-breaking change since applications still on React 16.12+ should still fully work with EUI, it's just that now React 17+ is supported as well

Agreed!

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@thompsongl
Copy link
Contributor

thompsongl commented Feb 7, 2022

I think this is a non-breaking change since applications still on React 16.12+ should still fully work with EUI, it's just that now React 17+ is supported as well

Yep, however the change to @emotion/react might be breaking; anything below 11.7 will cause warnings where it doesn't currently

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 7, 2022

Yep, however the change to @emotion/react might be breaking; anything below 11.7 will cause warnings where it doesn't currently

Ahh sorry that wasn't intentional, I was just trying to fix the semver. I thought 11.x wasn't valid semver, was I wrong about that? I can switch to ^11.0 if that makes more sense.

EDIT: I was totally wrong, .x is totally valid for peerDependencies - I'll go ahead and revert the change

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 7, 2022

OK, I tested (on all 4 browsers) the following utilities that might potentially be affected by the event delegation changes, and I'm fairly confident everything's working as before:

I also tested locally against Kibana with no issues - I tested Lens' datagrid as well as flyouts, everything is rendering as before.

Any objections if I merge today so we have 1 breaking change release with React/Typescript/export types etc all at once?

@thompsongl
Copy link
Contributor

Any objections if I merge today so we have 1 breaking change release with React/Typescript/export types etc all at once?

I'll review this afternoon 👍

@kibanamachine
Copy link

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

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, Constance!

Verified that the concerns raised in #4250 are resolved (probably with the dep upgrades) and focus traps work as expected.
Also confirmed that Kibana can accept the changes as long as react-beautiful-dnd is upgraded to v13.1

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 7, 2022

Thanks for flagging that issue Greg! I also just tested a combobox on Kibana (still on React 16.12) and it looks like the focus behavior is still working there as well.

I'll go ahead and merge, it's possible we still might run into shenanigans once we start an actual upgrade on Kibana and have to do a patch release - fingers crossed!

@cee-chen cee-chen merged commit 43579b2 into elastic:main Feb 7, 2022
@cee-chen cee-chen deleted the react-17 branch February 7, 2022 19:40
@thompsongl thompsongl mentioned this pull request Feb 7, 2022
9 tasks
@j-m j-m mentioned this pull request Apr 10, 2022
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.

React 17 [EuiFocusTrap] Virtualized options in EuiComboBox cannot be selected with React v17 when inside EuiFocusTrap

4 participants