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: Add TypeScript support for Cypress action events #1187

Merged
merged 3 commits into from Jan 16, 2018

Conversation

NicholasBoll
Copy link
Contributor

Fixes #1186

@@ -2934,6 +2942,130 @@ declare namespace Cypress {
(fn: (currentSubject: Subject) => void): Chainable<Subject>
}

/**
* These events come from the application currently under test (your application). These are the most useful events for you to listen to.
* @see https://docs.cypress.io/api/events/catalog-of-events.html#App-Events
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing we REALLY do not want is to put direct urls to any resource, because these are likely to change. The same url should go through our on.cypress.io redirect service. Usually it is just document title, this url would be https://on.cypress.io/catalog-of-events#App-Events

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. Thanks. That was one of my review comments. I was trying to figure out what the redirect URL was

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the URLs. Is there a conventions for those URL redirects? I tried a few including https://on.cypress.io/events/catalog-of-events#App-Events

@bahmutov bahmutov self-requested a review January 11, 2018 18:36
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.

All good except for direct documentation url

* // failing the test
* return false
* })
* @see https://docs.cypress.io/api/events/catalog-of-events.html#App-Events
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should there be linkability to individual events? Right now this will jump to the table of actions. Also I don't know if that is the official URL. I noticed the Cypress team use a redirect URL for Cypress commands. I couldn't find anything that matched that pattern (on.cypress.io/...) that redirected to the correct page. Feel free to suggest a redirect URL.

* })
* @see https://docs.cypress.io/api/events/catalog-of-events.html#App-Events
*/
(action: 'uncaught:exception', fn: (error: Error, runnable: Mocha.IRunnable) => false | void): void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs said it is the mocha runnable. I'm assuming this is the interface the docs meant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included an example only in this action. This action at least gives an template for other actions to have examples added.

* Fires before the test and all **before** and **beforeEach** hooks run.
* @see https://docs.cypress.io/api/events/catalog-of-events.html#App-Events
*/
(action: 'test:before:run', fn: (attributes: ObjectLike, test: Mocha.ITest) => void): void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was hard to tell what this object might look like. I couldn't find a clear emitter of this event. I found emitting of the :async version referenced in socket.coffee. Feel free to reference a file for more clear typing. Right now ObjectLike says there can by any key in the object that has a value of any. Object or object won't work because then it is an object with no properties.

(action: 'test:after:run', fn: (attributes: ObjectLike, test: Mocha.ITest) => void): void
}

// $CommandQueue from `command_queue.coffee` - a lot to type. Might be more useful if it was written in TS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't spend too much time fleshing out this entire interface. There is some implicit method definitions that come straight from lodash. I don't know if it is worth the effort before $CommandQueue is written in TypeScript. I added methods I thought might be important. Feel free to chime in if there are specific methods or properties that should be important to an external developers using the the command action event callbacks

Copy link
Contributor

Choose a reason for hiding this comment

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

absolutely, looks good

@@ -2960,6 +3092,14 @@ declare namespace Cypress {
expiry?: string
}

interface EnqueuedCommand {
name: string
args: any[]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think any is all that we can guarantee. There could be a lot of work to create an interface of internal mapping of named commands like select with argument shapes. I don't know if that is worthwhile right now. It would make more sense if command implementations are written in TypeScript. This at least won't give any errors. I consider these events pretty advanced and will probably require an understanding of the source code anyways. any could always be cast to a more specific type in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

args: any[]
type: string
chainerId: string
fn(...args: any[]): any
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't dig far enough to find out what the shape of this was

@@ -2994,6 +3134,11 @@ declare namespace Cypress {
whitelist: (xhr: any) => boolean
}

interface Viewport {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this in window.coffee

@NicholasBoll
Copy link
Contributor Author

I'll look into that unit test failure. npm run dtslint works locally.

@bahmutov
Copy link
Contributor

Just add "newline-per-chained-call": false to cli/types/tslint.json - new version of typescript@next breaks it. You would get this failure locally if from cli you removed node_modules folder and installed again.

@NicholasBoll
Copy link
Contributor Author

I updated and could reproduce the issue. I set the tslint.json setting and now only have a TypeScript error on typescript@next around the Omit implementation. This is probably a bug in TypeScript that will get resolved since there is otherwise no way of omitting properties of an interface.

@NicholasBoll
Copy link
Contributor Author

It was suggested maybe this PR broke it? microsoft/TypeScript#17912

@bahmutov
Copy link
Contributor

bahmutov commented Jan 11, 2018 via email

@NicholasBoll
Copy link
Contributor Author

I added a fix. The TypeScript team has filed an issue: microsoft/TypeScript#21148

I hope they will have a solution before TypeScript 2.7 comes out (either fixed or a workaround provided to library authors). Right now it works. I guess we'll see what happens later. Worst thing for the Cypress type definitions is changing Omit references to Pick and give a long list of keys to pick rather than a short list of keys to omit.

@NicholasBoll
Copy link
Contributor Author

It looks like the error will be fixed before TypeScript 2.7 ships: microsoft/TypeScript#21156

@brian-mann brian-mann merged commit ae8b147 into cypress-io:develop Jan 16, 2018
@brian-mann brian-mann added this to the 1.4.2 milestone Feb 4, 2018
@brian-mann
Copy link
Member

brian-mann commented Feb 5, 2018

Released in 1.4.2.

@NicholasBoll NicholasBoll deleted the feat/support-actions branch March 28, 2018 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants