-
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
chore: update react in the reporter from 17.0.2 to 18.3.1 #30567
Conversation
115fbe1
to
669fa5c
Compare
cypress Run #58225
Run Properties:
|
Project |
cypress
|
Branch Review |
chore/update_react_18
|
Run status |
Passed #58225
|
Run duration | 22m 15s |
Commit |
7a18bb25b8: chore: update to react 18 for the reporter from 17.0.2 to 18.3.1 [run ci]
|
Committer | AtofStryker |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
6
|
Pending |
1327
|
Skipped |
0
|
Passing |
29332
|
View all changes introduced in this branch ↗︎ |
UI Coverage
46.67%
|
|
---|---|
Untested elements |
187
|
Tested elements |
168
|
Accessibility
92.56%
|
|
---|---|
Failed rules |
3 critical
8 serious
2 moderate
2 minor
|
Failed elements |
895
|
@@ -11,12 +11,6 @@ export function setInitializedReporter (val: boolean) { | |||
hasInitializeReporter = val | |||
} | |||
|
|||
async function unmountReporter () { |
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 isn't used anywhere. Figured a good time to delete dead code as to unmount in react 18 we would need a root reference and I would rather avoid the complexity
@@ -56,11 +50,12 @@ function renderReporter ( | |||
testFilter: specsStore.testFilter, | |||
}) | |||
|
|||
window.UnifiedRunner.ReactDOM.render(reporter, root) | |||
const reactDomRoot = window.UnifiedRunner.ReactDOM.createRoot(root) |
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.
|
||
const currentTestLog = findReactInstance(cy.$$('.runnable-active', top?.document)[0]) | ||
|
||
currentTestLog.props.model._isOpen = true |
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 no longer works (hence why we are waiting the LONG_RUNNING_THRESHOLD
above).
The model does get updated in <Attempt/>
, but the prop passed into <Collapsible/>
in componentDidUpdate
still says false
, which seems like a bug.
Normally I would file an issue for this, but I would argue that waiting the full 2 seconds for the attempt to open is a better test since this is what the user would exactly experience and better tests our code, even though it adds a few seconds to each test leveraging this. We don't do this is many tests (maybe 7 tests total) so it seems fine.
@@ -154,8 +154,10 @@ declare global { | |||
if (window.Cypress) { | |||
window.state = appState | |||
window.render = (props) => { | |||
// @ts-ignore | |||
render(<Reporter {...props as Required<BaseReporterProps>} />, document.getElementById('app')) | |||
const container: HTMLElement = document.getElementById('app') as HTMLElement |
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.
see see https://react.dev/blog/2022/03/08/react-18-upgrade-guide#updates-to-client-rendering-apis. This render is used for cy-in-cy
669fa5c
to
7a18bb2
Compare
// Since updating the Reporter to React 18, this is no longer the case. The element looks to be rerendered | ||
// and the previous reference detached. We will need to come up with a different way to test this. | ||
// @see https://github.com/cypress-io/cypress/issues/30526. | ||
ensureCorrectHighlightPositions('#button', true) |
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.
comment says it all 😅
@@ -53,6 +52,7 @@ export function mount (jsx: React.ReactNode, options: MountOptions = {}, rerende | |||
// to wipe away any state | |||
cleanup() | |||
const internalOptions: InternalMountOptions = { | |||
// @ts-expect-error |
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 has to do with a type mismatch now that exists in the root of the monorepo 🥴 . This will be addressed with #29607
@@ -94,6 +94,7 @@ | |||
"@octokit/auth-app": "6.0.3", | |||
"@octokit/core": "5.0.2", | |||
"@percy/cli": "1.27.4", | |||
"@reach/dialog": "0.10.5", |
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.
for whatever reason this package is no longer getting hoisted. So when the runner tries to build the reporter, the package is missing. This puts it in the root of the repo so its available in both places as opposed to installing it in 2 places
Additional details
Updates the Cypress Reporter from React
17.0.2
to18.3.1
. This PR does the minimum work to get the Cypress reporter on React18.3.1
, which just means updating the render methods. Other deprecations and issues are scoped to #30574.Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?