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: remove command type todos #20601

Merged
merged 37 commits into from Apr 22, 2022
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
bff7f0b
Remove @ts-ignore.
sainthkh Mar 14, 2022
93f2b2b
clock.ts
sainthkh Mar 14, 2022
a0fbd97
cookies.ts
sainthkh Mar 14, 2022
e9120ad
debugging.ts
sainthkh Mar 14, 2022
9216026
files.ts
sainthkh Mar 14, 2022
edb7d39
exec.ts
sainthkh Mar 14, 2022
0c62852
location.ts
sainthkh Mar 14, 2022
ce302e8
misc.ts
sainthkh Mar 14, 2022
585fbe8
type.ts
sainthkh Mar 14, 2022
b650488
browser.ts
sainthkh Mar 14, 2022
9801c7c
angular.ts
sainthkh Mar 14, 2022
f4adb3a
navigation.ts
sainthkh Mar 14, 2022
b17da05
screenshot.ts
sainthkh Mar 14, 2022
0dfffc6
task.ts
sainthkh Mar 14, 2022
6674167
window.ts
sainthkh Mar 14, 2022
9cf79c8
focus.ts
sainthkh Mar 15, 2022
0659e3b
scroll.ts
sainthkh Mar 15, 2022
f03249a
scroll.ts
sainthkh Mar 15, 2022
95cc205
select.ts
sainthkh Mar 15, 2022
c8face7
submit.ts
sainthkh Mar 15, 2022
021774c
focused.ts
sainthkh Mar 15, 2022
5493561
querying.ts
sainthkh Mar 15, 2022
6b8c4be
root.ts .
sainthkh Mar 15, 2022
2d4191c
fix
sainthkh Mar 15, 2022
d2b2dd8
querying.ts
sainthkh Mar 18, 2022
136d989
feedback
sainthkh Mar 18, 2022
8fb7463
fix screenshot.ts test bug.
sainthkh Mar 18, 2022
33f0885
fix _log type.
sainthkh Apr 14, 2022
ea2126b
Merge branch 'develop' into remove-command-type-todos
flotwig Apr 15, 2022
b43ca6a
Merge branch 'develop' into remove-command-type-todos
jennifer-shehane Apr 18, 2022
7fcf88a
Merge branch 'develop' into remove-command-type-todos
rachelruderman Apr 19, 2022
c8e33fd
log to _log when we're manipulating `_log`.
sainthkh Apr 20, 2022
35c9445
feedback
sainthkh Apr 20, 2022
fc8e114
Use a better type with generics.
sainthkh Apr 20, 2022
4bd4536
Correct type.
sainthkh Apr 20, 2022
01de010
Merge branch 'develop' into remove-command-type-todos
tbiethman Apr 20, 2022
cf50abb
Merge branch 'develop' into remove-command-type-todos
flotwig Apr 22, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/types/cypress.d.ts
Expand Up @@ -2514,7 +2514,7 @@ declare namespace Cypress {
action: 'select' | 'drag-drop'
}

interface BlurOptions extends Loggable, Forceable { }
interface BlurOptions extends Loggable, Timeoutable, Forceable { }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cy.blur supports timeout but it was missing in types. I added it.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, what does .timeout actually do in cy.blur? it gets passed to Cypress.log, but shouldn't this be a synchronous operation? maybe we need to remove this if it's a no-op.

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 think it's a synchronous operation and harder to finish blur() over 4 seconds.

But it might be here because we need it for retry and verifyAssertions.

And it's also in the documentation. Removing it is a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

IDK, I feel like if it's a no-op currently, removing the option isn't really breaking. But it's ok, don't need to worry about it here.


interface CheckOptions extends Loggable, Timeoutable, ActionableOptions {
interval: number
Expand Down
Expand Up @@ -761,9 +761,9 @@ describe('src/cy/commands/screenshot', () => {
cy.get('.short-element').within(() => {
cy.screenshot({ capture: 'runner' })
}).then(() => {
// the runner was captured
expect(Cypress.action.withArgs('cy:before:screenshot').args[0][1].appOnly).to.be.true
expect(Cypress.automation.withArgs('take:screenshot').args[0][1].capture).to.equal('viewport')
// the runner was captured ("appOnly === true" means to hide the runner UI)
expect(Cypress.action.withArgs('cy:before:screenshot').args[0][1].appOnly).to.be.false
expect(Cypress.automation.withArgs('take:screenshot').args[0][1].capture).to.equal('runner')
Comment on lines -764 to +766
Copy link
Contributor Author

@sainthkh sainthkh Mar 18, 2022

Choose a reason for hiding this comment

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

According to the definition of appOnly below,

// "app only" means we're hiding the runner UI
const isAppOnly = ({ capture }) => {
return (capture === 'viewport') || (capture === 'fullPage')
}

it means to hide the runner UI. In other words, it should be false to capture the runner UI.

This weird test failure happened because

  1. appOnly exists only for testing.
  2. And the original code was trying to check the options given with options after userOptions alias is created.

screenshot (subject, name, options: any = {}) {
let userOptions = options
if (_.isObject(name)) {
userOptions = name
name = null
}
// make sure when we capture the entire test runner
// we are not limited to "within" subject
// https://github.com/cypress-io/cypress/issues/14253
if (options.capture !== 'runner') {

The code above, when name is { capture: 'runner' } and options is undefined. options.capture !== 'runner' is always true.

})
})

Expand Down
30 changes: 20 additions & 10 deletions packages/driver/src/cy/commands/actions/focus.ts
Expand Up @@ -4,16 +4,29 @@ import $dom from '../../../dom'
import $utils from '../../../cypress/utils'
import $errUtils from '../../../cypress/error_utils'
import $elements from '../../../dom/elements'
import type { Log } from '../../../cypress/log'

interface InternalFocusOptions extends Partial<Cypress.Loggable & Cypress.Timeoutable> {
_log?: Log
$el: JQuery
error: boolean
verify: boolean
}

interface InternalBlurOptions extends Partial<Cypress.BlurOptions> {
_log?: Log
$el: JQuery
$focused: JQuery
error: boolean
verify: boolean
}

export default (Commands, Cypress, cy) => {
return Commands.addAll({ prevSubject: ['element', 'window'] }, {
// TODO: any -> Partial<Cypress.Loggable & Cypress.Timeoutable>
focus (subject, options: any = {}) {
const userOptions = options

focus (subject, userOptions: Partial<Cypress.Loggable & Cypress.Timeoutable> = {}) {
// we should throw errors by default!
// but allow them to be silenced
options = _.defaults({}, userOptions, {
const options: InternalFocusOptions = _.defaults({}, userOptions, {
$el: subject,
error: true,
log: true,
Expand Down Expand Up @@ -85,13 +98,10 @@ export default (Commands, Cypress, cy) => {
return verifyAssertions()
},

// TODO: any -> Partial<Cypress.BlurOptions>
blur (subject, options: any = {}) {
const userOptions = options

blur (subject, userOptions: Partial<Cypress.BlurOptions> = {}) {
// we should throw errors by default!
// but allow them to be silenced
options = _.defaults({}, userOptions, {
const options: InternalBlurOptions = _.defaults({}, userOptions, {
$el: subject,
$focused: cy.getFocused(),
error: true,
Expand Down
39 changes: 23 additions & 16 deletions packages/driver/src/cy/commands/actions/scroll.ts
Expand Up @@ -5,6 +5,7 @@ import Promise from 'bluebird'
import $dom from '../../../dom'
import $utils from '../../../cypress/utils'
import $errUtils from '../../../cypress/error_utils'
import type { Log } from '../../../cypress/log'

const findScrollableParent = ($el, win) => {
const $parent = $dom.getParent($el)
Expand All @@ -28,12 +29,26 @@ const isNaNOrInfinity = (item) => {
return _.isNaN(num) || !_.isFinite(num)
}

interface InternalScrollIntoViewOptions extends Partial<Cypress.ScrollToOptions> {
_log?: Log
$el: JQuery
$parent: any
axis: string
offset?: object
}

interface InternalScrollToOptions extends Partial<Cypress.ScrollToOptions> {
_log?: Log
$el: any
x: number
y: number
error?: any
axis: string
}

export default (Commands, Cypress, cy, state) => {
Commands.addAll({ prevSubject: 'element' }, {
// TODO: any -> Partial<Cypress.ScrollToOptions>
scrollIntoView (subject, options: any = {}) {
const userOptions = options

scrollIntoView (subject, userOptions: Partial<Cypress.ScrollToOptions> = {}) {
if (!_.isObject(userOptions)) {
$errUtils.throwErrByPath('scrollIntoView.invalid_argument', { args: { arg: userOptions } })
}
Expand All @@ -49,7 +64,7 @@ export default (Commands, Cypress, cy, state) => {
$errUtils.throwErrByPath('scrollIntoView.multiple_elements', { args: { num: subject.length } })
}

options = _.defaults({}, userOptions, {
const options: InternalScrollIntoViewOptions = _.defaults({}, userOptions, {
$el: subject,
$parent: state('window'),
log: true,
Expand Down Expand Up @@ -115,9 +130,6 @@ export default (Commands, Cypress, cy, state) => {
const scrollIntoView = () => {
return new Promise((resolve, reject) => {
// scroll our axes
// TODO: done() came from jQuery animate(), specifically, EffectsOptions at misc.d.ts
// The type definition should be fixed at @types/jquery.scrollto.
// @ts-ignore
return $(options.$parent).scrollTo(options.$el, {
axis: options.axis,
easing: options.easing,
Expand Down Expand Up @@ -157,10 +169,8 @@ export default (Commands, Cypress, cy, state) => {
})

Commands.addAll({ prevSubject: ['optional', 'element', 'window'] }, {
// TODO: any -> Partial<Cypress.ScrollToOptions>
scrollTo (subject, xOrPosition, yOrOptions, options: any = {}) {
scrollTo (subject, xOrPosition, yOrOptions, userOptions: Partial<Cypress.ScrollToOptions> = {}) {
let x; let y
let userOptions = options

// check for undefined or null values
if (xOrPosition === undefined || xOrPosition === null) {
Expand Down Expand Up @@ -261,7 +271,7 @@ export default (Commands, Cypress, cy, state) => {
$errUtils.throwErrByPath('scrollTo.multiple_containers', { args: { num: $container.length } })
}

options = _.defaults({}, userOptions, {
const options: InternalScrollToOptions = _.defaults({}, userOptions, {
$el: $container,
log: true,
duration: 0,
Expand Down Expand Up @@ -361,10 +371,7 @@ export default (Commands, Cypress, cy, state) => {

const scrollTo = () => {
return new Promise((resolve, reject) => {
// scroll our axis'
// TODO: done() came from jQuery animate(), specifically, EffectsOptions at misc.d.ts
// The type definition should be fixed at @types/jquery.scrollto.
// @ts-ignore
// scroll our axis
$(options.$el).scrollTo({ left: x, top: y }, {
axis: options.axis,
easing: options.easing,
Expand Down
16 changes: 10 additions & 6 deletions packages/driver/src/cy/commands/actions/select.ts
Expand Up @@ -5,13 +5,19 @@ import $dom from '../../../dom'
import $utils from '../../../cypress/utils'
import $errUtils from '../../../cypress/error_utils'
import $elements from '../../../dom/elements'
import type { Log } from '../../../cypress/log'

const newLineRe = /\n/g

interface InternalSelectOptions extends Partial<Cypress.SelectOptions> {
_log?: Log
$el: JQuery<HTMLSelectElement>
error?: any
}

export default (Commands, Cypress, cy) => {
Commands.addAll({ prevSubject: 'element' }, {
// TODO: any -> Partial<Cypress.SelectOptions>
select (subject, valueOrTextOrIndex, options: any = {}) {
select (subject, valueOrTextOrIndex, userOptions: Partial<Cypress.SelectOptions> = {}) {
if (
!_.isNumber(valueOrTextOrIndex)
&& !_.isString(valueOrTextOrIndex)
Expand All @@ -28,9 +34,7 @@ export default (Commands, Cypress, cy) => {
$errUtils.throwErrByPath('select.invalid_array_argument', { args: { value: JSON.stringify(valueOrTextOrIndex) } })
}

const userOptions = options

options = _.defaults({}, userOptions, {
const options: InternalSelectOptions = _.defaults({}, userOptions, {
$el: subject,
log: true,
force: false,
Expand All @@ -55,7 +59,7 @@ export default (Commands, Cypress, cy) => {
},
})

options._log.snapshot('before', { next: 'after' })
options._log!.snapshot('before', { next: 'after' })
}

let node
Expand Down
15 changes: 9 additions & 6 deletions packages/driver/src/cy/commands/actions/submit.ts
Expand Up @@ -5,14 +5,17 @@ import $dom from '../../../dom'
import $utils from '../../../cypress/utils'
import $errUtils from '../../../cypress/error_utils'
import $actionability from '../../actionability'
import type { Log } from '../../../cypress/log'

interface InternalSubmitOptions extends Partial<Cypress.Loggable & Cypress.Timeoutable>{
_log?: Log
$el: JQuery<HTMLFormElement>
}

export default (Commands, Cypress, cy) => {
Commands.addAll({ prevSubject: 'element' }, {
// TODO: any -> Partial<Cypress.Loggable & Cypress.Timeoutable>
submit (subject, options: any = {}) {
const userOptions = options

options = _.defaults({}, userOptions, {
submit (subject, userOptions: Partial<Cypress.Loggable & Cypress.Timeoutable> = {}) {
sainthkh marked this conversation as resolved.
Show resolved Hide resolved
const options: InternalSubmitOptions = _.defaults({}, userOptions, {
log: true,
$el: subject,
})
Expand All @@ -35,7 +38,7 @@ export default (Commands, Cypress, cy) => {
},
})

options._log.snapshot('before', { next: 'after' })
options._log!.snapshot('before', { next: 'after' })
}

if (!options.$el.is('form')) {
Expand Down
33 changes: 20 additions & 13 deletions packages/driver/src/cy/commands/actions/type.ts
Expand Up @@ -8,23 +8,33 @@ import $utils from '../../../cypress/utils'
import $errUtils from '../../../cypress/error_utils'
import $actionability from '../../actionability'
import $Keyboard from '../../../cy/keyboard'
import type { Log } from '../../../cypress/log'

import debugFn from 'debug'
const debug = debugFn('cypress:driver:command:type')

interface InternalTypeOptions extends Partial<Cypress.TypeOptions> {
_log?: Log
$el: JQuery
ensure?: object
verify: boolean
interval?: number
}

interface InternalClearOptions extends Partial<Cypress.ClearOptions> {
_log?: Log
ensure?: object
}

export default function (Commands, Cypress, cy, state, config) {
const { keyboard } = cy.devices

// Note: These "change type of `any` to X" comments are written instead of changing them directly
// because Cypress extends user-given options with Cypress internal options.
// These comments will be removed after removing `// @ts-nocheck` comments in `packages/driver`.
// TODO: change the type of `any` to `Partial<Cypress.TypeOptions>`
function type (subject, chars, options: any = {}) {
const userOptions = options
function type (subject, chars, userOptions: Partial<Cypress.TypeOptions> = {}) {
let updateTable

// allow the el we're typing into to be
// changed by options -- used by cy.clear()
options = _.defaults({}, userOptions, {
const options: InternalTypeOptions = _.defaults({}, userOptions, {
$el: subject,
log: true,
verify: true,
Expand Down Expand Up @@ -110,7 +120,7 @@ export default function (Commands, Cypress, cy, state, config) {
},
})

options._log.snapshot('before', { next: 'after' })
options._log!.snapshot('before', { next: 'after' })
}

if (options.$el.length > 1) {
Expand Down Expand Up @@ -572,11 +582,8 @@ export default function (Commands, Cypress, cy, state, config) {
})
}

// TODO: change the type of `any` to `Partial<ClearOptions>`
function clear (subject, options: any = {}) {
const userOptions = options

options = _.defaults({}, userOptions, {
function clear (subject, userOptions: Partial<Cypress.ClearOptions> = {}) {
const options: InternalClearOptions = _.defaults({}, userOptions, {
log: true,
force: false,
waitForAnimations: config('waitForAnimations'),
Expand Down
12 changes: 7 additions & 5 deletions packages/driver/src/cy/commands/angular.ts
Expand Up @@ -3,9 +3,14 @@ import $ from 'jquery'
import Promise from 'bluebird'

import $errUtils from '../../cypress/error_utils'
import type { Log } from '../../cypress/log'

const ngPrefixes = ['ng-', 'ng_', 'data-ng-', 'x-ng-']

interface InternalNgOptions extends Partial<Cypress.Loggable & Cypress.Timeoutable> {
_log?: Log
}

export default (Commands, Cypress, cy, state) => {
const findByNgBinding = (binding, options) => {
const selector = '.ng-binding'
Expand Down Expand Up @@ -89,10 +94,7 @@ export default (Commands, Cypress, cy, state) => {
}

Commands.addAll({
// TODO: Change the options type from `any` to `Partial<Cypress.Loggable & Cypress.Timeoutable>`.
ng (type, selector, options: any = {}) {
const userOptions = options

ng (type, selector, userOptions: Partial<Cypress.Loggable & Cypress.Timeoutable> = {}) {
sainthkh marked this conversation as resolved.
Show resolved Hide resolved
// what about requirejs / browserify?
// we need to intelligently check to see if we're using those
// and if angular is available through them. throw a very specific
Expand All @@ -102,7 +104,7 @@ export default (Commands, Cypress, cy, state) => {
$errUtils.throwErrByPath('ng.no_global')
}

options = _.defaults({}, userOptions, { log: true })
const options: InternalNgOptions = _.defaults({}, userOptions, { log: true })

if (options.log) {
options._log = Cypress.log({
Expand Down
3 changes: 1 addition & 2 deletions packages/driver/src/cy/commands/clock.ts
Expand Up @@ -42,8 +42,7 @@ export default function (Commands, Cypress, cy, state) {
})

return Commands.addAll({ type: 'utility' }, {
// TODO: change the options type from `any` to Partial<Loggable>.
clock (subject, now, methods, options: any = {}) {
clock (subject, now, methods, options: Partial<Cypress.Loggable> = {}) {
let userOptions = options
const ctx = state('ctx')

Expand Down