feat: infer argument types in command implementations

[remcohaszing]

Cypress.Commands.add now only accepts commands that have been declared, and
the implementation is now properly typed.

User facing changelog

Custom command implementations are now typed based on the declared custom chainables.

Additional details

This provides type safety when implementing commands.

How has the user experience changed?

Given:

declare namespace Cypress {
  interface Chainable {
    newCommand: (arg: string) => void;
  }
}

Before:

Cypress.Commands.add('newCommand', (arg) => {
  // $ExpectType any
  arg
})

After:

Cypress.Commands.add('newCommand', (arg) => {
  // $ExpectType string
  arg
})

PR Tasks

• Have tests been added/updated?
• Have API changes been updated in the type definitions?

[cypress-bot]

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.

[CLAassistant]

All committers have signed the CLA.

[jennifer-shehane]

@remcohaszing Can you describe the problem that you were encountering before that requires this PR?

It looks like this is making it so that types are required for custom commands. We're concerned this will cause issues for users who are importing custom commands from plugins that are not typed and have not defined the custom command themselves. We don't want them to have an error for a custom command they didn't write. Is this accurate?

[remcohaszing]

It looks like this is making it so that types are required for custom commands. We're concerned this will cause issues for users who are importing custom commands from plugins that are not typed and have not defined the custom command themselves. We don't want them to have an error for a custom command they didn't write. Is this accurate?

This is already the case and TypeScript users such as myself love this. Cypress already has a dedicated section in the documentation for this. This basically means that if someone wishes to use for example cypress-image-snapshot, they also need to npm install @types/cypress-image-snapshot. This isn’t different from using other untyped libraries in TypeScript.

---

This pull request builds on the existing functionality by adding type safety for the implementation of commands, if they are written in TypeScript.

For example, let’s say a user has declared the following command:

namespace Cypress {
  export interface Chainable {
    login: (redirect: string) => void;
  }
}

When using the following implementation in Cypress 8.0.0, the type of redirect is any

Cypress.Commands.add('login', (redirect) => {
  cy.visit('/en/login', { qs: { redirect } });
  cy.get('#email').type(Cypress.env('ACCOUNT_EMAIL'));
  cy.get('#password').type(Cypress.env('ACCOUNT_PASSWORD'));
  cy.get('[type="submit"]').click();
  cy.location('pathname').should('equal', redirect);
});

One could add a type annotation, but it doesn’t even have to match the declared type

Cypress.Commands.add('login', (redirect: boolean /* obviously wrong */) => {
  cy.visit('/en/login', { qs: { redirect } });
  cy.get('#email').type(Cypress.env('ACCOUNT_EMAIL'));
  cy.get('#password').type(Cypress.env('ACCOUNT_PASSWORD'));
  cy.get('[type="submit"]').click();
  cy.location('pathname').should('equal', redirect);
});

With these changes, the type of the redirect property is inferred from the declared chainable, meaning the implementation without a type annotation is type safe and the implementation with a wrong type annotation is reported by TypeScript. This is also what the $ExpectType assertsions mean in the type tests.

[jennifer-shehane]

@remcohaszing Would this be a breaking change for users that have not defined types in custom commands previously? So it will error in that case?

[remcohaszing]

This would not be a breaking change.

Currently TypeScript users have to declare the custom commands in order to use them.

This change will identify commands that have been implemented, but are undeclared and therefor unused.

[chrisbreiding]

This is a nice enhancement 👍 Thanks for the PR!

I agree this is non-breaking.

[chrisbreiding]

Looks like we need to consider this breaking after all. See this type check failure when we test against the kitchensink. Adding commands in .js files with checkJS: true in tsconfig.json causes the failure since the types are stricter now and require type definitions for the custom command implementation, whereas they did not fail before.

That's okay though, we can target this at the next major release (9.0).

[remcohaszing]

Looks like we need to consider this breaking after all. See this type check failure when we test against the kitchensink. Adding commands in .js files with checkJS: true in tsconfig.json causes the failure since the types are stricter now and require type definitions for the custom command implementation, whereas they did not fail before.

I disagree. The situation for type checking with .js and checkJS: true is the same as for .ts files. If the type isn’t declared, then the type check for the implementation will fail just like it already does for usages.

I don’t know where the source code of the kitchensink lives, so I’m not sure what’s going on, but it’s one of the following:

1. Type checking is disabled for the places where these commands are used
2. The commands where type checking failed, are unused.

The proper solution would be to define these types in a TypeScript file, even if the rest is written in JavaScript with the checkJS option enabled.

---

I found the file here while typing this issue. On line 33 the command is used and the TypeScript error is suppressed because it’s undeclared.

[chrisbreiding]

The proper solution would be to define these types in a TypeScript file, even if the rest is written in JavaScript with the checkJS option enabled.

I agree, but if an upgrade to Cypress causes a user to have to change their code in order for their CI workflows to succeed, then it's a breaking change. That's the case here. The kitchensink needs those types defined in order for the type checking to pass with these changes. There could be other users in the same situation.

It's not a big deal. This will still get released, it will just have to wait a little while longer for the 9.0 release.

[remcohaszing]

if an upgrade to Cypress causes a user to have to change their code in order for their CI workflows to succeed, then it's a breaking change.

I agree with this.

That's the case here. The kitchensink needs those types defined in order for the type checking to pass with these changes. There could be other users in the same situation.

I don’t agree with this though. The kitchensink already has a type error, but it’s suppressed. If the suppressed issue is resolved in the kitchensink, then these changes will work.

---

That having said I can wait for Cypress 9. I just prefer to have this additional type safety rather sooner than later.

[cypress-bot]

Released in 9.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v9.0.0, please open a new issue.