chore: Remove pkg/driver //@ts-nocheck part 3 #19837

sainthkh · 2022-01-24T01:05:10Z

Related with chore: refactor create into class $Cy #18715
Next step of chore: remove pkg/driver //@ts-nocheck part 2 #19483

User facing changelog

N/A. It's just removing // @ts-nocheck comments and defining types to avoid type errors.

Additional details

Why was this change necessary? => To make pkg/driver files more type-safe.
What is affected by this change? => N/A
Any implementation details to explain? => N/A

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?

cypress-bot · 2022-01-24T01:05:13Z

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.

ryanthemanuel · 2022-01-25T17:22:52Z

packages/driver/src/cy/commands/navigation.ts

@@ -673,6 +679,8 @@ export default (Commands, Cypress, cy, state, config) => {
              onFail: options._log,
              args: { str },
            })
+
+            return // Error is thrown above, it exists to satisfy typescript


Would it make this cleaner to just return what is on line 678?

The return type of throwErrByPath is void, and 678 always throws. So, line 683 is never called. If we remove this line, TypeScript shows all path doesn't return value error. That's why I left this comment.

microsoft/TypeScript#13219

In other languages like Java, there is throws keyword that handles this situation, but TypeScript isn't. The link above is the request to that feature. It seems that it's forgotten or given low priority.

Looking at this comment, if you update error_utils.throwsErrorByPath to return type never, this should be fine without the extra return type.

microsoft/TypeScript#13625 (comment)

ryanthemanuel · 2022-01-25T17:23:00Z

packages/driver/src/cy/commands/navigation.ts

@@ -685,9 +693,12 @@ export default (Commands, Cypress, cy, state, config) => {
      }

      $errUtils.throwErrByPath('go.invalid_argument', { onFail: options._log })
+
+      return // Error is thrown above, it exists to satisfy typescript


Same as above.

emilyrohrbough · 2022-01-26T15:21:00Z

packages/driver/src/cy/commands/actions/type.ts

@@ -15,7 +14,8 @@ const debug = debugFn('cypress:driver:command:type')
 export default function (Commands, Cypress, cy, state, config) {
  const { keyboard } = cy.devices

-  function type (subject, chars, options = {}) {
+  // TODO: change the type of `any` to `Partial<Cypress.TypeOptions>`
+  function type (subject, chars, options: any = {}) {


What is preventing you from making this change now?

Cypress often extends then user-given options with some internal values like in line 24-34.

cypress/packages/driver/src/cy/commands/actions/type.ts

Lines 24 to 34 in bc87d6e

options = _.defaults({}, userOptions, {

$el: subject,

log: true,

verify: true,

force: false,

delay: config('keystrokeDelay') || $Keyboard.getConfig().keystrokeDelay,

release: true,

parseSpecialCharSequences: true,

waitForAnimations: config('waitForAnimations'),

animationDistanceThreshold: config('animationDistanceThreshold'),

})

And TypeScript treats it as a type error. That's why it's commented.

By the way, I tried to rename user-given options and internal options as opts in #6459. But it was rejected and refactored in the current style. Maybe we need to find a way to name our internal option objects to solve this problem.

Can you add a small notes summarizing what you explained here? It makes sense to me but might not be obvious to others without doing a deeper drive into the code base on the work required here.

The problem is that this comment exists on almost every command. So it's a bit hard to find the right place.

And I'll remove all of them after removing // @ts-nocheck comments. (I believe the part 4 will be the final.)

I estimated when removing TODOs will be done. And it might take 1-2 months in current speed. So, I decided to add a note above.

I guess we need this somewhere in our code.

packages/driver/src/cy/commands/actions/type.ts

emilyrohrbough · 2022-01-26T15:22:30Z

packages/driver/src/cy/commands/connectors.ts

 import _ from 'lodash'
 import Promise from 'bluebird'

 import $dom from '../../dom'
 import $utils from '../../cypress/utils'
-import $errUtils from '../../cypress/error_utils'
+import $errUtils, { CypressError } from '../../cypress/error_utils'

 const returnFalseIfThenable = (key, ...args) => {


Suggested change

const returnFalseIfThenable = (key, ...args) => {

const returnFalseIfThenable = (key, ...args): boolean => {

emilyrohrbough · 2022-01-26T15:25:24Z

packages/driver/src/cy/commands/cookies.ts

@@ -110,7 +108,7 @@ export default function (Commands, Cypress, cy, state, config) {
    })
  }

-  const getAndClear = (log, timeout, options = {}) => {
+  const getAndClear = (log?, timeout?, options = {}) => {


Why is log optional here? it's not optional for automateCookies

Because it's called without them at line 164.

cypress/packages/driver/src/cy/commands/cookies.ts

Line 164 in bc87d6e

return getAndClear()

emilyrohrbough · 2022-01-26T15:26:45Z

packages/driver/src/cy/commands/navigation.ts

+let previousDomainVisited: boolean | null = null
+let hasVisitedAboutBlank: boolean | null = null
+let currentlyVisitingAboutBlank: boolean | null = null
+let knownCommandCausedInstability: boolean | null = null


Seems like the defaults should be updated to false

packages/driver/src/cypress/browser.ts

emilyrohrbough · 2022-01-26T15:40:11Z

packages/driver/src/cypress/error_utils.ts

@@ -17,9 +15,9 @@ const crossOriginScriptRe = /^script error/i

 if (!Error.captureStackTrace) {
  Error.captureStackTrace = (err, fn) => {
-    const stack = (new Error()).stack
+    const stack = (new Error()).stack;


Suggested change

const stack = (new Error()).stack;

const stack = (new Error()).stack

; is necessary here because JavaScript tries to translate .stack\n\n(err as Error) as a function call.

emilyrohrbough · 2022-01-26T15:40:33Z

packages/driver/src/cypress/error_utils.ts

  return _.startsWith(err.message, 'Invalid Chai property')
 }

-const isCypressErr = (err = {}) => {
+const isCypressErr = (err: Error) => {


Suggested change

const isCypressErr = (err: Error) => {

const isCypressErr = (err: Error): boolean => {

emilyrohrbough · 2022-01-26T15:40:51Z

packages/driver/src/cypress/error_utils.ts

@@ -197,20 +195,20 @@ const makeErrFromObj = (obj) => {
  return err2
 }

-const throwErr = (err, options = {}) => {
+const throwErr = (err, options: any = {}) => {


Suggested change

const throwErr = (err, options: any = {}) => {

const throwErr = (err, options: any = {}): never => {

As never means a function never reaches its end, I had to create makeErrFromErr function.

emilyrohrbough · 2022-01-26T15:41:00Z

packages/driver/src/cypress/error_utils.ts

@@ -227,7 +225,7 @@ const throwErr = (err, options = {}) => {
  throw err
 }

-const throwErrByPath = (errPath, options = {}) => {
+const throwErrByPath = (errPath, options: any = {}) => {


Suggested change

const throwErrByPath = (errPath, options: any = {}) => {

const throwErrByPath = (errPath, options: any = {}): never => {

emilyrohrbough · 2022-02-01T14:14:18Z

packages/driver/src/config/jquery.ts

 import _ from 'lodash'

 import { scrollTo } from './jquery.scrollto'
 import $dom from '../dom'

+// Add missing types.
+interface ExtendedJQueryStatic extends JQueryStatic {


Won't this caused typescript errors for individuals using JQuery methods other than find and expr?

nvm. this isn't the types provided via the cli! sorry aboutt that

emilyrohrbough · 2022-02-02T23:34:11Z

@sainthkh can you take a look at the failing tests?

sainthkh · 2022-02-03T04:12:22Z

I fixed the type error by adding !. It's safe because defaultView isn't undefined.
I checked system-tests errors by adding empty commit. But it simply fails because of the timeouts. I cannot understand why. Sometimes, CircleCI fails when outside contributors use them. Maybe it's the case.

…tributors

@ts-nocheck

* develop: feat: gray out the path to system node in cypress run header (#20121) feat: redesign server errors (#20072) test: fix awesome-typescript-loader test and remove test-binary job (#20131) fix: Fix issues with stack traces and command log in Chrome 99 (#20049) fix: `cy.type(' ')` fires click event on button-like elements. (#20067) fix: `change`, `input` events are not fired when the same option is selected again. (#19623) build: publish vue3 on latest (#20099) chore: release @cypress/webpack-preprocessor-v5.11.1 chore: release @cypress/webpack-dev-server-v1.8.1 fix: detect newly added specs in dev-server compilation (#17950) chore: Remove pkg/driver //@ts-nocheck part 3 (#19837) chore: set up semantic-pull-request GitHub Action (#20091) chore: release @cypress/react-v5.12.2 fix: remove nullish coalescing in js files to support node 12 (#20094) docs: update @cypress/webpack-preprocessor links (#19902) refactor: use aliases instead of meta (#19566)

sainthkh marked this pull request as ready for review January 24, 2022 03:37

sainthkh requested a review from a team as a code owner January 24, 2022 03:37

sainthkh requested review from jennifer-shehane and removed request for a team January 24, 2022 03:37

jennifer-shehane removed their request for review January 24, 2022 16:06

jennifer-shehane assigned sainthkh Jan 25, 2022

ryanthemanuel requested review from emilyrohrbough and ryanthemanuel January 25, 2022 16:36

ryanthemanuel reviewed Jan 25, 2022

View reviewed changes

emilyrohrbough reviewed Jan 26, 2022

View reviewed changes

packages/driver/src/cy/commands/actions/type.ts Show resolved Hide resolved

emilyrohrbough reviewed Jan 26, 2022

View reviewed changes

packages/driver/src/cypress/browser.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Jan 26, 2022

View reviewed changes

packages/driver/src/cypress/browser.ts Outdated Show resolved Hide resolved

emilyrohrbough reviewed Jan 26, 2022

View reviewed changes

sainthkh force-pushed the remove-ts-nocheck-part3 branch 2 times, most recently from 07d37f0 to f3fe6cd Compare January 31, 2022 02:54

emilyrohrbough requested review from ryanthemanuel and emilyrohrbough January 31, 2022 16:28

ryanthemanuel previously approved these changes Jan 31, 2022

View reviewed changes

emilyrohrbough reviewed Feb 1, 2022

View reviewed changes

emilyrohrbough previously approved these changes Feb 1, 2022

View reviewed changes

sainthkh added 6 commits February 3, 2022 11:54

commands.ts

f404f16

cookies.ts

89a1f91

error_utils.ts

330913f

location.ts

8e4291e

fix error

61ed8c5

Add note.

4b9b7aa

sainthkh force-pushed the remove-ts-nocheck-part3 branch 5 times, most recently from 8a25a22 to 6844eae Compare February 3, 2022 04:08

sainthkh force-pushed the remove-ts-nocheck-part3 branch from 6844eae to 4c9b618 Compare February 3, 2022 04:13

feedbacks.

b47480f

sainthkh force-pushed the remove-ts-nocheck-part3 branch from 4c9b618 to b47480f Compare February 3, 2022 04:23

Merge branch 'develop' into remove-ts-nocheck-part3

3ff094b

flotwig mentioned this pull request Feb 4, 2022

system-tests jobs are hanging for external contributors #20057

Closed

sainthkh mentioned this pull request Feb 7, 2022

fix: change, input events are not fired when the same option is selected again. #19623

Merged

1 task

jennifer-shehane requested review from ryanthemanuel and emilyrohrbough February 7, 2022 16:37

flotwig added 2 commits February 7, 2022 13:42

chore(contributing): fix network_error_handling_spec for external con…

a7e4e96

…tributors

Merge branch 'develop' into remove-ts-nocheck-part3

d9a91d3

emilyrohrbough approved these changes Feb 8, 2022

View reviewed changes

ryanthemanuel approved these changes Feb 8, 2022

View reviewed changes

Merge branch 'develop' into remove-ts-nocheck-part3

154dca5

emilyrohrbough merged commit bf0d4db into cypress-io:develop Feb 8, 2022

tgriesser added a commit that referenced this pull request Feb 10, 2022

fix: changes from #19837 causing flake in e2e_cookies_spec

9866e2f

tgriesser mentioned this pull request Feb 10, 2022

fix: changes from #19837 causing flake in e2e_cookies_spec #20139

Closed

sainthkh mentioned this pull request Feb 14, 2022

chore: remove pkg/driver //@ts-nocheck final #20169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Remove pkg/driver //@ts-nocheck part 3 #19837

chore: Remove pkg/driver //@ts-nocheck part 3 #19837

sainthkh commented Jan 24, 2022

cypress-bot bot commented Jan 24, 2022

ryanthemanuel Jan 25, 2022

sainthkh Jan 26, 2022

emilyrohrbough Jan 26, 2022

ryanthemanuel Jan 25, 2022

emilyrohrbough Jan 26, 2022

sainthkh Jan 31, 2022

emilyrohrbough Feb 1, 2022

sainthkh Feb 3, 2022

sainthkh Feb 3, 2022

emilyrohrbough Jan 26, 2022

emilyrohrbough Jan 26, 2022

sainthkh Jan 31, 2022

emilyrohrbough Jan 26, 2022

sainthkh Jan 31, 2022

emilyrohrbough Jan 26, 2022

sainthkh Jan 31, 2022

emilyrohrbough Jan 26, 2022

sainthkh Jan 31, 2022

emilyrohrbough Jan 26, 2022

sainthkh Jan 31, 2022

emilyrohrbough Jan 26, 2022

emilyrohrbough Feb 1, 2022

emilyrohrbough Feb 1, 2022

emilyrohrbough commented Feb 2, 2022

sainthkh commented Feb 3, 2022

	options = _.defaults({}, userOptions, {
	$el: subject,
	log: true,
	verify: true,
	force: false,
	delay: config('keystrokeDelay') \|\| $Keyboard.getConfig().keystrokeDelay,
	release: true,
	parseSpecialCharSequences: true,
	waitForAnimations: config('waitForAnimations'),
	animationDistanceThreshold: config('animationDistanceThreshold'),
	})

	const returnFalseIfThenable = (key, ...args) => {
	const returnFalseIfThenable = (key, ...args): boolean => {

	const stack = (new Error()).stack;
	const stack = (new Error()).stack

	const isCypressErr = (err: Error) => {
	const isCypressErr = (err: Error): boolean => {

	const throwErr = (err, options: any = {}) => {
	const throwErr = (err, options: any = {}): never => {

	const throwErrByPath = (errPath, options: any = {}) => {
	const throwErrByPath = (errPath, options: any = {}): never => {

chore: Remove pkg/driver //@ts-nocheck part 3 #19837

chore: Remove pkg/driver //@ts-nocheck part 3 #19837

Conversation

sainthkh commented Jan 24, 2022

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough commented Feb 2, 2022

sainthkh commented Feb 3, 2022