andywer / typed-emitter

🔩 Type-safe event emitter interface for TypeScript
MIT License
268 stars 24 forks source link

Enhancement: Provide way for class inheritance #7

Closed yoshigev closed 4 years ago

yoshigev commented 4 years ago

Hi @andywer, Thank you for the great package.

I would like to suggest providing a way for a class to inherit EventEmitter using TypedEmitter. Example:

// package code
function TypedEmitterClass<T>() { return EventEmitter as unknown as new () => TypedEmitter<T>; }

// user's code
interface MessageEvents {
  error: (error: Error) => void,
  message: (body: string, from: string) => void
}

class MessageEmitter extends TypedEmitterClass<MessageEvents>() {
  constructor() {
    super();
    this.emit('message', 'Hi there!', 'no-reply@test.com');
  }
}

The reason for using a function TypedEmitterClass() is to take use of generics. It is also possible to do it without:

const MessageEmitterBase: new () => TypedEmitter<MessageEvents> = EventEmitter;

class MessageEmitter extends MessageEmitterBase {
  constructor() {
    super();
    this.emit('message', 'Hi there!', 'no-reply@test.com');
  }
}

The TypedEmitterClass function can be provided by the package. But even if you decide not to add it to the package, maybe you can at least add such example on the readme.

Thanks, Yehoshua

andywer commented 4 years ago

Hey @yoshigev. Thanks for sharing!

So the reason why the package only provides the types and doesn't integrate with any implementation is as follows:

The idea is that it's not obvious what kind of EventEmitter implementation the library user will go for. In node.js it's probably gonna be the built-in events package, but when used in a front-end project, it could be used with pretty much any implementation. But even in the node.js case I wouldn't bet my house on it.

On the other hand, joining the purely-types-only typed-emitter with a random implementation should be trivial:

class MyEventEmitter extends EventEmitter implements TypedEmitter<MyEvents> {}

Eager to hear your feedback 🙂

yoshigev commented 4 years ago

Thank you @andywer for the fast reply.

I thought of figured out that the package is intentionally declaration-only.

The problem I encountered with the trivial implementation is that it doesn't give type-safety, because the class inherits the full interface of the EventEmitter class it extends. I tried to find a way not to inherit the full interface, but didn't manage to. That means the all the calls to emit within the class will not be validated (as the interface of EventEmitter allows them), and that all the calls to on from outside the class will only be validated if the type of the instantiated object will be TypedEmitter:

class MyEventEmitter extends EventEmitter implements TypedEmitter<MyEvents> {
  constructor() {
    super();
    this.emit('aaa'); // NOT validated
  }
}

const e = new MyEventEmitter();
e.on('aaa', () => {}); // NOT validated

const e1: TypedEmitter<MyEvents> = e;
e1.on('aaa', () => {}); // validated

My solution is that the instantiation of the EventEmitter class will be done through a function/variable that casts it to TypedEmitter, but it leaves the implementation of the original EventEmitter.

BTW, if there is complex inheritance structure (e.g., a base class is an event emitter and a derived class needs to expose additional set of events), my solution will also not work.

Thanks.

andywer commented 4 years ago

Hmm… Have you tried class MyEventEmitter extends (EventEmitter as TypedEmitter<MyEvents>) {} yet?

yoshigev commented 4 years ago

Just tried it and got Type 'TypedEventEmitter<MyEvents>' is not a constructor function type. I've tried adding a constructor to the interface, like:

interface TypedEmitter2<T> extends TypedEmitter<T> {
  new(): TypedEmitter2<T>;
}

But this gave additional errors on classes that are implementing the interface but extending EventEmitter directly.

Another option that did work for me (maybe this example should be added to the readme):

class MyEventEmitter extends (EventEmitter as new () => TypedEmitter<MyEvents>) {}

(Trying to make it generic - class MyEventEmitter<T> extends (EventEmitter as new () => TypedEmitter<T>) {} - resulted with the following error: Base class expressions cannot reference class type parameters.)

andywer commented 4 years ago

Does this work?

type TypedEmitterClass<T> = new () => TypedEmitter<T>

class MyEventEmitter<T> extends (EventEmitter as TypedEmitterClass<T>) {}
yoshigev commented 4 years ago

Again the same error: Base class expressions cannot reference class type parameters.

According to https://github.com/microsoft/TypeScript/issues/24122, this used to work in the past.

I think the solution without the generics is good enough, and no change is needed in the package code.

andywer commented 4 years ago

Cool, let's update the readme then 🙂

andywer commented 4 years ago

See #8.