fix: Add more precise types to Cypress.Commands

[xumepadismal]

• Closes Typings for Command.add are missing types for the subject #18879
• Closes Cypress.Commands.add type doesn't take into account prevSubject to define the type of the custom command #18940
• Closes Cypress.Commands.overwrite typescript problem appearing in 9.1 #19095

User facing changelog

• Custom command implementations typings take into account prevSubject variants
• Custom command implementations now allows to NOT return a value
• Custom command overwrites typings take into account originalFn function

PR Tasks

• Have tests been added/updated?
• Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
• Has a PR for user-facing changes been opened in cypress-documentation?
• Have API changes been updated in the type definitions?
• N/A Have new configuration options been added to the cypress.schema.json?

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

[emilyrohrbough]

@xumepadismal FYI - #18967 issued by @remcohaszing has similar types changes to Cypress.Commands as well. It looks like it takes prevSubject into account. Where you aware?

@remcohaszing Did you by chance come across this PR?

I'm curious what you both think of each other's proposed changes.

Edit- it actually looks like this draft PR was also opened to tackle similar issues: #18880.

[xumepadismal]

@emilyrohrbough oh I missed these PRs indeed...

Here's my thoughts about the difference.

Regarding the preSubject

PR #18880 seems to propose more future-proof approach to prevSubject typings because it supports other hypothetical options in CommandOptions besides prevSubject.

But on the other hand it doesn't support array of prevSubjects which IS supportred by this PR. You can refer to cypress-tests.ts in Changes of this PR to see the examples of how types are inferred.

Regarding the void

In #18967 return type of fn is just void which is not accurate. We should allow void but not require it.
In my PR it is ReturnType<ChainableMethods[T]> | void which is better correspond the docs.

ChainableMethods vs ConditionalKeys

I'm uncertain about what approach is better but it seems that ConditionalKeys proposed in #18967 will filter off more unapplicable keys from Chainable. I don't think there is a much difference for the consumer though.

[xumepadismal]

I can modify my PR to take into account all the goodies from #18967 and #18880 if this is acceptable and nobody minds :)

[remcohaszing]

I think this PR does everything #18967 does plus more (prevSubject), so I recommend to close that one and merge this instead.

Regarding the void

In #18967 return type of fn is just void which is not accurate. We should allow void but not require it. In my PR it is ReturnType<ChainableMethods[T]> | void which is better correspond the docs.

I used void because it turned out a return type of void | Whatever is equal to a return type of void for callback functions. Technically the unwrapped value of the chainable is also a valid return type.

[xumepadismal]

@remcohaszing

I used void because it turned out a return type of void | Whatever is equal to a return type of void for callback functions.

Hmm I played around this and it seems that TS checks the return type correctly, no? You can return either correct type or nothing. Error if you try to return something inconsistent to interface.

[xumepadismal]

Now this PR should also cover everything in #18880. Hopefully I didn't miss anything, review will be welcome :)

[SimeonC]

Looks like this covers all the cases from my PR #18880 , thanks 👍

Could you add that this closes #18879 as well?

[hgabor995]

Will this section work fine even if 'optional' isn't in the first place?

[xumepadismal]

Just pushed a commit to support this case. I'm little bit surprised the code looks even cleaner with this fix :)

[mrkjdy]

I'm not sure if this is the right place to post this, but I got a chance to checkout the changes in the PR, and when overwriting commands like 'select' it's not obvious that you need to add type parameters to overwrite like this in order for the correct types to be picked up:

Cypress.Commands.overwrite<'select', 'element'>('select', (originalFn, prevSubject, valueOrTextOrIndex, options) => {
  // ...
});

After this PR is merged can the docs be updated to mention this?

[chrisbreiding]

I agree with @mrkjdy that the docs should be updated to reflect these changes. We prefer to do docs updates in tandem with the code updates and then release them together.

@xumepadismal Would you mind opening a PR in the cypress-documentation updating the typescript support doc?

[xumepadismal]

I agree with @mrkjdy that the docs should be updated to reflect these changes. We prefer to do docs updates in tandem with the code updates and then release them together.

@xumepadismal Would you mind opening a PR in the cypress-documentation updating the typescript support doc?

Sorry very busy these days but I'll try to allocate some time till the end of the week to create a PR in the docs

[nitzanashi]

may I ask why the Subject is set to be unknown now?

[NicholasBoll]

This PR introduces issues with Cypress.Commands.overwrite on commands with overloads. There is no way for Typescript to know which overload should be used, so the last overload is resolved. Overall, the PS is nice for Cypress.Commands.add to remove duplicate typing, but it fails for Cypress.Commands.overwrite. If there is no overloads for the command, it works fine and is nice to have originalFn properly typed, but there is no way to choose the overload. I've had to be clever with a visit override to do something on a beforeload event:

Example:

Cypress.Commands.overwrite('visit', (originalFn, urlOrOptions, inputOptions = {}) => {
  let options: typeof urlOrOptions; // weird. Typescript doesn't think `urlOrOptions` could be a `string`

  // usually these types of guards are type guards, but now this is only a runtime guard  
  if (typeof urlOrOptions === 'object') {
    options = urlOrOptions;
  } else {
    // wait, `urlOrOptions` is of type `never` here...
    options = {url: urlOrOptions, ...inputOptions};
  }

  return originalFn({
    ...options,
    onBeforeLoad(win) {
      options.onBeforeLoad?.(win);
      supports(); // prime the ally.js supports cache so it doesn't mess with the cypress-plugin-tab
    },
  });
});

In this example, urlOrOptions is string | Partial<Cypress.VisitOptions> & { url: string } because of the overload, but since the last overload is taken, Typescript now thinks the type is never a string. I can no longer override parameters to type them correctly for internal use unless I cast (Cypress.Commands.overwrite as any) first.

[NicholasBoll]

Also slightly nuanced is the return type of the original function:

Cypress.Commands.overwrite('visit', (originalFn, options) => {
  const foo = originalFn(options);

  return foo
});

In this example, foo is a Bluebird promise, but the types say it is Cypress.Chainable<Cypress.AUTWindow>. I ran into this issue with some overrides:

return Cypress.Promise.try(() => {
  return originalFn(subject, options)
})
.then(value => {
  // value should be `JQuery<HTMLElement>` since the original fn returns a jQuery element
  // but `value` is typed as `Cypress.Chainable` since Bluebird cannot unwrap the subject because the way this is typed
  // says `originalFn` returns `Cypress.Chainable<Subject>` instead of `Subject` or `Bluebird.Promise<Subject>`
})

The problem is Cypress commands normalized around a Cypress.Chainable return type, but the original functions do not have any such normalization.

[elio-maggini]

.