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.

Sign up for GitHub

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

Jump to bottom

chore: refactor cy funcs #19080

Merged

jennifer-shehane merged 24 commits into cypress-io:develop from sainthkh:refactor-cy-funcs

Dec 8, 2021

Contributor

sainthkh commented Nov 23, 2021 •

edited

Loading

Related with chore: refactor create into class $Cy #18715

User facing changelog

N/A. It's a refactoring.

Additional details

Why was this change necessary? => To use modern syntax to define a class, not _.extend().
What is affected by this change? => N/A
Any implementation details to explain? => I moved cy functions to class $Cy.

How has the user experience changed?

N/A

PR Tasks

[na] Have tests been added/updated?
[na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?
[na] Have new configuration options been added to the cypress.schema.json?

Contributor

cypress-bot bot commented Nov 23, 2021

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

sainthkh force-pushed the refactor-cy-funcs branch 3 times, most recently from a9905ac to 7bd9315 Compare

November 25, 2021 02:24

sainthkh marked this pull request as ready for review

November 26, 2021 00:44

sainthkh requested a review from a team as a code owner

November 26, 2021 00:44

sainthkh requested review from jennifer-shehane and removed request for a team

November 26, 2021 00:44

jennifer-shehane removed their request for review

November 29, 2021 16:37

pstakoun requested review from emilyrohrbough and tbiethman

November 29, 2021 16:41

emilyrohrbough reviewed

View reviewed changes

packages/driver/src/cypress.ts

@@ @@ -14,7 +14,7 @@ import browserInfo from './cypress/browser' @@
               import $scriptUtils from './cypress/script_utils'
               import $Commands from './cypress/commands'
-              import $Cy from './cypress/cy'
+              import { $Cy } from './cypress/cy'

Member

emilyrohrbough Nov 29, 2021

Suggested change

      
            import { $Cy } from './cypress/cy'
          
            import { Cy } from './cypress/cy'

Contributor Author

sainthkh Nov 30, 2021

The name of the internal class is $Cy, not Cy.

emilyrohrbough reviewed

View reviewed changes

packages/driver/src/cypress/command_queue.ts

Comment on lines -87 to -89

-                  const add = (command) => {
-                    queue.add(command)
-                  }

Member

emilyrohrbough Nov 29, 2021

Did this get dropped on accident?

Contributor Author

sainthkh Nov 29, 2021

This can be deleted because we are using inheritance in the new code.

emilyrohrbough reviewed

View reviewed changes

packages/driver/src/cypress/command_queue.ts

+                timeout: any
+                whenStable: any
+                cleanup: any
+                fail: any

Member

emilyrohrbough Nov 29, 2021 •

edited

Loading

Shouldn't timeout, cleanup, fail, be of type func?

Contributor Author

sainthkh Nov 30, 2021

Precisely, they are $Cy['timeout'], $Cy['cleanup'], $Cy['fail']. But the problem is that it creates a circular reference problem because command_queue is required in cy.ts.

The goal of this PR is migrating functions inside _.extend(cy, {}) to $Cy class. So, I left them out for the later PR.

emilyrohrbough reviewed

View reviewed changes

packages/driver/src/cypress/command_queue.ts

+                whenStable: any
+                cleanup: any
+                fail: any
+                isCy: any

Member

emilyrohrbough Nov 29, 2021

Should the types match what is documented?

Contributor Author

sainthkh Nov 30, 2021

You're right. I'll handle this in the part 3 of this PR series.

emilyrohrbough reviewed

View reviewed changes

packages/driver/src/cypress/command_queue.ts

+              export class CommandQueue extends Queue<Command> {
+                state: any
+                timeout: any
+                whenStable: any

Member

emilyrohrbough Nov 29, 2021

Isn't this a promise type?

Contributor Author

sainthkh Nov 30, 2021 •

edited

Loading

Yes, it's a function that returns a Promise (code here).

As I said above, it's left out for the part 3.

I decided to do this later because this PR is big enough for now(+1000/-1000) and there are some hard things like state below and other problems hidden by //@ts-nocheck.

emilyrohrbough reviewed

View reviewed changes

packages/driver/src/cypress/command_queue.ts

-                  const logs = (filter) => {
-                    let logs = _.flatten(_.invokeMap(queue.get(), 'get', 'logs'))
+              export class CommandQueue extends Queue<Command> {
+                state: any

Member

emilyrohrbough Nov 29, 2021

Should this be the GetterSetter type? https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cypress/setter_getter.d.ts

Contributor Author

sainthkh Nov 30, 2021

The type of state function isn't that easy. To make it useful for our development, we need to create a new definition like:

interface StateFunc {
  (param: 'document') => Document
  (param: 'done') => boolean
 // etc..
}

I left it as any to do this work in the next PR.

emilyrohrbough reviewed

View reviewed changes

packages/driver/src/cypress/cy.ts

-                    // push these onto the beginning of the commands array
-                    memo.unshift(command)
+                  const wrap = function (firstCall) {
+                    fn = cy.commandFns[name]

Member

emilyrohrbough Nov 29, 2021

Suggested change

      
                  fn = cy.commandFns[name]
          
                  fn = this.commandFns[name]

Contributor Author

sainthkh Nov 30, 2021

It should be cy because wrap function is not an arrow function. When we use this, this tries to find the value connected with wrap, not the instance of class $Cy.

sainthkh added 12 commits

November 30, 2021 08:58


          move state('specWindow'), warnMixingPromisesAndCommands

b24618f


          move isCy, runnableCtx, urlNavigationEvent

157e60d


          Queue, CommandQueue.

84ff477


          fix failures.

16413d4


          move queue

199fe2a


          isStopped, contentWindowListeners.

ad14e42


          enqueue, getCommandsUntilFirstParentOrValidSubject

932501d


          fix failures.

e0d97bd


          pushSubjectAndValidatel, doneEarly

5aea317


          initialize

acd429d


          fix failures.

e70b8da


          stop, reset

64e2599

sainthkh added 10 commits

November 30, 2021 08:58


          addCommandSync addChainer addCommand now

8e5c30c


          replayCommandsFrom

8d1caa4


          onBeforeAppWindowLoad

51d5515


          bind public functions.

981ad75


          onUncaughtException

15d15bd


          fix error.

e35af20


          fix error.

abd2037


          setRunnable

e7ae213


          remove create + private

45807fa


          Remove TODO comment.

7ed0d70

sainthkh force-pushed the refactor-cy-funcs branch from 26dbd3e to 7ed0d70 Compare

November 30, 2021 00:36

tbiethman reviewed

View reviewed changes

packages/driver/src/cypress.ts

    
                  // create cy and expose globally

                  this.cy = $Cy.create(specWindow, this, this.Cookies, this.state, this.config, logFn)

                  this.cy = new $Cy(specWindow, this, this.Cookies, this.state, this.config)

Contributor

tbiethman Nov 30, 2021

For curious reviewers, logFn isn't used by the constructor (or even the create function at this point) so it's safe to omit.

tbiethman reviewed

View reviewed changes

packages/driver/src/util/queue.ts

-              export default {
-                create: <T>(queueables: T[] = []) => {
-                  let stopped = false
+              export class Queue<T> {

Contributor

tbiethman Nov 30, 2021

Can we continue to have this be the default export?

Contributor Author

sainthkh Nov 30, 2021 •

edited

Loading

There is no written or public guideline for Cypress project. But default is usually removed, because it's a translation of module.exports = {}.

Can I ask you why we should use default for this?

Contributor

tbiethman Dec 1, 2021

It was previously the default export, so I was just curious why it changed. For files that have a single export that is sufficiently described by the file name, I have preferred a default export over an individual one.

But without an active lint rule or other guidelines to enforce it, this is fine if it's consistent with other files in the package.

flotwig self-requested a review

December 1, 2021 16:35

tbiethman approved these changes

View reviewed changes

Contributor

tbiethman left a comment

I took the changes for a test drive and saw no issues locally 👍

flotwig approved these changes

View reviewed changes

emilyrohrbough and others added 2 commits

December 6, 2021 18:22


          Merge branch 'develop' into refactor-cy-funcs

1e418ea


          Merge branch 'develop' into refactor-cy-funcs

65a64d7

jennifer-shehane merged commit d18878f into cypress-io:develop

sainthkh mentioned this pull request

chore: Remove pkg/driver @ts-nocheck part 1 #19353

Merged

tgriesser added a commit that referenced this pull request


          Merge branch 'develop' into 10.0-release

223d5fa

* develop:
  chore(deps): update dependency ssri to 6.0.2 [security] (#19351)
  chore: Fix server unit tests running on mac by using actual tmp dir (#19350)
  fix: Add more precise types to Cypress.Commands (#19003)
  fix: Do not screenshot or trigger the failed event when tests are skipped (#19331)
  fix (#19262)
  fix: throw when writing to 'read only' properties of `config` (#18896)
  fix: close chrome when closing electron (#19322)
  fix: disable automatic request retries (#19161)
  chore: refactor cy funcs (#19080)
  chore(deps): update dependency @ffmpeg-installer/ffmpeg to v1.1.0 🌟 (#19300)

tgriesser added a commit that referenced this pull request


          Merge branch '10.0-release' into tgriesser/10.0-release/refactor-life…

92842dd

…cycle

* 10.0-release:
  build: remove syncRemoteGraphQL from codegen
  chore: fix incorrect type from merge
  build: allow work with local dashboard (#19376)
  chore: Test example recipes against chrome (#19362)
  test(unify): Settings e2e tests (#19324)
  chore(deps): update dependency ssri to 6.0.2 [security] (#19351)
  fix: spec from story generation, add deps for install (#19352)
  chore: Fix server unit tests running on mac by using actual tmp dir (#19350)
  fix: Add more precise types to Cypress.Commands (#19003)
  fix: Do not screenshot or trigger the failed event when tests are skipped (#19331)
  fix (#19262)
  fix: throw when writing to 'read only' properties of `config` (#18896)
  fix: close chrome when closing electron (#19322)
  fix: disable automatic request retries (#19161)
  chore: refactor cy funcs (#19080)
  chore(deps): update dependency @ffmpeg-installer/ffmpeg to v1.1.0 🌟 (#19300)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet