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

feat: infer argument types in command implementations #17496

Merged

Conversation

remcohaszing
Copy link
Contributor

@remcohaszing remcohaszing commented Jul 27, 2021

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.Commands.add` now only accepts commands that have been declared, and
the implementation is now properly typed.
@remcohaszing remcohaszing requested a review from a team as a code owner July 27, 2021 10:08
@remcohaszing remcohaszing requested review from jennifer-shehane and kuceb and removed request for a team July 27, 2021 10:08
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jul 27, 2021

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented Jul 27, 2021

CLA assistant check
All committers have signed the CLA.

@jennifer-shehane
Copy link
Member

@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?

@jennifer-shehane jennifer-shehane added the type: breaking change Requires a new major release version label Jul 27, 2021
@remcohaszing
Copy link
Contributor Author

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

@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
Copy link
Contributor Author

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.

@jennifer-shehane jennifer-shehane removed the type: breaking change Requires a new major release version label Jul 30, 2021
chrisbreiding
chrisbreiding previously approved these changes Aug 3, 2021
Copy link
Contributor

@chrisbreiding chrisbreiding left a comment

Choose a reason for hiding this comment

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

This is a nice enhancement 👍 Thanks for the PR!

I agree this is non-breaking.

@chrisbreiding
Copy link
Contributor

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).

@chrisbreiding chrisbreiding changed the base branch from develop to 9.0-release August 5, 2021 13:30
@remcohaszing
Copy link
Contributor Author

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

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

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.

@jennifer-shehane jennifer-shehane added the type: breaking change Requires a new major release version label Aug 6, 2021
@jennifer-shehane jennifer-shehane requested review from chrisbreiding and removed request for kuceb August 6, 2021 14:14
@chrisbreiding chrisbreiding merged commit bfa8347 into cypress-io:9.0-release Aug 10, 2021
@chrisbreiding chrisbreiding mentioned this pull request Aug 10, 2021
7 tasks
appsemble-bot pushed a commit to appsemble/appsemble that referenced this pull request Aug 13, 2021
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 10, 2021

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.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Nov 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: breaking change Requires a new major release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants