build: Cypress React 18 support and React version switcher #6933
Conversation
| // tested. It has to be directly compared against process.env.REACT_VERSION | ||
| // for tree-shaking to work and not throw an error because of a missing import. | ||
| let cypressMount: typeof mount; | ||
| if (process.env.REACT_VERSION === '18') { |
There was a problem hiding this comment.
This can't be extracted to a utility function because the if statement won't be properly tree-shaken anymore. It wouldn't be a big deal but keeping both requires includes them in the bundle and causes webpack module resolver to throw because of react-dom/client differences between v18 and earlier versions.
There was a problem hiding this comment.
This looks great to me! I a few questions (mostly open-ended ones) I'd love to get your thoughts on, but otherewise comments are mostly optional.
I do have a minor yarn.lock cleanup I'd like to push up here in a bit (Greg and Chandler used to do that kind of thing a lot and the habit spread to me) Scratch that, yarn didn't like that cleanup and kept reverting it 😭
| if (process.env.REACT_VERSION === '18') { | ||
| cypressMount = require('@cypress/react18').mount; | ||
| } else { | ||
| cypressMount = require('@cypress/react').mount; |
There was a problem hiding this comment.
Asking for my own edification/understanding - React 16 & 17 use the same mount fn/library, is that correct?
There was a problem hiding this comment.
Correct, they can use the same @cypress/react version because the ReactDOM.render API is the same
| ### Testing specific version of React | ||
|
|
||
| By default, EUI Cypress tests are run using the latest supported version of React. | ||
| You can change that behavior and run e2e tests using a different React version by passing the `--react-version` option set to `16`, `17` or `18`. |
There was a problem hiding this comment.
Should we / do we need to set up our test-ci script to run all our Cypress tests through each React version?
There was a problem hiding this comment.
Yeah we definitely should, but that probably means writing a script for handling all of that updated version switching logic. I'll think of a good solution this week and add it in a separate PR as it's not really needed straight away :)
| "react-docgen-typescript": "^2.2.2", | ||
| "react-dom": "^18.2.0", | ||
| "react-dom-16": "npm:react-dom@^16.14.0", | ||
| "react-dom-17": "npm:react-dom@^17.0.2", |
There was a problem hiding this comment.
[not related to this PR, just asking a question to get your thoughts] Just curious, when do you think we should drop React 16/17 support / remove these dependencies? Once Kibana is on React 18?
There was a problem hiding this comment.
We probably could drop React 16 support with the official release of React 18-compatible EUI, but that's something we should confirm with Kibana teams
| cy.mount( | ||
| <Fragment> | ||
| <> |
There was a problem hiding this comment.
💯 🎉 thanks for cleaning this up! (+ other lint/type cleanups in the PR!)
|
jenkins test this |
|
@tkajtoch If this continues to fail, maybe we fast-track the Cypress bump to v12 and see if that helps. |
|
@1Copenut all React 18 PRs fail in CI due to typing errors for now, this is expected. I'll add a note to PR description |
|
This change also ran the Cypress component (a11y) tests without issue, so that was a nice bonus! |
* 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
* 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
* 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
* 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
Summary
Add @cypress/react18 renderer and implement React version switching logic to run Cypress tests against any version of react supported by EUI.
Failing tests when running cypress using React 18 will be addressed in a separate PR.
QA
Please note that this PR will fail in CI due to wrong types and unit test issues related to React 18
yarnto install new packagesyarn test-cypress --react-version 17(or--react-version 16) and see it passing all testsyarn test-cypressand see some tests failing because of React 18 differencesGeneral checklist