-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 |
||
"@semantic-release/changelog": "6.0.3", | ||
"@semantic-release/git": "10.0.1", | ||
"@types/better-sqlite3": "^7.6.3", | ||
|
@@ -112,8 +113,8 @@ | |
"@types/mocha": "8.0.3", | ||
"@types/node": "20.16.0", | ||
"@types/prismjs": "1.16.0", | ||
"@types/react": "17.0.83", | ||
"@types/react-dom": "17.0.25", | ||
"@types/react": "18.3.12", | ||
"@types/react-dom": "18.3.1", | ||
"@types/request-promise": "4.1.45", | ||
"@types/send": "^0.17.1", | ||
"@types/sinon-chai": "3.2.3", | ||
|
@@ -189,8 +190,8 @@ | |
"pluralize": "8.0.0", | ||
"print-arch": "1.0.0", | ||
"proxyquire": "2.1.3", | ||
"react": "17.0.2", | ||
"react-dom": "17.0.2", | ||
"react": "18.3.1", | ||
"react-dom": "18.3.1", | ||
"rimraf": "5.0.10", | ||
"semantic-release": "22.0.12", | ||
"semantic-release-monorepo": "8.0.2", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 |
||
// We do not need to unmount the reporter at any point right now, | ||
// but this will likely be useful for cleaning up at some point. | ||
window.UnifiedRunner.ReactDOM.unmountComponentAtNode(getReporterElement()) | ||
} | ||
|
||
async function resetReporter () { | ||
if (hasInitializeReporter) { | ||
await getEventManager().resetReporter() | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
reactDomRoot.render(reporter) | ||
} | ||
|
||
export const UnifiedReporterAPI = { | ||
unmountReporter, | ||
setupReporter, | ||
hasInitializeReporter, | ||
resetReporter, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,7 +54,14 @@ describe('rect highlight', { browser: '!webkit' }, () => { | |
|
||
it('correct target position during click', () => { | ||
clickAndPin('#button') | ||
ensureCorrectHighlightPositions('#button') | ||
// TODO: We are currently skipping the element comparison on the highlight. | ||
// What looks to be occurring, is that the DOM reference fetched in elementFromPoint | ||
// is a stale detached DOM reference of the highlight container over the button. | ||
// In React 17, the "elementFromPoint" and "div[data-layer=Content]" share the same reference. | ||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. comment says it all 😅 |
||
ensureCorrectTargetPosition('#button') | ||
}) | ||
|
||
|
@@ -100,7 +107,7 @@ const ensureCorrectTargetPosition = (sel) => { | |
}) | ||
} | ||
|
||
const ensureCorrectHighlightPositions = (sel) => { | ||
const ensureCorrectHighlightPositions = (sel, skipElementComparison) => { | ||
return cy.wrap(null, { timeout: 4000 }).should(() => { | ||
const els = { | ||
content: cy.$$('div[data-layer=Content]'), | ||
|
@@ -112,12 +119,14 @@ const ensureCorrectHighlightPositions = (sel) => { | |
return $el[0].getBoundingClientRect() | ||
}) | ||
|
||
const doc = els.content[0].ownerDocument | ||
if (!skipElementComparison) { | ||
const doc = els.content[0].ownerDocument | ||
|
||
const contentHighlightCenter = [dims.content.x + dims.content.width / 2, dims.content.y + dims.content.height / 2] | ||
const highlightedEl = doc.elementFromPoint(...contentHighlightCenter) | ||
const contentHighlightCenter = [dims.content.x + dims.content.width / 2, dims.content.y + dims.content.height / 2] | ||
const highlightedEl = doc.elementFromPoint(...contentHighlightCenter) | ||
|
||
expect(highlightedEl).eq(els.content[0]) | ||
expect(highlightedEl).eq(els.content[0]) | ||
} | ||
|
||
expectToBeInside(dims.content, dims.padding, 'content to be inside padding') | ||
expectToBeInside(dims.padding, dims.border, 'padding to be inside border') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,42 +21,31 @@ export const findReactInstance = function (dom) { | |
} | ||
|
||
export const clickCommandLog = (sel, type) => { | ||
return cy.wait(10) | ||
// trigger the LONG_RUNNING_THRESHOLD to display the command line | ||
// this adds time to test but makes a more accurate test as React 18+ does not rerender when setting internals | ||
return cy.wait(2000) | ||
.then(() => { | ||
return withMutableReporterState(() => { | ||
const commandLogEl = getCommandLogWithText(sel, type) | ||
const reactCommandInstance = findReactInstance(commandLogEl[0]) | ||
const commandLogEl = getCommandLogWithText(sel, type) | ||
|
||
if (!reactCommandInstance) { | ||
assert(false, 'failed to get command log React instance') | ||
} | ||
const reactCommandInstance = findReactInstance(commandLogEl[0]) | ||
|
||
reactCommandInstance.props.appState.isRunning = false | ||
const inner = $(commandLogEl).find('.command-wrapper-text') | ||
if (!reactCommandInstance) { | ||
assert(false, 'failed to get command log React instance') | ||
} | ||
|
||
reactCommandInstance.props.appState.isRunning = false | ||
const inner = $(commandLogEl).find('.command-wrapper-text') | ||
|
||
inner.get(0).click() | ||
inner.get(0).click() | ||
|
||
// wait slightly for a repaint of the reporter | ||
cy.wait(10).then(() => { | ||
// make sure command was pinned, otherwise throw a better error message | ||
expect(cy.$$('.runnable-active .command-pin', top?.document).length, 'command should be pinned').ok | ||
}) | ||
}) | ||
} | ||
|
||
export const withMutableReporterState = (fn) => { | ||
// @ts-ignore | ||
top?.UnifiedRunner.MobX.configure({ enforceActions: 'never' }) | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. this no longer works (hence why we are waiting the The model does get updated in 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. |
||
|
||
return Promise.try(fn) | ||
.then(() => { | ||
// @ts-ignore | ||
top?.UnifiedRunner.MobX.configure({ enforceActions: 'always' }) | ||
}) | ||
} | ||
|
||
const wrapped = (obj) => cy.wrap(obj, { log: false }) | ||
|
||
export const shouldBeCalled = (stub) => wrapped(stub).should('be.called') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ import { action, runInAction } from 'mobx' | |
import { observer } from 'mobx-react' | ||
import cs from 'classnames' | ||
import React, { Component } from 'react' | ||
import { render } from 'react-dom' | ||
import { createRoot } from 'react-dom/client' | ||
// @ts-ignore | ||
import EQ from 'css-element-queries/src/ElementQueries' | ||
|
||
|
@@ -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 commentThe 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 |
||
const root = createRoot(container) | ||
|
||
root.render(<Reporter {...props as Required<BaseReporterProps>} />) | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8201,7 +8201,22 @@ | |
dependencies: | ||
"@types/react" "^17" | ||
|
||
"@types/react@*", "@types/[email protected]", "@types/react@^17": | ||
"@types/[email protected]": | ||
version "18.3.1" | ||
resolved "https://registry.npmjs.org/@types/react-dom/-/react-dom-18.3.1.tgz#1e4654c08a9cdcfb6594c780ac59b55aad42fe07" | ||
integrity sha512-qW1Mfv8taImTthu4KoXgDfLuk4bydU6Q/TkADnDWWHwi4NX4BR+LWfTp2sVmTqRrsHvyDDTelgelxJ+SsejKKQ== | ||
dependencies: | ||
"@types/react" "*" | ||
|
||
"@types/react@*", "@types/[email protected]": | ||
version "18.3.12" | ||
resolved "https://registry.npmjs.org/@types/react/-/react-18.3.12.tgz#99419f182ccd69151813b7ee24b792fe08774f60" | ||
integrity sha512-D2wOSq/d6Agt28q7rSI3jhU7G6aiuzljDGZ2hTZHIkrTLUI+AF3WMeKkEZ9nN2fkBAlcktT6vcZjDFiIhMYEQw== | ||
dependencies: | ||
"@types/prop-types" "*" | ||
csstype "^3.0.2" | ||
|
||
"@types/[email protected]", "@types/react@^17": | ||
version "17.0.83" | ||
resolved "https://registry.npmjs.org/@types/react/-/react-17.0.83.tgz#b477c56387b74279281149dcf5ba2a1e2216d131" | ||
integrity sha512-l0m4ArKJvmFtR4e8UmKrj1pB4tUgOhJITf+mADyF/p69Ts1YAR/E+G9XEM0mHXKVRa1dQNHseyyDNzeuAXfXQw== | ||
|
@@ -26266,6 +26281,14 @@ [email protected], react-dom@^17.0.2: | |
object-assign "^4.1.1" | ||
scheduler "^0.20.2" | ||
|
||
[email protected]: | ||
version "18.3.1" | ||
resolved "https://registry.npmjs.org/react-dom/-/react-dom-18.3.1.tgz#c2265d79511b57d479b3dd3fdfa51536494c5cb4" | ||
integrity sha512-5m4nQKp+rZRb09LNH59GM4BxTh9251/ylbKIbpe7TpGxfJ+9kv6BLkLBXIjjspbgbnIBNqlI23tRnTWT0snUIw== | ||
dependencies: | ||
loose-envify "^1.1.0" | ||
scheduler "^0.23.2" | ||
|
||
react-dom@^15.3.2: | ||
version "15.7.0" | ||
resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-15.7.0.tgz#39106dee996d0742fb0f43d567ef8b8153483ab2" | ||
|
@@ -26379,6 +26402,13 @@ [email protected], react@^17.0.2: | |
loose-envify "^1.1.0" | ||
object-assign "^4.1.1" | ||
|
||
[email protected]: | ||
version "18.3.1" | ||
resolved "https://registry.npmjs.org/react/-/react-18.3.1.tgz#49ab892009c53933625bd16b2533fc754cab2891" | ||
integrity sha512-wS+hAgJShR0KhEvPJArfuPVN1+Hz1t0Y6n5jLrGQbkb4urgPE/0Rve+1kMB1v/oWgHgm4WIcV+i7F2pTVj+2iQ== | ||
dependencies: | ||
loose-envify "^1.1.0" | ||
|
||
react@^15.3.2: | ||
version "15.7.0" | ||
resolved "https://registry.yarnpkg.com/react/-/react-15.7.0.tgz#10308fd42ac6912a250bf00380751abc41ac7106" | ||
|
@@ -27518,6 +27548,13 @@ scheduler@^0.20.2: | |
loose-envify "^1.1.0" | ||
object-assign "^4.1.1" | ||
|
||
scheduler@^0.23.2: | ||
version "0.23.2" | ||
resolved "https://registry.npmjs.org/scheduler/-/scheduler-0.23.2.tgz#414ba64a3b282892e944cf2108ecc078d115cdc3" | ||
integrity sha512-UOShsPwz7NrMUqhR6t0hWjFduvOzbtv7toDH1/hIrfRNIDBnnBWd0CwJTGvTpngVlmwGCdP9/Zl/tVrDqcuYzQ== | ||
dependencies: | ||
loose-envify "^1.1.0" | ||
|
||
schema-utils@>1.0.0, schema-utils@^4.0.0, schema-utils@^4.2.0: | ||
version "4.2.0" | ||
resolved "https://registry.yarnpkg.com/schema-utils/-/schema-utils-4.2.0.tgz#70d7c93e153a273a805801882ebd3bff20d89c8b" | ||
|
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