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

TypeScript type definitions #1040

Closed
4 of 5 tasks
NicholasBoll opened this issue Dec 7, 2017 · 11 comments
Closed
4 of 5 tasks

TypeScript type definitions #1040

NicholasBoll opened this issue Dec 7, 2017 · 11 comments
Assignees
Labels
Epic Requires breaking up into smaller issues topic: typescript

Comments

@NicholasBoll
Copy link
Contributor

NicholasBoll commented Dec 7, 2017

This is more of a discussion about the strategy for Cypress having type definitions. @bahmutov mentioned the long term goal is for Cypress to be written in TypeScript.

I had a few proposals for how the types should be structured to maximize type safety and minimize explicit type casting. These changes would start to shape the architecture of the eventual TypeScript code. I don't want to step on toes, but I want types to be correct.

Here are the proposals my type definitions have:

  • Add type-based tests using https://github.com/Microsoft/dtslint - this helps to validate type breakages similar to unit tests
  • Add types to Cypress wrappings like Cypress.$, Cypress._, etc.
  • Break Chainable into 3 separate interfaces: ParentCommand, Command<Subject> and CommandElement<Subject>. There is also a CommandAs<Subject> for cy.route return type. Also Terminal which only has .then. Those names aren't set in stone, but shouldn't be changed once they are created. This makes it so cy.clearCookies().clearCookies() or cy.wrap({ foo: 'bar' }).trigger('mouseover') are type errors and auto completion won't even suggest invalid chains.
    • ParentCommand: Commands that are initiated straight from cy. - starts a chain
    • Command<Subject>: Commands for any subject type. Subject becomes the type for .then.
    • CommandElement<Subject = JQuery>: Commands for chained commands that are meant to work with JQuery-wrapped elements. As far as I can tell, all elements are JQuery-wrapped elements.

What we get with these type definitions:

cy.get('.my-element').then(element => {
  element; // $ExpectType JQuery<HTMLElement>
});

cy.get<HTMLInputElement>('input').then(element => {
  element; // $ExpectType JQuery<HTMLInputElement>
  element[0]; // $ExpectType HTMLInputElement
  element.focus();
});

cy.wrap({ foo: 'bar' }).then(obj => {
  obj; // $ExpectType { foo: 'bar' }
});

// .its is typed-checked with keyof
cy.wrap({ foo: 'bar' }).its('foo').then(subject => {
  subject; // $ExpectType string
});

Additionally (an totally optional), I added chai suggestions for .should and .and using an interface where happy-path string values are defined per command type. This is only for convenience and doesn't actually change or dictate anything. Ex:

// from `CommandElement` chain interface
interface Chainer<Subject> {
  (chainer: 'have.length', value: number): Command<Subject>;
  // etc - standard chai and chai-sinon here - fallbacks for any string
}

interface ChainerElement {
  (chainer: 'be.checked'): CommandElement;
  (chainer: 'be.visible'): CommandElement;
  (chainer: 'have.attr', value?: string): CommandElement;
  // etc - chai-jquery here - fallbacks for any string
}

// usage:
cy.get('.my-element').should('be.visible'); // `be.visible` is a suggestion while typing `be.vis` or hitting Cmd+Space in most editors will give the full list of suggestions.

Gleb's todo list:

@bahmutov bahmutov self-assigned this Dec 7, 2017
@bahmutov
Copy link
Contributor

bahmutov commented Dec 7, 2017

I like everything about it. I have not used dtslint - that seems like a useful tool, we only are using tslint and typescript compiler itself.

Is it possible to split the work into multiple smaller pull requests?

  • start using dtslint
  • add definitions to Cypress.$, Cypress._, etc
  • add missing definitions and docs to the commands as they are now
  • chai assertions
  • split and update AddCommand definition

@NicholasBoll
Copy link
Contributor Author

Definitely. Right now I'm changing everything together, but I'm happy to break it up (was thinking about doing that more incrementally).

At the moment I'm working a bit on the type definitions using dtslint against the kitchen sink: https://github.com/cypress-io/cypress-example-kitchensink. I've noticed a few instances where the kitchen sink doesn't agree with the documentation. A few examples:

  • The kitchen sink uses .should sometimes in the place of .then. I can either add should to a terminal state or the kitchen sink should be updated to use .then(function() { expect(); }
  • cy.then() is in the kitchen sink, but not in docs (as a parent command)
  • cy.server yields a server instance in the kitchen sink, but the docs say it terminates with null.
  • cy.clearLocalStorage() yields LocalStorage in the kitchen sink, but the docs say it terminates with null

I'm going to assume the kitchen sink is correct for now, so probably the docs would need to be updated.

@brian-mann
Copy link
Member

Some things we documented in anticipation of making these changes.

cy.then is going to be removed as a parent command. cy.resolve is being added as an alias of cy.wrap.

There is a technical difference between should and then, and its worth noting that they are not interchangeable - there is always a reason to use one in place of the other. Seeing both utilized is common and correct.

cy.server should yield null not a server instance. Docs are correct - implementation is wrong.

cy.clearLocalStorage should also yield null. Docs are correct - implementation is wrong.

@brian-mann
Copy link
Member

Here are a bunch of random notes for improvements to various commands and inconsistencies with the docs.

  • cy.focus

    • needs { force: true }
    • needs to wait until element is focused / focusable?
  • cy.blur

    • needs to retry until element is focused / focusable?
  • cy.readFile + cy.writeFile

    • need default timeouts
    • as it stands they cannot timeout :/
    • as it stands we are not retrying problems reading the file
      • like permissions, ENOTENT, EISDIR
    • writeFile needs to retry
      • even in cases of permissions, EISDIR, etc
      • this insulates us further from flake
    • readFile
      • the docs say it will retry if ENOENT but i see no code that does this
      • i believe as written it will instantly fail
  • cy.as

    • needs to support options because it supports assertions
    • needs to throw if adding assertions
      • they normally would only run once
    • needs to support ‘not.exist’ which should change its behavior to wait until the property DOESNT exist
  • cy.tick

    • need to support options.log false
    • requirements are that it should be called after clock
  • cy.getCookie

    • does it yield null when the cookie doesn’t exist?
  • cy.within

    • does have requirements to be called on a DOM elements
    • what about assertions? can’t it have assertions?
    • it says it yields the same subject?
  • cy.root

    • instead of root being document it should be
    • or rather... document.rootElement (?)
  • cy.getCookie

    • should retry!
    • just like readFile
  • cy.clearLocalStorage

    • needs to yield null, not the removed storage object(s)
    • needs to clear ALL local storage and not just the storage bound to the current URL / domain
    • needs to use automation API’s to do this
    • needs to yield null, not the keys <— docs have been updated to reflect this, but its not what it does today!
  • cy.server

    • needs to yield null
    • docs have been updated to reflect this, but its not what it does today!
  • cy.debug

    • will this run / retry assertions?
    • will assertions be slurped up from the previous command?
  • cy.each

    • what happens when you add assertions?
    • should it yield the same subject?
    • since its dogfooding thenFn, will this time out if the promise doesn’t resolve?
  • cy.then

    • does it verify upcoming assertions
    • Outer assertions should retry #543
    • needs to retry its immediate upcoming assertions
      • if there have been no cypress commands added?
      • this can get quite complex
  • cy.invoke

    • will this retry the same way as cy.its until the property is a function?
    • it probably should NOT do this, since invoking the function may induce side effects
    • but it should retry if its not a function (?)
  • cy.writeFile

    • should yield null
  • cy.debug

    • use an animated gif

@NicholasBoll
Copy link
Contributor Author

cy.spy and cy.stub are a bit weird. The docs say it just returns sinon.spy and sinon.stub respectively, but the kitchen sink has an .as attached to both. I'll make it return the spy/stub + .as method.

@NicholasBoll
Copy link
Contributor Author

Thanks @brian-mann. I'll update.

@NicholasBoll
Copy link
Contributor Author

@brian-mann Is this a supported use-case (in the kitchen sink, but not the docs):

// Specify the route to listen to method 'POST'
cy.route('POST', '/comments').as('postComment');

cy.wait('@postComment');

// get the route - notice the use of `cy.get`?
cy.get('@postComment').then(function(xhr) {
  expect(xhr.requestBody).to.include('email');
  expect(xhr.requestHeaders).to.have.property('Content-Type');
  expect(xhr.responseBody).to.have.property('name', 'Using POST in cy.route()');
});

That is pretty difficult to type since an alias could be anything. The docs say .get only returns elements even if an alias is used. What is valid according to the docs (and already typed properly):

cy.route('POST', '/comments').as('postComment');

cy.wait('@postComment').then(function(xhr) {
  expect(xhr.requestBody).to.include('email');
  expect(xhr.requestHeaders).to.have.property('Content-Type');
  expect(xhr.responseBody).to.have.property('name', 'Using POST in cy.route()');
});

@brian-mann
Copy link
Member

Using the @ means to reference an alias.

An alias could be: a route, an element, or a primitive / regular object.

https://docs.cypress.io/guides/core-concepts/variables-and-aliases.html#What-You’ll-Learn

That shows some additional use cases. You can alias a fixture and then use cy.get('@theFixture') to access the object structure.

Getting an aliased element has special semantics - it either yields you the element, or if its stale, it'll go query for it agian.

Getting an aliased route also has special semantics - it yields you the associated XHR http request + response data based on the index (or latest one) you've asked for. This case is not super well documented but there are tests for it and it technically works.

@brian-mann
Copy link
Member

So yes, it will be hard to type. It could literally be anything

@NicholasBoll
Copy link
Contributor Author

Aliases are very powerful, but they can't be typed since by nature they mutate this. It could be typed if the chain was continued or a chainable was allowed to return a variable:

// allowing a chain to be assigned to a variable
const foo = cy.wrap('foo');
foo.then(subject => {
  subject; // $ExpectType string
});

// just chaining
cy.wrap('foo').then(subject = {
  subject: // $ExpectType string
});

I don't know enough about the implementation, but I think the first one could be possible. It would probably need some metadata but would be no problem to type.

An option might be to separate some special use cases into different getters. Ex: cy.getRoute gets a route by alias. I can do function overrides in TypeScript, but alias and selector are both strings making it difficult for TypeScript to pick the right one (putting the JQuery one first essentially makes it the default).

I will have to leave it up to generic overrides:

cy.wrap('foo').as('foo');
cy.get<string>('@foo').should('eq', 'foo');

By default, cy.get will return a JQuery wrapped element unless you override the generic with a non JQuery subject. That may be the best we can do. I like to avoid requiring end users to specify generics, but they are sometimes necessary.

For comparison, .invoke is currently untypable in TypeScript due to lack of conditional mapped types (keyof only function values) and lack of a typing the return type of invoking a function type (returnof or type expressions). Aliases and invoke are the only concepts I can't type implicitly.

@jennifer-shehane
Copy link
Member

I will be closing this issue, since we have TypeScript definitions now.

I know there is some discussion in this thread for improvements. If there are features/enhancements regarding TypeScript in the comments that are left to be completed, please open them in a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic Requires breaking up into smaller issues topic: typescript
Projects
None yet
Development

No branches or pull requests

4 participants