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

Conversation

sainthkh
Copy link
Collaborator

@sainthkh sainthkh commented Mar 14, 2022

User facing changelog

N/A. It's an internal cleanup. I removed command type TODOs.

Additional details

  • Why was this change necessary? => I had to add a lot of TODOs on commands when trying to remove // @ts-ignore comments. They had to be cleaned up.
  • What is affected by this change? => Nothing.

Any implementation details to explain?

Long ago, I tried to name internally-extended options with opts in #6459. opts and options are a bit confusing and this idea was turned down. We decided to use options and userOptions.

But the problem is that TypeScript doesn't like extending objects. To do that, we need to use general types like any, Record, etc.

To solve this problem, we need a way to name this internally-extended options. I thought about a lot of name candidates like iOptions, internalOptions, etc. But decided to use _options.

Traditionally, private class members were named with initial _ (link). I think it's simple name for our purpose (options with internal private members).

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

@cypress-bot cypress-bot bot commented Mar 14, 2022

Thanks for taking the time to open a PR!

@sainthkh sainthkh force-pushed the remove-command-type-todos branch from 1265cd0 to abac9fd Compare Mar 15, 2022
interface BlurOptions extends Loggable, Forceable { }
interface BlurOptions extends Loggable, Timeoutable, Forceable { }
Copy link
Collaborator Author

@sainthkh sainthkh Mar 15, 2022

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
Member

@flotwig flotwig Apr 13, 2022

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
Collaborator Author

@sainthkh sainthkh Apr 14, 2022

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
Member

@flotwig flotwig Apr 14, 2022

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.

@sainthkh sainthkh marked this pull request as ready for review Mar 15, 2022
@sainthkh sainthkh requested a review from as a code owner Mar 15, 2022
@sainthkh sainthkh requested review from jennifer-shehane (assigned from cypress-io/end-to-end) and removed request for Mar 15, 2022
@sainthkh sainthkh force-pushed the remove-command-type-todos branch from abac9fd to 67dcf9b Compare Mar 15, 2022
@@ -109,16 +117,15 @@ export default (Commands, Cypress, cy, state) => {
return verifyAssertions()
},

// TODO: change the type of `any` to `Partial<Cypress.WriteFileOPtions & Cypress.Timeoutable>`
writeFile (fileName, contents, encoding, options: any = {}) {
writeFile (fileName, contents, encoding, options: Partial<Cypress.WriteFileOptions & Cypress.Timeoutable> = {}) {
Copy link
Member

@emilyrohrbough emilyrohrbough Mar 16, 2022

Choose a reason for hiding this comment

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

It would be easier to review this if we renamed the parameter instead of changing options to _options. The majority of the changes would be adding the types then.

Copy link
Collaborator 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.

I changed options to userOptions and _options to options.

I was trying not to change the parameter name, because I thought it would be great to generate type definitions automatically from the source code.

But I realized that it is almost impossible for us because the first subject parameter should be specified on our commands but they don't exist in our API.

So, it doesn't matter to change those names.

This PR focuses on fixing the type TODOs.

In the next PR, other un-typed options should be renamed to userOptions and types should be specified.

@sainthkh sainthkh force-pushed the remove-command-type-todos branch from 4052c73 to 136d989 Compare Mar 18, 2022
// 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')
Copy link
Collaborator 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.

@tbiethman tbiethman requested review from flotwig and removed request for jennifer-shehane Apr 13, 2022
@flotwig flotwig requested a review from emilyrohrbough Apr 13, 2022
Copy link
Member

@flotwig flotwig left a comment

nice, i love a good TS improvement PR. just a couple of questions

interface BlurOptions extends Loggable, Forceable { }
interface BlurOptions extends Loggable, Timeoutable, Forceable { }
Copy link
Member

@flotwig flotwig Apr 13, 2022

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.

packages/driver/src/cy/commands/files.ts Outdated Show resolved Hide resolved
@flotwig flotwig self-requested a review Apr 14, 2022
@tbiethman tbiethman requested review from tbiethman and removed request for emilyrohrbough Apr 14, 2022
@@ -973,13 +974,13 @@ export default (Commands, Cypress, cy, state, config) => {
}

if (options.log) {
Copy link
Contributor

@tbiethman tbiethman Apr 14, 2022

Choose a reason for hiding this comment

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

Might be worth changing the check at this point since we're fairly removed from the initial setting of _log.

Suggested change
if (options.log) {
if (options._log) {

Copy link
Collaborator Author

@sainthkh sainthkh Apr 18, 2022

Choose a reason for hiding this comment

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

log and _log can be a bit different.

  • log is a user-provided or default option.
  • _log can be undefined if some conditions are met.

I personally think changing log to _log doesn't change much in this case, but I believe it's safer to leave it as-is.

Copy link
Member

@emilyrohrbough emilyrohrbough Apr 19, 2022

Choose a reason for hiding this comment

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

+1. Agree with @tbiethman, at this point _log is referencing the Log instance created by the command when the user options are log = true

Copy link
Collaborator Author

@sainthkh sainthkh Apr 20, 2022

Choose a reason for hiding this comment

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

I changed log to _log when we're trying to manipulate the _log.

Copy link
Contributor

@tbiethman tbiethman left a comment

Just had the one minor comment

@sainthkh
Copy link
Collaborator Author

@sainthkh sainthkh commented Apr 18, 2022

Failure caused by #20377. Fix at #21106.

@@ -12,6 +12,7 @@ import type { HTMLTextLikeElement } from '../dom/elements'
import $selection from '../dom/selection'
import $utils from '../cypress/utils'
import $window from '../dom/window'
import type { Log } from '../cypress/log'
Copy link
Member

@emilyrohrbough emilyrohrbough Apr 19, 2022

Choose a reason for hiding this comment

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

These should pull from the existing Log Types

Suggested change
import type { Log } from '../cypress/log'
- import type { Log } from '../cypress/log'

Copy link
Collaborator Author

@sainthkh sainthkh Apr 20, 2022

Choose a reason for hiding this comment

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

It's not possible because the Log class under driver/src/cypress/log.ts has some internal methods.

Here are some examples:

if (options._log) {
consoleObj = options._log.invoke('consoleProps')
}

const l = cmd.getLastLog()
if (l) {
l.merge(log)

@@ -680,7 +681,7 @@ export interface typeOptions {
force?: boolean
simulated?: boolean
release?: boolean
_log?: any
_log?: Log
Copy link
Member

@emilyrohrbough emilyrohrbough Apr 19, 2022

Choose a reason for hiding this comment

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

Suggested change
_log?: Log
_log?: Cypress.Log

Copy link
Collaborator Author

@sainthkh sainthkh Apr 20, 2022

Choose a reason for hiding this comment

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

Please check the comment above.

@@ -973,13 +974,13 @@ export default (Commands, Cypress, cy, state, config) => {
}

if (options.log) {
Copy link
Member

@emilyrohrbough emilyrohrbough Apr 19, 2022

Choose a reason for hiding this comment

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

+1. Agree with @tbiethman, at this point _log is referencing the Log instance created by the command when the user options are log = true

packages/driver/src/cy/commands/actions/submit.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/commands/angular.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/commands/cookies.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/commands/cookies.ts Outdated Show resolved Hide resolved
import type { Log } from '../../cypress/log'

interface InternalReadFileOptions extends Partial<Cypress.Loggable & Cypress.Timeoutable> {
_log?: Log
Copy link
Member

@emilyrohrbough emilyrohrbough Apr 19, 2022

Choose a reason for hiding this comment

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

Suggested change
_log?: Log
_log?: Cypress.Log

Copy link
Collaborator Author

@sainthkh sainthkh Apr 20, 2022

Choose a reason for hiding this comment

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

Please check the comment above.

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

interface InternalDocumentOptions extends Partial<Cypress.Loggable & Cypress.Timeoutable> {
Copy link
Member

@emilyrohrbough emilyrohrbough Apr 19, 2022

Choose a reason for hiding this comment

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

Can we reuse this interface?

Copy link
Collaborator Author

@sainthkh sainthkh Apr 20, 2022

Choose a reason for hiding this comment

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

I've chose this way because they're for different commands. They can grow differently. And it's only 2 of them (it's not 3 strike out yet.)

/// <reference types="jquery"/>

-interface ScrollToOptions {
+interface ScrollToOptions extends JQuery.EffectsOptions<any> {
Copy link
Member

@emilyrohrbough emilyrohrbough Apr 19, 2022

Choose a reason for hiding this comment

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

There wasn't a dep update. Did we need to update this patch?

Copy link
Collaborator Author

@sainthkh sainthkh Apr 20, 2022

Choose a reason for hiding this comment

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

It's because of the code below:

return $(options.$parent).scrollTo(options.$el, {
axis: options.axis,
easing: options.easing,
duration: options.duration,
offset: options.offset,
done () {
return resolve(options.$el)
},
fail () {
// its Promise object is rejected
try {
return $errUtils.throwErrByPath('scrollTo.animation_failed')
} catch (err) {
return reject(err)
}
},
always () {
if (parentIsWin) {
delete options.$parent.contentWindow
}
},
})
})

ScrollToOptions uses Jquery.EffectsOptions methods like done(), always(), but they don't exist without extending it.

@emilyrohrbough
Copy link
Member

@emilyrohrbough emilyrohrbough commented Apr 19, 2022

@sainthkh FYI - there are a BUNCH of log types coming this week! You'll appreciate them 😄 https://github.com/cypress-io/cypress/pull/18075/files#diff-a2cd504f16ad316e4675f37a2b3d9d7032d6cc6e3b591934a7b453b16a7ad924R10

@tbiethman tbiethman requested a review from emilyrohrbough Apr 21, 2022
@flotwig flotwig merged commit 0bb655e into cypress-io:develop Apr 22, 2022
53 checks passed
@sainthkh sainthkh mentioned this pull request Apr 25, 2022
4 tasks
@sainthkh sainthkh mentioned this pull request May 9, 2022
1 task
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.

None yet

6 participants