-
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: refactor create
into class $Cy
#18715
chore: refactor create
into class $Cy
#18715
Conversation
Thanks for taking the time to open a PR!
|
8b1d81e
to
07b7cb3
Compare
packages/driver/src/cy/mouse.ts
Outdated
@@ -707,6 +707,8 @@ const create = (state, keyboard, focused, Cypress) => { | |||
return mouse | |||
} | |||
|
|||
export interface Mouse extends ReturnType<typeof create> {} |
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.
nit: I personally like when exports are at the end of files so it's easier to find. Thoughts?
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 also thought about it at first but I placed it there because it would be better to place export
functions in one place.
But it seems that your idea is better. I changed it.
|
||
getAvailableAliases, |
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.
was this export used in other places?
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.
It was an internal function used only inside aliases.ts
. So, I removed it.
} | ||
}, | ||
export interface IAssertions { | ||
verifyUpcomingAssertions: ReturnType<typeof create>['verifyUpcomingAssertions'] |
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.
Missing finishAssertions?
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 intentionally didn't add it because it's not a public API. Check the code below:
cypress/packages/driver/src/cypress/cy.ts
Lines 611 to 612 in 50d1070
assert: assertions.assert, | |
verifyUpcomingAssertions: assertions.verifyUpcomingAssertions, |
// we notify the outside world because this is what the runner uses to | ||
// show the 'loading spinner' during an app page loading transition event | ||
return Cypress.action('cy:stability:changed', stable, event) | ||
// eslint-disable-next-line @cypress/dev/arrow-body-multiline-braces |
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.
If you had maintained the code structure here, would you have needed to disable this rule?
export const create = (Cypress, state) => {
const isStabe = () => {}
const whenStable = () => {}
return {
isStable,
whenStable,
}
}
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 structured them in this way because I want to emphasize all functions are exported. When we use return
, we cannot be sure if it's exported or not until we check if it's included in return
. I didn't follow this convention there are internal functions or functions that are shared between other functions.
@@ -118,58 +120,284 @@ const setTopOnError = function (Cypress, cy) { | |||
|
|||
// NOTE: this makes the cy object an instance | |||
// TODO: refactor the 'create' method below into this class |
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.
TO DO is done? 😄
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 yet. This is the first step to remove it. I'll remove it in the next PR.
e4bcacc
to
b2a99d0
Compare
2544da9
to
f5d740e
Compare
* develop: (329 commits) chore: Update Chrome (stable) to 96.0.4664.45 (#18931) fix: Loading of specs with % in the filename (#18877) chore: refactor `create` into class `$Cy` (#18715) chore: Update Chrome (beta) to 96.0.4664.45 (#18891) fix: flaky `system-tests-firefox` job (#18848) release 9.0.0 feat: ensure major release have conduit app wait on localhost:3000 fix install-required-node use --legacy-peer-deps feat: ensure major release fix darwin node install chore(driver): fix integration test retry configuration (#18643) feat(deps): update dependency electron to v15 🌟 (#18317) chore: Bind this correctly when setting response headers with cy.route() (#18859) feat: create config package for config validation (#18589) chore: patch `winston` to suppress `padLevels` warning (#18824) chore: test out major release build fix: remove outdated npm registry links (#18727) fix: Adding an existing command with `Cypress.Commands.add()` will throw an error (#18587) ...
* develop: (52 commits) feat: use hoisted yarn install in binary build (#17285) fix: compile npm packages for node 12 (#18989) fix: show call count even if `cy.stub().log(false)`. (#18907) chore: Update TypeScript to 4.4.4 (#18930) fix: wrap playground selectors in double quotes if not included (#18442) fix: flaky settings_spec test (#18979) chore: Update Chrome (stable) to 96.0.4664.45 (#18931) fix: Loading of specs with % in the filename (#18877) chore: refactor `create` into class `$Cy` (#18715) chore: Update Chrome (beta) to 96.0.4664.45 (#18891) fix: flaky `system-tests-firefox` job (#18848) chore: release @cypress/webpack-preprocessor-v5.10.0 chore: release @cypress/vue-v3.0.5 chore: release @cypress/schematic-v1.6.0 chore: release create-cypress-tests-v1.2.0 release 9.0.0 feat: ensure major release have conduit app wait on localhost:3000 fix install-required-node use --legacy-peer-deps ...
* 10.0-release: (56 commits) chore: post-merge cleanup feat: use hoisted yarn install in binary build (#17285) fix: fix spec list header, "Create specs" prompt, add workspace recommended apollo extension (#18993) feat(unify): reporter settings (#18946) feat: add devServer to config file (#18962) fix: compile npm packages for node 12 (#18989) fix: show call count even if `cy.stub().log(false)`. (#18907) chore: Update TypeScript to 4.4.4 (#18930) fix: wrap playground selectors in double quotes if not included (#18442) fix: flaky settings_spec test (#18979) chore: Update Chrome (stable) to 96.0.4664.45 (#18931) fix: Loading of specs with % in the filename (#18877) chore: refactor `create` into class `$Cy` (#18715) chore: Update Chrome (beta) to 96.0.4664.45 (#18891) fix: flaky `system-tests-firefox` job (#18848) chore: release @cypress/webpack-preprocessor-v5.10.0 chore: release @cypress/vue-v3.0.5 chore: release @cypress/schematic-v1.6.0 chore: release create-cypress-tests-v1.2.0 release 9.0.0 ...
…e-data-clean-refactor * tgriesser/chore/e2e-data-clean: (76 commits) chore: post-merge cleanup feat: use hoisted yarn install in binary build (#17285) fix: fix spec list header, "Create specs" prompt, add workspace recommended apollo extension (#18993) feat(unify): reporter settings (#18946) feat: add devServer to config file (#18962) fix: compile npm packages for node 12 (#18989) fix: show call count even if `cy.stub().log(false)`. (#18907) chore: Update TypeScript to 4.4.4 (#18930) feat: use fuzzy search (#18966) fix: onUnmounted warning in topnav (#18988) fix: wrap playground selectors in double quotes if not included (#18442) fix: flaky settings_spec test (#18979) fix: CYPRESS_INTERNAL_VITE_DEV for development feat: Create default config file (#18943) feat(app): support editor preference (#18932) chore: Update Chrome (stable) to 96.0.4664.45 (#18931) fix: Loading of specs with % in the filename (#18877) feat: improve vite DX (#18937) chore: refactor `create` into class `$Cy` (#18715) feat: Use plugins on config files (#18798) ...
User facing changelog
N/A
Additional details
$Cy
class is dummy and all the definitions were done inside thecreate
function and_.extend
. We're moving the parts into it.implementation details
_.extend
(next PR)// @ts-nocheck
incy.ts
and other outside functions.create
out ofexport default
because it requires one more indentation.$Cy
that are defined outsidecy.ts
and merged into it by_.extend
. I solved the problem by creating interfaces for them. In the code, you'll find many interfaces likeITimeouts
,IJQuery
, etc.How has the user experience changed?
N/A
PR Tasks
N/A