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

Add support for React 18 #7012

Merged
merged 24 commits into from
Aug 1, 2023
Merged

Add support for React 18 #7012

merged 24 commits into from
Aug 1, 2023

Conversation

tkajtoch
Copy link
Member

@tkajtoch tkajtoch commented Jul 28, 2023

Summary

This PR resolves #6182 and contains all necessary changes to make EUI work fully compatible with React 18. Every commit represents a separate set of changes that were all reviewed and merged.

When this PR gets merged EUI will officially support React 16.12+, React 17 and React 18.

Changes

  • Upgrade react, react-dom and their type definitions to v18 versions
  • Add children types to all components and their props types
  • Resolve internal TypeScript errors caused by upgrading type definitions of above libraries
  • Add React version switching logic to unit and e2e test runners so EUI can be tested against all supported React versions
  • Adjust selected components to work in React 17 and 18 <StrictMode>. Note that not all components support StrictMode at this moment.

QA

  • Check CI linting, type checking and automated testing steps are all passing
  • Open preview environment docs site and verify the components behave as expected
  • Perform local tests
    • Checkout this PR
    • Run rm -rf node_modules; yarn to do a clean install
    • Run yarn start and open the local docs site
    • Verify it's loading, is responsive and EuiPortal, EuiPopover and other components work as expected

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 breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@tkajtoch tkajtoch marked this pull request as ready for review July 31, 2023 15:15
@cee-chen
Copy link
Member

cee-chen commented Jul 31, 2023

It's happening!!

@tkajtoch, do you mind describing the steps you took to test Kibana against this PR? In particular, were you able to test CI ahead of time with the prerelease RC? Either in the PR description or in a comment is fine

Comment on lines +5 to +6
- Replaced the underlying drag-and-drop library from `react-beautiful-dnd` to its fork `@hello-pangea/dnd` ([#7012](https://github.com/elastic/eui/pull/7012))
- No code updates are needed if using only `<EuiDragDropContext>`, `<EuiDroppable>` and `<EuiDraggable>` with no direct imports from `react-beautiful-dnd`. In case you were importing things from `react-beautiful-dnd` and using them together with EUI components, you need to switch to `@hello-pangea/dnd` which has cross-compatible API.
Copy link
Member

Choose a reason for hiding this comment

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

I went back and forth on this a bit, but in the end I really like the thoughtfulness of marking this as a breaking change. Thanks Tomasz!

@tkajtoch
Copy link
Member Author

tkajtoch commented Jul 31, 2023

@cee-chen Sure! I created a prerelease version of EUI based on feature/react-18 and then updated version of @elastic/eui on my local kibana instance which I later pushed here to test against CI.

Locally I tested yarn build, yarn lint and yarn start to see if any errors appear either in the console or in runtime - I found 3 small typescript errors in x-pack/plugins/index_management/public/application/components/component_templates/component_template_selector/component_templates_selection.tsx caused by updated dnd types and everything else running perfectly in runtime.

In CI I noticed the same 3 typescript errors and ~15 failing unit tests caused by small snapshot differences (e.g. adding data-test-subj or updating the style attribute in markdown_editor_text_area.tsx).

These failing tests are easily fixable and are not a sign of something wrong happening there. I'm also confident the changes this PR introduces don't really affect apps using React 17, so all first-party elastic services should work just like before. We might, however, see some issues being opened in the future for React 18 support and unusual use-cases, but that's expected. Not that I noticed any :)


I forgot to mention - I've also clicked through lots of pages in kibana and tested many if its features, especially the Observability ones.

yarn.lock Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Making a note of something we discussed in our sync today - Chandler and Greg previously used to manually merge dependencies in yarn.lock, we're avoiding doing this as a team going forward (possibly also something to discuss as part of #6862)

@cee-chen
Copy link
Member

cee-chen commented Jul 31, 2023

Tooltips are currently totally broken for me on local but not on staging:

@1Copenut any chance you can pull down this PR and follow the QA steps in the PR description (rm -rf node_modules && yarn) and confirm if you can repro?

NOTE: You may want to finish any other non-React-18 reviews or work you're currently working on before doing this, rm -rf node_modules each time can be a bit of a headache

List of components working in staging but not locally:

There's several other non-breaking console.errors that also emit as a result of strict mode, e.g. valid DOM nesting as well as keys etc. that we should likely take the time to fix later/while we're implementing strict mode.

@1Copenut
Copy link
Contributor

@1Copenut any chance you can pull down this PR and follow the QA steps in the PR description (rm -rf node_modules && yarn) and confirm if you can repro?

@cee-chen I pulled the code down and tested the links you provided on my local. I was able to recreate the issues you identified. I'll check them on staging as well.

@tkajtoch
Copy link
Member Author

I removed <StrictMode> support from changelog and docs app source after finding a few issues with it in development mode. We'll address these issues separately and announce it when it's ready, hopefully in the next EUI release.

@1Copenut
Copy link
Contributor

All four links @cee-chen provided earlier are working locally after latest change to <Strict Mode>.

@cee-chen
Copy link
Member

@tkajtoch Our EuiWrappingPopover demo needs to be converted away from the old React 17 render method:

render(<PopoverApp anchor={thisAnchor} />, container);

Screenshot 2023-07-31 at 1 39 10 PM

@1Copenut
Copy link
Contributor

Retesting the <EuiWrappingPopover> in local after pulling code from @cee-chen latest push. The error message is no longer present.

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

I reviewed code files one at a time to make sure I understood changes and any code refactors. I evaluated the items Cee called out as not working in local host and clicked through components that had code changes.

I noticed one additional React error on EuiColorPicker - option toggling ( http://localhost:8030/#/forms/color-selection#option-toggling ) that's not appearing in production. Looks like the addStop prop is ending up rendered into the DOM.

Screen Shot 2023-07-31 at 4 41 13 PM

@cee-chen
Copy link
Member

@1Copenut this is likely already happening in prod - let's fix that as a separate PR. Would be a nice quick fix for support week!

@1Copenut
Copy link
Contributor

1Copenut commented Jul 31, 2023

@1Copenut this is likely already happening in prod - let's fix that as a separate PR. Would be a nice quick fix for support week!

Got the issue flagged. I'll put it in the Planned for this week support queue.

#7014

…-pangea/dnd (#6868)

* feat(DragAndDrop): Switch from react-beautiful-dnd to its fork @hello-pangea/dnd

* test(EuiDraggable): add snapshot test for the basic render assertion and rename test cases
* build(deps): Bump react and react-dom to v18

* refactor(docs): update docs site initialization to use createRoot API

* build(babel): enforce classic react runtime and allow declare fields in babel config

* refactor(*): update children prop types to use React 18 PropsWithChildren

* build(*): bump react and react-dom versions

* style(EuiForm): replace React.FC with FunctionComponent usage

* style(*): reorder PropsWithChildren usage to be from least to most specific
* refactor: convert Cypress mount and realMount commands to TypeScript

* build: Conditionally use @cypress/react or @cypress/react18 depending on REACT_VERSION environment variable

* build: add Cypress React version switcher

* docs: update cypress-testing.md

* build: move react version console.log
* build: convert jest config to .js

* build: add react version switching

* build: add @cfaester/enzyme-adapter-react-18 enzyme adapter and use it when REACT_VERSION is 18

* build: update @testing-library/react and provide an alternative version for testing React 16 and 17

* build: polyfill TextEncoder for running React 18 tests

* build: set IS_REACT_ACT_ENVIRONMENT = true when running React 18 enzyme adapter

* test: reset dnd server context when running on react 16 and 17

* test: fix fireEvent import to use act-wrapped react equivalent

* test: update form inline snapshot due to whitespace mismatch
* test: unify style properties values between React 18 and earlier versions

* test: Separate Droppable snapshots by React version and add describeByReactVersion utility

* test: separate Draggable snapshots by React version

* test: separate column selector snapshots by React version

* test: separate column sorting snapshots by React version

* fix: unify empty string rendering in snapshots between testing-library v14 (react 18-compatible) serializing and earlier versions

* docs: add testByReactVersion jsdoc
* test: replace @testing-library/dom with @testing-library/react as the latter exports are wrapped in act()

* test: wrap function calls with act() that need it

* test: use act() wrapper for jest.advanceTimersByTime calls and add actAdvanceTimersByTime utility function

* test: wait for element to be rendered before taking a snapshot to fix act() warning
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
@elastic elastic deleted a comment from kibanamachine Aug 1, 2023
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

PHEW. Skimmed all the files and those were my remaining comments. I pushed up my minor feedback items for speed/expedience. Let's finally merge this in once CI passes!!! 🚢

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @tkajtoch

@tkajtoch tkajtoch merged commit d3e8e0b into main Aug 1, 2023
1 check passed
@tkajtoch tkajtoch deleted the feature/react-18 branch August 1, 2023 16:03
jbudz pushed a commit to elastic/kibana that referenced this pull request Aug 14, 2023
`85.1.0` ➡️ `86.0.0`

⚠️ The biggest change in this PR is migrating the `react-beautiful-dnd`
dependency to it's open-source forked successor, `@hello-pangea/dnd`.
This new fork has better typescript support and additionally supports
both React 17 and React 18.

## [`86.0.0`](https://github.com/elastic/eui/tree/v86.0.0)

- Added React 18 support (StrictMode not yet supported).
([#7012](elastic/eui#7012))

**Deprecations**

- Deprecated `euiPaletteComplimentary`; Use `euiPaletteComplementary`
instead. ([#6992](elastic/eui#6992))

**Breaking changes**

- Replaced the underlying drag-and-drop library from
`react-beautiful-dnd` to its fork `@hello-pangea/dnd`
([#7012](elastic/eui#7012))
([#7012](elastic/eui#7012))
- No code updates are needed if using only `<EuiDragDropContext>`,
`<EuiDroppable>` and `<EuiDraggable>` with no direct imports from
`react-beautiful-dnd`. In case you were importing things from
`react-beautiful-dnd` and using them together with EUI components, you
need to switch to `@hello-pangea/dnd` which has cross-compatible API.

---------

Co-authored-by: Tomasz Kajtoch <[email protected]>
Co-authored-by: Tomasz Kajtoch <[email protected]>
Co-authored-by: Cee Chen <[email protected]>
Co-authored-by: Drew Tate <[email protected]>
bryce-b pushed a commit to elastic/kibana that referenced this pull request Aug 22, 2023
`85.1.0` ➡️ `86.0.0`

⚠️ The biggest change in this PR is migrating the `react-beautiful-dnd`
dependency to it's open-source forked successor, `@hello-pangea/dnd`.
This new fork has better typescript support and additionally supports
both React 17 and React 18.

## [`86.0.0`](https://github.com/elastic/eui/tree/v86.0.0)

- Added React 18 support (StrictMode not yet supported).
([#7012](elastic/eui#7012))

**Deprecations**

- Deprecated `euiPaletteComplimentary`; Use `euiPaletteComplementary`
instead. ([#6992](elastic/eui#6992))

**Breaking changes**

- Replaced the underlying drag-and-drop library from
`react-beautiful-dnd` to its fork `@hello-pangea/dnd`
([#7012](elastic/eui#7012))
([#7012](elastic/eui#7012))
- No code updates are needed if using only `<EuiDragDropContext>`,
`<EuiDroppable>` and `<EuiDraggable>` with no direct imports from
`react-beautiful-dnd`. In case you were importing things from
`react-beautiful-dnd` and using them together with EUI components, you
need to switch to `@hello-pangea/dnd` which has cross-compatible API.

---------

Co-authored-by: Tomasz Kajtoch <[email protected]>
Co-authored-by: Tomasz Kajtoch <[email protected]>
Co-authored-by: Cee Chen <[email protected]>
Co-authored-by: Drew Tate <[email protected]>
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.

[Meta] React 18 Support
5 participants