-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix: remove unnecessary eventemitter2 type definitions from cy, Cypress #21286
Conversation
Thanks for taking the time to open a PR!
|
@@ -87,3 +87,17 @@ namespace CypressActionCommandOptionTests { | |||
cy.get('el').click({scrollBehavior: false}) | |||
cy.get('el').click({scrollBehavior: true}) // $ExpectError | |||
} | |||
|
|||
namespace CyEventEmitterTests { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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`
cli/types/cypress-eventemitter.d.ts
Outdated
off: CyActions | ||
emit: EventEmitter2['emit'] | ||
removeListener: EventEmitter2['removeListener'] | ||
removeAllListeners: EventEmitter2['removeAllListeners'] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
User facing changelog
Remove unnecessary
eventemitter2
type definitions fromcy
andCypress
.Additional details
eventemitter2
definitions fromcy
andCypress
. Especially,cy.waitFor
is really confusing withcy.wait
for beginners.How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?cypress.schema.json
?