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

fix: add missing Cypress.Commands.addAll() types #20894

Merged
merged 6 commits into from Apr 11, 2022

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Apr 4, 2022

User facing changelog

Document Cypress.Commands.addAll().

Additional details

  • Why was this change necessary? => Document Cypress.Commands.addAll() for those who want to add commands at once.
  • What is affected by this change? => N/A
  • Any implementation details to explain? => Change the type of args to any because they can be anything. Otherwise, it breaks the type check.

How has the user experience changed?

They can use Cypress.Commands.addAll.

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 4, 2022

Thanks for taking the time to open a PR!

@sainthkh sainthkh marked this pull request as ready for review April 4, 2022 07:52
@sainthkh sainthkh requested a review from a team as a code owner April 4, 2022 07:52
@sainthkh sainthkh requested review from jennifer-shehane and removed request for a team April 4, 2022 07:52
@@ -467,6 +473,14 @@ declare namespace Cypress {
add<T extends keyof Chainable, S extends PrevSubject>(
name: T, options: CommandOptions & { prevSubject: S[] }, fn: CommandFnWithSubject<T, PrevSubjectMap<void>[S]>,
): void
addAll<T extends keyof Chainable>(args: CommandFnArgs<T>): void
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
addAll<T extends keyof Chainable>(args: CommandFnArgs<T>): void
addAll<T extends keyof Chainable>(fn: CommandFnArgs<T>): void

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 named it args because it's an object with functions. Or should we name it fns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After leaving the comment above, I felt that fns better describes the purpose of the argument than args. I changed it that way.

@@ -24,9 +24,15 @@ declare namespace Cypress {
interface CommandFn<T extends keyof ChainableMethods> {
(this: Mocha.Context, ...args: Parameters<ChainableMethods[T]>): ReturnType<ChainableMethods[T]> | void
}
interface CommandFnArgs<T extends keyof ChainableMethods> {
[name: string]: (this: Mocha.Context, ...args: any) => (ReturnType<ChainableMethods[T]> | void)
Copy link
Member

Choose a reason for hiding this comment

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

why ...args: any instead of ...args: Parameters<ChainableMethods[T]>? Could we re-using the existing CommandFn interface?

interface CommandFnArgs<T extends keyof ChainableMethods> {
   [name: string]: CommandFn<T>
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first, I wrote it this way, because ...args: Parameters<ChainableMethods[T]> doesn't work. TypeScript tries to interpret args as the union type of all possible cy method arg types (i.e. [type of cy.get parameters] | [type of cy.visit parameters] | etc... ).

This happened because <T extends keyof ChainableMethods> part defined after the interface means that this CommandFnArgs interface should use the parameters of one of ChainableMethods (i.e. one of the name of cy methods) for all of its functions. It's not the right behavior.

So, I thought about using generics inside [key] brackets. I realized that the purpose of addAll is to add a new command. We're not reusing any cy command. They should be any, because we don't know what they are. They're defined by users.

Copy link
Member

Choose a reason for hiding this comment

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

Gottchya. That makes sense! Thank you for the explanation!

@BlueWinds BlueWinds requested review from davidmunechika and removed request for jennifer-shehane April 6, 2022 15:34
@emilyrohrbough emilyrohrbough changed the title feat: Cypress.Commands.addAll() types fix: add missing Cypress.Commands.addAll() types Apr 11, 2022
@emilyrohrbough emilyrohrbough merged commit 4815a56 into cypress-io:develop Apr 11, 2022
tgriesser added a commit that referenced this pull request Apr 14, 2022
* 10.0-release: (25 commits)
  fix: stop running spec when switching specs (#21038)
  fix: remove asset size warnings and enable nuxt e2e tests (#21074)
  feat: swap the #__cy_root id selector to become data-cy-root for component mounting (#20951)
  fix: Doc changes around vue2 (#21066)
  feat: Add vue2 package from npm/vue/v2 branch (#21026)
  skip failing test
  fix: add possible frameworks to object API config (#21056)
  fix snapshot spacing
  fix system test
  fix snapshot
  update snapshots
  rename spec files
  rename files
  update snapshots
  release 9.5.4 [skip ci]
  fix(regression): cy.pause() should not be ignored with `cypress run --headed --no-exit` (#20877)
  fix: add missing Cypress.Commands.addAll() types (#20894)
  chore: Don't store video and screenshot artifacts for runs (#20979)
  chore: Update Chrome (beta) to 101.0.4951.26 (#20957)
  chore: remove parallelism from test-binary-against-repo jobs (#21004)
  ...
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.

Cypress.Commands.add with object argument
3 participants