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.

Sign up for GitHub

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

Jump to bottom

fix: remove unnecessary eventemitter2 type definitions from cy, Cypress #21286

Merged
merged 8 commits into from
May 11, 2022
Merged

fix: remove unnecessary eventemitter2 type definitions from cy, Cypress #21286

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bff96a4
fix: remove unnecessary eventemittor2 type definitions from cy, Cypress
sainthkh May 2, 2022
1bbdb51
Merge branch 'develop' into issue-20556
flotwig May 2, 2022
e166028
feedback.
sainthkh May 9, 2022
e7ae31b
Merge branch 'develop' into issue-20556
sainthkh May 9, 2022
e9594b4
Merge branch 'develop' into issue-20556
sainthkh May 10, 2022
498dcde
fix trailing whitespace error.
sainthkh May 10, 2022
d8d3f5d
Merge branch 'develop' into issue-20556
tbiethman May 11, 2022
458994b
Merge branch 'develop' into issue-20556
tbiethman May 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion cli/types/cypress-eventemitter.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,23 @@
// Cypress, cy, Log inherits EventEmitter.
type EventEmitter2 = import("eventemitter2").EventEmitter2

interface EventEmitter extends EventEmitter2 {
type event = import("eventemitter2").event
type eventNS = import("eventemitter2").eventNS
type ListenerFn = import("eventemitter2").ListenerFn
type Listener = import("eventemitter2").Listener
type OnOptions = import("eventemitter2").OnOptions

interface CyActions extends Cypress.Actions {
(event: event | eventNS, listener: ListenerFn, options?: boolean|OnOptions): this|Listener
}

interface CyEventEmitter {
once: CyActions
on: CyActions
off: CyActions
emit: EventEmitter2['emit']
removeListener: EventEmitter2['removeListener']
removeAllListeners: EventEmitter2['removeAllListeners']
Copy link
Contributor

Choose a reason for hiding this comment

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

Our docs say that Cypress/cy are "standard Node event emitters": https://docs.cypress.io/api/events/catalog-of-events#Binding-to-Events

I'm wondering if we need to either:

  • Change the wording there if we only intend to support a subset of the API
  • Add more functions here to align with what Node's event emitter provides, like addListener. The other missing APIs are probably less frequently used, but it's hard to say.

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 decided to change the type to Omit<EventEmitter2, 'waitFor'>.

Because other functions are harmless, they're really unlikely to be used in the real world cases, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

proxyTo: (cy: Cypress.cy) => null
emitMap: (eventName: string, args: any[]) => Array<(...args: any[]) => any>
emitThen: (eventName: string, args: any[]) => Bluebird.BluebirdStatic
Expand Down
4 changes: 2 additions & 2 deletions cli/types/cypress-global-vars.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ cy.get('button').click()
cy.get('.result').contains('Expected text')
```
*/
declare const cy: Cypress.cy & EventEmitter
declare const cy: Cypress.cy & CyEventEmitter

/**
* Global variable `Cypress` holds common utilities and constants.
Expand All @@ -19,4 +19,4 @@ Cypress.version // => "1.4.0"
Cypress._ // => Lodash _
```
*/
declare const Cypress: Cypress.Cypress & EventEmitter
declare const Cypress: Cypress.Cypress & CyEventEmitter
14 changes: 14 additions & 0 deletions cli/types/tests/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,17 @@ namespace CypressActionCommandOptionTests {
cy.get('el').click({scrollBehavior: false})
cy.get('el').click({scrollBehavior: true}) // $ExpectError
}

namespace CyEventEmitterTests {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put a comment here (or alongside the types themselves) explaining why we'd expect errors for waitFor/prependListener? Without context, these assertions seem a little random.

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 wrote the reason like below:

// `waitFor` doesn't exist in Node EventEmitter 
// and it confuses the users with `cy.wait`

cy.waitFor() // $ExpectError
cy.prependListener() // $ExpectError
cy.on('random', () => {})
cy.removeAllListeners()
cy.removeListener('a', () => {})

Cypress.waitFor() // $ExpectError
Cypress.prependListener() // $ExpectError
Cypress.on('random', () => {})
Cypress.removeAllListeners()
Cypress.removeListener('a', () => {})
}