-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
feat: React 18 support #22876
feat: React 18 support #22876
Conversation
Thanks for taking the time to open a PR!
|
circle.yml
Outdated
@@ -38,7 +38,7 @@ macWorkflowFilters: &darwin-workflow-filters | |||
- equal: [ develop, << pipeline.git.branch >> ] | |||
- equal: [ 'global-mode-issue', << pipeline.git.branch >> ] | |||
- matches: | |||
pattern: "-release$" | |||
pattern: "lmiller/react-adapters" |
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.
So CT team can test the binary - most of the team is on mac.
@lmiller1990 After setting up my React 18 CRA project with the CT wizard, my component.js file looks like this
Should I expect this import to be |
@astone123 right, I wonder if we want to update launchpad to scaffold You should definitely see the warning in the console, though. That's strange. I will check. |
npm/react18/package.json
Outdated
"typescript": "^4.2.3" | ||
}, | ||
"peerDependencies": { | ||
"@types/react": "^16.9.16 || ^17.0.0", |
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.
"@types/react": "^16.9.16 || ^17.0.0", | |
"@types/react": "^18.0.0", |
npm/react18/package.json
Outdated
"@rollup/plugin-commonjs": "^17.1.0", | ||
"@rollup/plugin-node-resolve": "^11.1.1", | ||
"cypress": "0.0.0-development", | ||
"react": "16.8.6", |
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.
Should we update react deps to 18 for this package? Not sure if these are really even used...
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.
I can try but I think this messes up the monorepo dependencies since Lerna hoisting is not perfect. This dependency is not used at all (we grab their React version) but would be nice to have the correct deps if we can.
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.
Not really used but going to update it anyway!
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.
Tested with the binary, looks good! Just a few nitpicks, going to ✅
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.
Looks good. I spun up a React 18 project using CRA and made a small test using cypress/react18
. No ReactDOM
error 👍🏻
Disregard my earlier comment... I wasn't seeing the error when using cypress/react
because my test wasn't actually mounting anything 🤦🏻♂️
Yeah I think we should make an issue to update launchpad for this |
@astone123 issue for launchpad #22890 |
Vite/Webpack passing, just some weird Percy API error blocking them from going green. Merging! |
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
User facing changelog
Include a
cypress/react18
library to support React 18 in component testing.Additional details
Brief: (Cypress team only): https://github.com/cypress-io/tech-briefs/tree/main/briefs/cy-mount-library-versioning
In the next major version of Cypress,
cypress/react
will support the main version of React on npm - that is, 18, at least right now. Until then, React 18 users can opt in to use our React 18 adapter viaimport { mount } from 'cypress/react18'
.I refactored the
npm/react
code so we can share it across multiple React adapters. The main files you'll want to review arenpm/react
. The react 18 adapter code is the most interesting - you can see how 99% of the React adapter code is shared.cypress/react18
just injects the React 18 specific code. I like this pattern, it should let us create more React adapters if we need them in the future (or maybe even a Next.js specific adapter, should the need arise).The bulk of the changes here are to tests - previously, we would cache a single React version for all system tests, to save time installing it each time. Now we support different versions of React with non-compatible mounting APIs, we need to make sure all the test projects using React correctly declare their React version.
Steps to test
I've added some tests. You could just run the React 18 test. It's here: https://github.com/cypress-io/cypress/pull/22876/files#diff-43b08f660ee024cc300876efc9942e863a03f097cf3ae13d9dfece74bc11896eR1
I'll add a pre-release binary to test more. Scroll down and grab it.
Testing:
Once you did that, make a test:
You'll see a warning in your console. You're using
cypress/react
. Update supportFile:Now it works, no warning.
We will update launchpad to scaffold the correct import at a later date, just focusing on getting a react adapter shipped for now.
How has the user experience changed?
Not warning when mounting React 18 - right now
cypress/react
is designed for React 17, and you get a warning in the console.PR Tasks
cypress-documentation
?type definitions
?