rxjs fromEvent compatibility

[huan]

Hi, @andywer

I hope you are doing well!

Recently I'm enjoying your typed-emitter module, it really nice, I love it!

After I armed all my EventEmitter with the beautiful typed-emitter, I found a compatibility problem with the fromEvent from RxJS: It does not accept our typed-emitter armed emitters.

Here's the minimum reproducible code: (related to wechaty/redux#4)

import { EventEmitter } from 'events'
import TypedEventEmitter  from 'typed-emitter'

import { fromEvent } from 'rxjs'

interface Events {
  foo: (n: number) => void
  bar: (s: string) => void
}

const TypedEmitter = EventEmitter as new () => TypedEventEmitter<Events>
const emitter = new TypedEmitter()

fromEvent(emitter, 'foo')
// Error: Type 'TypedEventEmitter<Events>' is not assignable to type 'JQueryStyleEventEmitter'.

It seems that it's due to the limitation of RxJS itself, here's some discussion about the reason that caused this issue: ReactiveX/rxjs#4891

I try to solve this by using another great typed module rxjs-from-emitter created by @ devanshj , however it seems that we start runing into another TypeScript bug:

type Identity = <T>(x: T) => T;
type Test = Identity extends (x: string) => infer T ? T : never;
// Test is expected to be `string` but is `unknown`

At last, with the help of warm-hearted @devanshj, we have created a fromTypedEvent at devanshj/rxjs-from-emitter#4 (comment) and it works like a charm.

However, because the solution requires a dependence of typed-emitter, so @devanshj does not want to include it in his rxjs-from-emitter module.

My question is: do we have a solution for this problem? If we do not, could we include this support (the code from @devanshj) to our project, so that I can use it by direct import from the module to keep us DRY, instead of creating it for every of my project?

Thank you for your great module again, and looking forward for your reply.

Have a nice day!

[andywer]

Hey @huan! Thanks for the kind words 🙂

Be aware that this is a types-only package, so we cannot actually include any code in here. Maybe the fact that we don't add any runtime code is a good argument to ask @devanshj if it's not worth adding the package after all?

[devanshj]

@andywer Hi Andy!

I completely understand that it's tempting to support typed-emitter in rxjs-from-emitter so that it becomes one-stop solution to deal with emitters and fromEvent especially when typed-emitter is popular and go-to solution for typing emitters.

BUT that would make sense if rxjs-from-emitter goals were helping people to use emitters in rxjs, increase productivity, etc. But those are not exactly the goals of rxjs-from-emitter it's more like a possibility than a solution. It has the philosophy and goal of supporting emitters without having to do anything special (which is not possible yet because of TypeScript) so ootb support of typed-emitter goes against that. If the goal was to provide a solution, support of typed-emitter would have been the first thing I add to it :) If rxjs-from-emitter was part of rxjs I would have been more than happy to drop this goal of mine be more practical and support it, but it's not part of rxjs (yet ;).

But the problem still stands I can think about three solutions here:

1. Suggest rxjs to support typed-emitter itself and it would make sense because the goals I said which are not of rxjs-from-emitter would most probably be the goals of rxjs. That's why they support wide range of emitter interfaces. So given the popularity of typed-emitter they would probably be happy to support it, I can help them with a PR if they agree. Also I think they especially try to should support it as using typed-emitter with fromEvent gives an unwarrant error because of this flaw in the types.

2. Support rxjs's fromEvent in typed-emitter itself via import { fromEvent } from "typed-emitter/rxjs" with { "optionalDependencies": { "rxjs": "*" } } so in this way typed-emitter works as it is for non-rxjs users. This also opens up a possible pattern of supporting other libraries that deal with emitters let's say "typed-emitter/xstream", "typed-emitter/most", etc. I'm not sure if that is in alignment of the goals of typed-emitter. Also I would first suggest to try out solution one.

3. As I said above merge rxjs-from-emitter to rxjs and I'm happy to support typed-emitter ootb

/cc @benlesh @cartant this might be of your interest

[andywer]

Very good points!

About 2. (Support rxjs's fromEvent in typed-emitter itself): Gotta have a good night's sleep about that. I definitely see your point, but I would also want to keep the package typedefs-only.

[devanshj]

Thanks! Yeah, alternatively you can still keep the package typedef only by exporting the type instead of the implementation so user would use it like this:

import { fromEvent as rxjsFromEvent } from "rxjs";
import { FromEvent } from "typed-emitter/rxjs";

export const fromEvent = rxjsFromEvent as FromEvent;

But you would still need to have rxjs as optional dependency for the Observable type. (/me thinks this would have been solved if TypeScript had higher kinded types haha)

[andywer]

@devanshj On the other hand, a "re-export with augmented typedefs"-only runtime under a separate entry point might be fine… Let me think about it for a short bit 😉

[devanshj]

Yeah that could work didn't think about that, would be a much better solution also!

[andywer]

Hey @huan!

Wdyt? Would you like to contribute a PR?
I can do it, too, but might take a little until I find some time.

[huan]

Hi, @andywer,

It's great to know that you'd like to accept this feature!

Of course, I'd like to contribute a PR, will do it when I got some time.

Thank you very much for both of your great ideas and discussions!

[huan]

Hello @andywer ,

Sorry for the long delay before I send PR for adding the FromEvent type.

Please have a look at #19 and let me know what you think.

Basically, I have just copied all codes written by @devanshj from his issue comments and pasted them to the rxjs/index.d.ts.

A noticeable change is that we need to add a new property __events to the TypedEventEmitter which is required by the new code for inferencing our interface successfully.

BTW: I have also submitted another PR to enhance the fromEvent on the RxJS repo, please feel free to comment if you are interested. (However, it seems that PR does not compatible with our TypedEmitter, yet):

• fix(fromEvent): infer from Node.js EventEmitter with types ReactiveX/rxjs#6669