Skip to content
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

Merged

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Nov 1, 2021

User facing changelog

N/A

Additional details

  • Why was this change necessary? => $Cy class is dummy and all the definitions were done inside the create function and _.extend. We're moving the parts into it.
  • What is affected by this change? => N/A

implementation details

  • At first, I wanted to move everything in this PR, but decided not to. Because it's still really big (+1675/-1667). I decided to break the PR to 3 steps.
    1. Handle outside functions (this PR).
    2. Handle queue and the functions inside _.extend (next PR)
    3. Remove // @ts-nocheck in cy.ts and other outside functions.
  • I moved create out of export default because it requires one more indentation.
  • There are many functions in $Cy that are defined outside cy.ts and merged into it by _.extend. I solved the problem by creating interfaces for them. In the code, you'll find many interfaces like ITimeouts, IJQuery, etc.

How has the user experience changed?

N/A

PR Tasks

N/A

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 1, 2021

Thanks for taking the time to open a PR!

@sainthkh sainthkh force-pushed the refactor-create-into-class-cy branch 2 times, most recently from 8b1d81e to 07b7cb3 Compare November 8, 2021 03:56
@sainthkh sainthkh marked this pull request as ready for review November 8, 2021 05:39
@sainthkh sainthkh requested a review from a team as a code owner November 8, 2021 05:39
@sainthkh sainthkh requested review from jennifer-shehane and removed request for a team November 8, 2021 05:40
@@ -707,6 +707,8 @@ const create = (state, keyboard, focused, Cypress) => {
return mouse
}

export interface Mouse extends ReturnType<typeof create> {}
Copy link
Member

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?

Copy link
Contributor Author

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.

packages/driver/src/cy/aliases.ts Outdated Show resolved Hide resolved

getAvailableAliases,
Copy link
Member

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?

Copy link
Contributor Author

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']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing finishAssertions?

Copy link
Contributor Author

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:

assert: assertions.assert,
verifyUpcomingAssertions: assertions.verifyUpcomingAssertions,

packages/driver/src/cy/ensures.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/ensures.ts Show resolved Hide resolved
// 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
Copy link
Member

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,
  }
}

Copy link
Contributor Author

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TO DO is done? 😄

Copy link
Contributor Author

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.

emilyrohrbough
emilyrohrbough previously approved these changes Nov 15, 2021
packages/driver/src/cypress/cy.ts Outdated Show resolved Hide resolved
@chrisbreiding chrisbreiding merged commit 3817e50 into cypress-io:develop Nov 17, 2021
tgriesser added a commit that referenced this pull request Nov 18, 2021
* 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)
  ...
tgriesser added a commit that referenced this pull request Nov 20, 2021
* 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
  ...
tgriesser added a commit that referenced this pull request Nov 21, 2021
* 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
  ...
tgriesser added a commit that referenced this pull request Nov 21, 2021
…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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants