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

fix event emitter type mismatches #50

Closed

Conversation

paulsmithkc
Copy link

Fixes #49

Adds methods overloads to the Minipass class, which match the signatures required by the EventEmitter base class.

See node:events

@paulsmithkc
Copy link
Author

@isaacs Can we get some eyes on this patch?

@isaacs
Copy link
Owner

isaacs commented May 17, 2023

Is the issue just that symbol isn't allowed?

Ie, couldn't this work instead?

diff --git a/index.d.ts b/index.d.ts
index 86384b0..6437bad 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -69,7 +69,7 @@ export namespace Minipass {
     : ObjectModeOptions
 
   interface EventArguments {
-    [k: string]: unknown[]
+    [k: string | symbol]: unknown[]
   }
   /**
    * The listing of events that a Minipass class can emit.

@isaacs
Copy link
Owner

isaacs commented May 17, 2023

The reason I'm suggesting that is that it's a quite nice type safety feature to have the arguments to event handlers be set as unknown by default for undeclared events, so that they have to be type checked before use, rather than any, which tells TS to ignore them entirely and just lets you do anything.

@isaacs isaacs closed this in 0c7b958 May 17, 2023
@paulsmithkc
Copy link
Author

@isaacs My proposed changes do preserve type safety for known types. However the overloads are needed and must match exactly with the types in Node's EventEmitter class.

@isaacs
Copy link
Owner

isaacs commented May 18, 2023

They don't preserve type safety for undeclared events, though.

What error are you seeing with the latest Minipass publish? I'm using this now in rimraf, glob, and the next-gen version of tap, without any problems.

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.

TypeScript errors in index.d.ts
3 participants