chore: Update type definitions to match API more closely #1048

NicholasBoll · 2017-12-09T02:39:16Z

Addresses some of the suggestions in #1040 and closes #1050

This PR finishes out the Cypress API types using the existing Chainable interface. While not technically correct according to the implementation of Commands, it passes the kitchen sink example.

I have code pending for breaking up the Command interface into smaller and more restrictive interfaces, but that will probably require more discussion.

Changes:

Added mocha types
Added chai types (with sinon-chai and chai-jquery extensions)
Added API methods that were missing
Fixed a few API methods that had pre 1.0 type definitions
Changed Chainable to Chainable<Subject>
Added ChainableArray<Subject> for dealing with Array subjects. Without this, .each and .spread were impossible to type without resorting to any
Added the Kitchen sink tests with a few modifications talked about in TypeScript type definitions #1040

NicholasBoll · 2017-12-09T02:48:34Z

@bahmutov This is the next part. I kept the same Chainable interface, but added Subject to it. This should fill in types to chained stuff.

The following is how it is currently typed:

cy.wrap({ foo: 'bar' })
.then(s => {
  s // $ExpectType { foo: string }
  return s
})
.then(s => {
  s // $ExpectType { foo: string }
})
.its('foo')
.then(s => {
  s // $ExpectType string
})

NicholasBoll · 2017-12-09T04:53:24Z

cli/types/index.d.ts

-    children(options?: LoggableTimeoutable): Chainable
-    children(selector: string, options?: LoggableTimeoutable): Chainable
+    children<E extends Node = HTMLElement>(options?: Partial<Loggable & Timeoutable>): Chainable<JQuery<E>>
+    children<K extends keyof HTMLElementTagNameMap>(selector: K, options?: Partial<Loggable & Timeoutable>): Chainable<JQuery<HTMLElementTagNameMap[K]>>    


This line allows for tag-based selectors to automatically infer the right element. For example:

cy.get('form') .then(el => { el // $ExpectType JQuery<HTMLFormElement> }) .children('input') .then(el => { el // $ExpectType JQuery<HTMLInputElement> })

Of course these can be overridden:

cy.get<HTMLFormElement>('.main-form') .then(el => { el // $ExpectType JQuery<HTMLFormElement> }) .children<HTMLInputElement>('.input') .then(el => { el // $ExpectType JQuery<HTMLInputElement> })

NicholasBoll · 2017-12-09T04:58:55Z

cli/types/index.d.ts

-    contains(num: number | RegExp): Chainable
-    contains(selector: string, text: string, options?: LoggableTimeoutable): Chainable
+    contains(content: string | number | RegExp): Chainable<Subject>
+    contains<E extends Node = HTMLElement>(content: string | number | RegExp): Chainable<JQuery<E>>


These generic overrides are for when the user knows what the element is going to be and needs access to special properties or methods on the HTML element. It is probably going to be pretty rare, but may be required in rare circumstances (maybe a hacked file upload that requires an HTMLInputElement type.

The fallback for all generic overrides is HTMLElement which contains all the generic methods and properties. Also the Subject returned by all Element selectors is a JQuery object which helps to normalize a lot of calls.

NicholasBoll · 2017-12-09T05:03:49Z

cli/types/index.d.ts

     * @see https://on.cypress.io/filter
     */
-    filter(selector: string, options?: LoggableTimeoutable): Chainable
+    filter<K extends keyof HTMLElementTagNameMap>(selector: K, options?: Partial<Loggable & Timeoutable>): Chainable<JQuery<HTMLElementTagNameMap[K]>> // automatically returns the correct HTMLElement type


A way of getting the right element without using a generic would be to use the .filter method:

cy.get('.input') // HTMLElement since we can't infer type .filter('input') // HTMLInputElement since 'input' is a keyof match .then($input => { $el // $ExpectType JQuery<HTMLInputElement> })

That would also always make sure you have an <input> or else the test will fail before it gets there. I prefer this over generic overrides since overriding generics is basically overriding the type system (possible runtime error in that case)

I wonder if, given that info, that should be the way to get a specific element type? I don't like seeing generics in non-library code - it is just too easy to misuse.

Generics would still be required in areas we can't infer based on input (like fixture contents or results from a .as).

yeah, I bet sometimes as <...> would still be needed, unfortunately (or fortunately if you think of the flexibility)

I think generic overrides are necessary sometimes. I think it is a good practice to avoid the requirement. I've seen developers introduce runtime errors by incorrectly overriding them. It can be tricky because type errors can produce very difficult "type traces".

NicholasBoll · 2017-12-09T05:12:48Z

cli/types/index.d.ts

     * @see https://on.cypress.io/not
     */
-    not(selector: string, options?: LoggableTimeoutable): Chainable
+    not<E extends Node = HTMLElement>(selector: string, options?: Partial<Loggable & Timeoutable>): Chainable<JQuery<E>>


We can't narrow the type down much with a .not. I'm not even sure a generic should be used here. HTMLElement is what all elements would have in common. I think I'll remove the generic from this.

NicholasBoll · 2017-12-09T06:35:41Z

cli/types/index.d.ts

     * @see https://on.cypress.io/spread
     */
-    spread(fn: (...args: any[]) => any): Chainable
+    spread(fn: (...args: any[]) => void): Chainable<Subject> // Can't type this any better without breaking up Chainable


These types aren't quite right. I had them working with commands being broken down. Without that, I don't have a way of typing a Subject that is an array as just a Subject again.

I may have to either add a ChainableArray interface or leave anything hitting .spread as any

NicholasBoll · 2017-12-09T07:29:20Z

cli/types/index.d.ts

+  /**
+   * Chainable interface with stronger typing for array subjects
+   */
+  interface ChainableArray<Subject> extends Omit<Chainable<Subject[]>, 'and' | 'as' | 'each' | 'should' | 'then'> {


I added ChainableArray<Subject> to properly capture types for .each and .spread and theoretically any Command that is meant to work on arrays. Without this, .each and .spread would be forced to resort to any for parameters, putting more type burden on the application author.

The Omit part basically takes the whole Chainable interface and removes some keys ('and', 'as', etc in this case). I'm defining the ChainableArray interface to be the same as Chainable except a few methods are replaced with different type definitions. I could simply extend but then the wrong overrides would be chosen.

bahmutov · 2017-12-09T15:58:24Z

cli/types/tests/kitchen-sink.ts

@@ -0,0 +1,1470 @@
+// Samples taken from the cypress kitchen sink example (https://github.com/cypress-io/cypress-example-kitchensink)


is this file automatically passing dtslint because there are no $ExpectType assertions?

dtslint will look for compilation errors as well - unless you use // $ExpectError on the previous line.

You can see the TS compilation error it caught here: https://circleci.com/gh/cypress-io/cypress/7985

It is really cool - you can make assertions if you want errors for something.

bahmutov

Love it!!!

@NicholasBoll how are you so good at TypeScript? Any resources that you can recommend for @brian-mann and me?

NicholasBoll · 2017-12-09T16:14:09Z

@bahmutov I find the blog by Marius Schulz to be an excellent way to learn TypeScript features: https://blog.mariusschulz.com/. I've been keeping up on the release notes of TypeScript as well (I do that with Cypress too 😄)

Really it takes some practice. I forced myself to understand type definitions. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react-redux/index.d.ts is a good example of a small, but dense definition file. Also just spending (wasting?) time trying to make a type better. Sometimes it panned out, sometimes not and a dizzying rabbit hole sometimes.

I didn't understand different modules types very well until working on these type definitions - and how to mix modules types (TypeScript detects module mode based on the use of import/export keywords).

NicholasBoll · 2017-12-09T16:16:24Z

What do you think about the split of Subject and Subject[] paths? I didn't want to go down that route, but I'm guessing .each and .spread are powerful and used enough that justified the work.

.invoke still can't be typed properly, but since it just passes the Subject given to it, the any type doesn't follow the chain like it did with .each and .spread

NicholasBoll · 2017-12-10T12:15:49Z

cli/types/index.d.ts

     */
-    spread(fn: (...args: any[]) => any): Chainable
+    spy(): sinon.SinonSpy & Chainable<sinon.SinonSpy>


@brian-mann The docs say cy.spy returns sinon.spy, but I also see an alias. Here I'm adding all of Chainable to the return, but I'm guessing that isn't correct. I could find the implementation to confirm.

Does cy.spy() and cy.stub have only a special non-chainable as method added to them and nothing else? If so, I should update this type

Correct. They only have a single .as method added to them. This brings them into parity with other Cypress idioms. They show up in the Command Log highlighted with the alias, and it also enables you to use them with cy.get('@alias')

Here's the code.

https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cy/commands/agents.coffee#L189

FWIW there is also a log method which enables you to turn off Command Log logging. This is necessary in situations where the spied / stubbed function is called hundreds of times and would lock up the UI.

Thanks. Not sure what to call the interface. Perhaps Agent? It looks like it returns a Spy/Stub with a chainable interface with just .log and .as.

Fixed. I see that withArgs was overridden, so the Agent interface does the same thing (to allow the continuation of the Agent chainable

NicholasBoll · 2017-12-10T13:30:02Z

I would like to get rid of ChainableArray and push the subtle workarounds onto the developer. Maybe documentation updates can nudge TypeScript users in the right direction. ChainableArray adds a complexity that is difficult to explain and maintain. Knowing that there are long-term plans to move Cypress to TypeScript will make ChainableArray even more difficult to maintain in source .ts files.

Background:

ChainableArray is a convenient concept to help the developer use Cypress in a strongly typed way without thinking about the subtleties of array subjects, but does so at the expense of maintenance on the Cypress team. Also if someone extends Cypress commands, they may also be exposed to this complexity if they return an array from a command.

I'm just not a fan of the complexity introduced by ChainableArray. It solves a problem around 2 chainable methods that I can't properly type any other way: each and spread. Also .its can't return the proper chainable interface (.its('foo') can't, but .then(s => s.foo) can). The problem is I can't extract a type from an array (proposal: microsoft/TypeScript#6606) or branch of type detection (closest proposal: microsoft/TypeScript#12424) - it is just a limitation of TypeScript.

Workarounds:

A developer can get around this in a strongly typed way without having to override generics or specify types:

Spread:

cy.then([first, second]) vs cy.spread(first, second). The TypeScript language understands how to extract the types in the first example, but not the second. The following workaround is pretty clean and reasonable.

cy.wrap([1, 2])
  .then(([first, second]) => {
    first // $ExpectType number
  })

Each:

Don't use cy.each, but a cy.then and a Cypress._.each (This workaround is a bit ugly):

cy.wrap([1, 2])
  .then(subject => {
    Cypress._.each(subject, (item) => {
      item // $ExpectType number
    })
  })

The primary use case for .each seems to be around JQuery-wrapped elements and that is how it is currently typed on Chainable. I can add a fall-through that will allow the following (much cleaner, but requires a type override if the developer is using a non-JQuery item like in this case):

cy.wrap([1, 2])
  .each((item: number) => {
    item // $ExpectType number
  })

NicholasBoll · 2017-12-10T13:51:45Z

I have removed the ChainableArray interface here: 20e9832

@bahmutov Could you review with those changes and let me know what you think: a single Chainable with a few workarounds to .spread and .each mentioned above or a an extra ChainableArray interface that requires a workaround with .its

brian-mann · 2017-12-10T18:19:43Z

Unfortunately cy.each is not analogous to Cypress._.each.

The lodash method is purely synchronous - whereas cy.each will respect things like promises and ensures each callback function runs serially. cy.each also does some nice things under the hood like yielding you wrapped jQuery objects (instead of the underlying raw element) to make it consistent with other commands.

NicholasBoll · 2017-12-10T18:55:05Z

I see the implementation of cy.each here: https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cy/commands/connectors.coffee#L275

If the Subject is a JQuery or Element, the typings are fine as they are now:

cy.get('.someSelector')
  .each(($el, index, list) => {
    $el // $ExpectType JQuery<HTMLElement>
    index // $ExpectType number
    list // $ExpectType HTMLElement[]
  })

If the subject isn't a JQuery-wrapped element, the type must be overridden (since it could be an array of anything):

cy.wrap([1, 2, 3])
  .each((item: number, index, list: number[]) => {
    item // $ExpectType number
    index // $ExpectType number
    list // $ExpectType number[]
  })

Cypress._.each would be fine with synchronous values. I think declaring the type in the .each is okay. To remove the requirement of those type overrides, something like ChainableArray would be required. I think ChainableArray is nice, but it does place a burden on the type implementation. Breaking that implementation accidentally can be mitigate with type tests (just like unit tests). It does present a funny type implementation. Chainable APIs are already a bit funky to type. The following is an example of adding a chainable method:

namespace Cypress {
  interface Chainable<Subject> {
    myMethod<S>(fn: () => S): Chainable<S>

    myMethodThatReturnsAnArray<S>(fn: () => S[]): ChainableArray<S>
  }
}

Cypress.add('myMethod', /** ... */)
Cypress.add('myMethodThatReturnsAnArray', /** */)

I guess that isn't too bad. It is up to you

bahmutov · 2017-12-11T21:22:05Z

I am fine with the user having to provide types once in a while, like you showed

cy.wrap([1, 2, 3])
  .each((item: number, index, list: number[]) => {
    item // $ExpectType number
    index // $ExpectType number
    list // $ExpectType number[]
  })

brian-mann · 2017-12-12T02:05:08Z

Can this be merged in? I see incremental work on it, so wasn't sure if its all ready to go or not.

I see that this PR is closing: #1050

But from the latest commits it appears it's doing more than just what @bahmutov listed on it. Can one of you provide me with a summary of what it's doing so I can write the changelog. Trying to get 1.2.0 out tonight, thanks.

NicholasBoll · 2017-12-12T02:21:43Z

Thanks @bahmutov. I think that is fine for this PR. In #1040 you asked me to break up type definitions into separate and targeted PRs. This one focused on adding missing method APIs and updating existing method APIs. This PR leaves the Chainable interface alone. I think if we split Chainable to include something like ChainableArray, that can be another PR.

@brian-mann One of the commits removed the scope creep to add another interface (that should be part of a different PR). The other fixed the cy.spy and cy.stub types to match the agent.coffee (only chainable .as and .log and wrap .withArgs just like the implementation file)

#1050 summarises the changes well. It is ready to be merged.

NicholasBoll · 2017-12-12T02:22:21Z

Thanks @brian-mann and @bahmutov for the feedback to make the type definitions as accurate as possible.

This reverts commit e9aa9a3. This brings back TypeScript definition that is necessary for Cypress 1.1.4 to pass TS linter

NicholasBoll · 2017-12-15T02:01:24Z

Just a note for @brian-mann or @jennifer-shehane or whoever does the release notes - this change is not breaking for anyone simply consuming Cypress using JavaScript or TypeScript. It will be a breaking change for anyone extending Cypress in TypeScript. The signature of the Chainer interface changed from Chainer to Chainer<Subject>

Maybe a note:

Before:

declare namespace Cypress {
  interface Chainer {
    myMethod(): Cypress.Chainer
  }
}

After:

declare namespace Cypress {
  interface Chainer<Subject> {
    myMethod(): Cypress.Chainer<Subject>
  }
}

brian-mann · 2017-12-15T02:06:44Z

Yeah these kinds of things are definitely critical for us to note. Appreciate it.

…

On Thu, Dec 14, 2017 at 9:01 PM, Nicholas Boll ***@***.***> wrote: Just a note for @brian-mann <https://github.com/brian-mann> or @jennifer-shehane <https://github.com/jennifer-shehane> or whoever does the release notes - this change is not breaking for anyone simply consuming Cypress using JavaScript or TypeScript. It will be a breaking change for anyone *extending* Cypress in TypeScript. The signature of the Chainer interface changed from Chainer to Chainer<Subject> Maybe a note: Before: namespace Cypress { interface Chainer { myMethod(): Cypress.Chainer } } After: namespace Cypress { interface Chainer<Subject> { myMethod(): Cypress.Chainer<Subject> } } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1048 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABNc8K_Tv9IJ0OqJ5VjYF-33A66lRtvWks5tAdL2gaJpZM4Q73mi> .

NicholasBoll · 2017-12-15T02:23:51Z

I've tried to reduce breaking changes when working with type definitions. Sorry for not mentioning earlier. I'll be sure to add notes for the change log as part of type PRs. I think Cypress is great and I want everyone using it to agree.

Honestly, I didn't know people were using TypeScript + Cypress. The @types/cypress were outdated (pre-1.0). I was planning to make a PR against DefinitelyTyped and just when I was about to I saw Gleb add them to Cypress directly. Things are moving fast. I'm glad to see the community providing feedback! I'm glad my work to help our team adopt Cypress + TypeScript is helping others as well.

NicholasBoll added 3 commits December 8, 2017 16:56

chore: Fill out Cypress API types

510b07d

chore: Convert Chainable into a Generic with a subject

92d950c

chore: undefined return of then keeps Subject

6333a14

NicholasBoll added 2 commits December 8, 2017 21:36

fix: Remove incorrect comment

cd8a511

chore: Add element keys to all selector methods

bc45b13

NicholasBoll commented Dec 9, 2017

View reviewed changes

chore: Remove generic from cy.not - it doesn't make sense

e09cf5e

NicholasBoll commented Dec 9, 2017

View reviewed changes

NicholasBoll added 2 commits December 9, 2017 00:11

chore: Add ChainableArray for array Subjects

f8a5b61

chore: Add ChainerArray to thread types through

48ea890

NicholasBoll commented Dec 9, 2017

View reviewed changes

bahmutov reviewed Dec 9, 2017

View reviewed changes

bahmutov approved these changes Dec 9, 2017

View reviewed changes

bahmutov mentioned this pull request Dec 9, 2017

Update TS type definitions to match API more closely #1050

Closed

NicholasBoll commented Dec 10, 2017

View reviewed changes

chore: Remove ChainableArray interface

20e9832

chore: Fix stub/spy types - returns Agent interface

0edb228

NicholasBoll mentioned this pull request Dec 11, 2017

TypeScript type definitions #1040

Closed

5 tasks

Merge branch 'develop' into chore/fill-out-api-types

42fed13

brian-mann merged commit 5d76163 into cypress-io:develop Dec 12, 2017

NicholasBoll deleted the chore/fill-out-api-types branch December 12, 2017 04:15

NicholasBoll mentioned this pull request Dec 12, 2017

chore: Add extra chainer types for auto-complete #1059

Merged

NicholasBoll referenced this pull request in cypress-io/cypress-example-recipes Dec 14, 2017

Revert "no longer need duplicate expect declaration"

a6b1ad5

This reverts commit e9aa9a3. This brings back TypeScript definition that is necessary for Cypress 1.1.4 to pass TS linter

This was referenced Apr 27, 2020

Upgrading Cypress from 4.0.1 to 4.3.0 results in conflicting jquery typedefs #7063

Closed

mocha type definitions conflict with @types/jest #6690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Update type definitions to match API more closely #1048

chore: Update type definitions to match API more closely #1048

NicholasBoll commented Dec 9, 2017 •

edited by bahmutov

Loading

NicholasBoll commented Dec 9, 2017

NicholasBoll Dec 9, 2017

NicholasBoll Dec 9, 2017

NicholasBoll Dec 9, 2017

NicholasBoll Dec 9, 2017

bahmutov Dec 9, 2017

NicholasBoll Dec 9, 2017

NicholasBoll Dec 9, 2017

NicholasBoll Dec 9, 2017

NicholasBoll Dec 9, 2017

NicholasBoll Dec 9, 2017

bahmutov Dec 9, 2017

NicholasBoll Dec 9, 2017

NicholasBoll Dec 9, 2017

bahmutov left a comment

NicholasBoll commented Dec 9, 2017 •

edited

Loading

NicholasBoll commented Dec 9, 2017

NicholasBoll Dec 10, 2017

brian-mann Dec 10, 2017

brian-mann Dec 10, 2017

NicholasBoll Dec 10, 2017

NicholasBoll Dec 11, 2017

NicholasBoll commented Dec 10, 2017 •

edited

Loading

NicholasBoll commented Dec 10, 2017

brian-mann commented Dec 10, 2017

NicholasBoll commented Dec 10, 2017 •

edited

Loading

bahmutov commented Dec 11, 2017

brian-mann commented Dec 12, 2017

NicholasBoll commented Dec 12, 2017

NicholasBoll commented Dec 12, 2017

NicholasBoll commented Dec 15, 2017 •

edited

Loading

brian-mann commented Dec 15, 2017 via email

NicholasBoll commented Dec 15, 2017

		@@ -0,0 +1,1470 @@
		// Samples taken from the cypress kitchen sink example (https://github.com/cypress-io/cypress-example-kitchensink)

chore: Update type definitions to match API more closely #1048

chore: Update type definitions to match API more closely #1048

Conversation

NicholasBoll commented Dec 9, 2017 • edited by bahmutov Loading

NicholasBoll commented Dec 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bahmutov left a comment

Choose a reason for hiding this comment

NicholasBoll commented Dec 9, 2017 • edited Loading

NicholasBoll commented Dec 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicholasBoll commented Dec 10, 2017 • edited Loading

Background:

Workarounds:

Spread:

Each:

NicholasBoll commented Dec 10, 2017

brian-mann commented Dec 10, 2017

NicholasBoll commented Dec 10, 2017 • edited Loading

bahmutov commented Dec 11, 2017

brian-mann commented Dec 12, 2017

NicholasBoll commented Dec 12, 2017

NicholasBoll commented Dec 12, 2017

NicholasBoll commented Dec 15, 2017 • edited Loading

brian-mann commented Dec 15, 2017 via email

NicholasBoll commented Dec 15, 2017

NicholasBoll commented Dec 9, 2017 •

edited by bahmutov

Loading

NicholasBoll commented Dec 9, 2017 •

edited

Loading

NicholasBoll commented Dec 10, 2017 •

edited

Loading

NicholasBoll commented Dec 10, 2017 •

edited

Loading

NicholasBoll commented Dec 15, 2017 •

edited

Loading