-
Notifications
You must be signed in to change notification settings - Fork 842
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
build: Cypress React 18 support and React version switcher #6933
Changes from 4 commits
e0be518
e99eb25
64ea471
1cff2cb
a140921
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import React, { ReactNode } from 'react'; | ||
import { EuiProvider } from '../../../src'; | ||
import type { mount } from '@cypress/react18'; | ||
|
||
// Pick cypress mount function based on which React version is currently being | ||
// 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') { | ||
cypressMount = require('@cypress/react18').mount; | ||
} else { | ||
cypressMount = require('@cypress/react').mount; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, they can use the same @cypress/react version because the |
||
} | ||
|
||
const mountCommand = (children: ReactNode): ReturnType<typeof mount> => { | ||
return cypressMount(<EuiProvider>{children}</EuiProvider>); | ||
}; | ||
|
||
// Export only the type to not confuse code-completion tools | ||
export type mountCommand = typeof mountCommand; | ||
|
||
Cypress.Commands.add('mount', mountCommand); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,21 +6,26 @@ | |
* Side Public License, v 1. | ||
*/ | ||
|
||
import { React, Fragment } from 'react'; | ||
import React, { ReactNode } from 'react'; | ||
import './mount'; | ||
|
||
Cypress.Commands.add('realMount', (children) => { | ||
const realMountCommand = (children: ReactNode) => { | ||
cy.mount( | ||
<Fragment> | ||
<> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 🎉 thanks for cleaning this up! (+ other lint/type cleanups in the PR!) |
||
<div | ||
data-test-subj="cypress-real-event-target" | ||
style={{ height: '1px', width: '1px' }} | ||
/> | ||
{children} | ||
</Fragment> | ||
</> | ||
).then(() => { | ||
cy.get('[data-test-subj="cypress-real-event-target"]').realClick({ | ||
position: 'topLeft', | ||
}); | ||
}); | ||
}); | ||
}; | ||
|
||
// Export only the type to not confuse code-completion tools | ||
export type realMountCommand = typeof realMountCommand; | ||
|
||
Cypress.Commands.add('realMount', realMountCommand); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,7 +117,8 @@ | |
"@babel/preset-typescript": "^7.21.5", | ||
"@babel/template": "^7.21.9", | ||
"@cypress/code-coverage": "^3.10.0", | ||
"@cypress/react": "^5.10.3", | ||
"@cypress/react": "^7.0.3", | ||
"@cypress/react18": "^2.0.0", | ||
"@cypress/webpack-dev-server": "^1.7.0", | ||
"@elastic/charts": "^53.1.1", | ||
"@elastic/datemath": "^5.0.3", | ||
|
@@ -226,8 +227,12 @@ | |
"puppeteer": "^5.5.0", | ||
"raw-loader": "^4.0.1", | ||
"react": "^18.2.0", | ||
"react-16": "npm:react@^16.14.0", | ||
"react-17": "npm:react@^17.0.2", | ||
"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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
"react-helmet": "^6.1.0", | ||
"react-redux": "^7.2.1", | ||
"react-refresh": "^0.11.0", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,11 @@ By default tests are run using the light theme. Dark mode can be enabled by pass | |
|
||
To ensure tests use up-to-date styles, the test runner compiles our SCSS to CSS before executing Cypress. This adds some processing time before the tests can run, and often the existing locally-built styles are still accurate. The CSS compilation step can be skipped by passing the `--skip-css` flag to `yarn test-cypress`, `yarn test-cypress-dev` and `yarn test-cypress-a11y`. | ||
|
||
### 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we / do we need to set up our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds great! |
||
|
||
### Cypress arguments | ||
|
||
You can also pass [Cypress CLI arguments](https://docs.cypress.io/guides/guides/command-line). For example: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 bothrequire
s includes them in the bundle and causes webpack module resolver to throw because ofreact-dom/client
differences between v18 and earlier versions.