Skip to content

Provide strong types for 'invoke' command (#4022) #4907

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

Merged
merged 7 commits into from
Aug 6, 2019

Conversation

mayfieldiv
Copy link
Contributor

@mayfieldiv mayfieldiv commented Aug 2, 2019

Closes #4022

Provides strong typing for arguments and return type of invoke command. Also ensures functionName is the key of a function on the Subject.

Since typing becomes more strict, this is technically a breaking change, but isn't considered a big deal (see #4907 (comment))

Pre-merge Tasks

  • Have tests been added/updated for the changes in this PR?
  • Have the type definitions been updated with any user-facing API changes?

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2019

CLA assistant check
All committers have signed the CLA.

@jennifer-shehane
Copy link
Member

We don't consider type changes as breaking changes since the typescript checks can always be disabled.

@mayfieldiv mayfieldiv force-pushed the issue-4022 branch 2 times, most recently from 8157e28 to 3607df0 Compare August 3, 2019 01:06
@mayfieldiv mayfieldiv changed the title Add strong return type to 'invoke' command (#4022) Provide strong types for 'invoke' command (#4022) Aug 3, 2019
*
* @see https://on.cypress.io/invoke
*/
invoke(functionName: keyof Subject, ...args: any[]): Chainable<Subject> // don't have a way to express return types yet
invoke<
TActualSubject extends (Subject extends Node | Node[] ? JQuery<Subject> : Subject),
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 reading through #806 (comment) and looking at the implementation of invoke, I added handling for the jQuery wrapping invoke does on its inputs

Copy link
Contributor

Choose a reason for hiding this comment

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

can you add an example here to the jsdoc for the user to understand the API method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a couple examples based on the docs

Copy link
Contributor

@bahmutov bahmutov left a comment

Choose a reason for hiding this comment

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

looks good now

@bahmutov bahmutov merged commit 15685db into cypress-io:develop Aug 6, 2019
@bahmutov
Copy link
Contributor

bahmutov commented Aug 6, 2019

hmm, seems the types of arguments to jQuery element are too strict, these tests in Kitchensink are failing https://circleci.com/gh/cypress-io/cypress/144563

cypress/integration/examples/actions.spec.js:229:22 - error TS2554: Expected 1 arguments, but got 2.

229       .invoke('val', 25)
                         ~~

where the test is simply

cy.get('.trigger-input-range')
  .invoke('val', 25)

@bahmutov
Copy link
Contributor

bahmutov commented Aug 6, 2019

I will revert the merged commit and will reopen the issue, don't want to release a breaking type change. Seems like the type is close, but stops at a single argument there

bahmutov added a commit that referenced this pull request Aug 6, 2019
This reverts commit 15685db.

The type for `invoke` command seems to break jQuery method
that take an argument like

```js
cy.get('.trigger-input-range')
  .invoke('val', 25)
```
grabartley pushed a commit to grabartley/cypress that referenced this pull request Oct 6, 2019
…io#4907)

* add strong return type to 'invoke' command

* refactor invoke for clarity. use dtslint tests

* add strong args types using typescript 3.0+ features

* handle invoke's jquery wrapping of its input

* move invoke tests to namespace

* add example to jsdocs of 'invoke'
grabartley pushed a commit to grabartley/cypress that referenced this pull request Oct 6, 2019
…ypress-io#4907)"

This reverts commit 15685db.

The type for `invoke` command seems to break jQuery method
that take an argument like

```js
cy.get('.trigger-input-range')
  .invoke('val', 25)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strongly typing the invoke function
5 participants