fix(fromEvent): infer from Node.js EventEmitter with types

[huan]

Description:

Using fromEvent with a Node.js EventEmitter will get a Observerable<unknown>, as the below TS code shows:

class NodeEventeEmitterTest {
  addListener(eventName: 'foo', listener: (bar: number) => void)      { return this; }
  removeListener(eventName: 'foo', listener: (bar: number) => void )  { return this; }
}
const emitter = new NodeEventeEmitterTest();
const $ = fromEvent(test, 'foo');
// const $: Observable<unknown>

I'll add a unit test first, to demonstrate this situation,
then try to add some code to fix & improve its compatibility.

Related issue (if exists):

• fix(fromEvent): improve typings for JQueryStyleEventEmitter #5584
• fix(fromEvent): Support React Native and node-compatible event sources. #3821
• Proposal: Statically typed `fromEvent` #6338
• Do not specify explicit type parameters. Signatures with type parameters that cannot be inferred will be removed in v8. wechaty/redux#4
• Function argument inference only handles one overload microsoft/TypeScript#26591 (comment)
• Return type inference of overloaded function when argument is known microsoft/TypeScript#27027 (comment)
• Inconsistent inferred overloads in conditional types for no-arg signatures with strictFunctionTypes microsoft/TypeScript#28867
• Can not get return type when util.promisify an function with overloading definations microsoft/TypeScript#26048

The fix for this unit test:

Before the code fix, the unit test shows:

Error: spec/observables/fromEvent-spec.ts(445,13): error TS2322: Type 'boolean' is not assignable to type 'never'.

see GitHub Action

After the code fix

It should be able to turn green, with the correct typing inference: Observable<number> instead of Observable<unknown>

class NodeEventeEmitterTest {
  addListener(eventName: 'foo', listener: (bar: number) => void)      { return this; }
  removeListener(eventName: 'foo', listener: (bar: number) => void )  { return this; }
}
const emitter = new NodeEventeEmitterTest();
const $ = fromEvent(test, 'foo');
// const $: Observable<number>

[huan]

After the latest commit f25a174 it turns out the code has a bug:

if there is more than one event in the emitter, the referenced type is not right.

This problem is because "overload resolution in conditional types is not supported yet" in current versions of TypeScript (said @RyanCavanaugh, from here)

However, there have workarounds that can make it work, only need we write more codes (luckily they will be all typing definitions so there will be no runtime overhead added).

This bug also exists in the JQueryStyleEventEmitter from the current RxJS code base, I'm trying to figure it out now but no clues yet so far, reviews & help are welcome.

[huan]

After two days of hard work, I finally made it!

I want to use fromEvent(nodeEventEmitter, 'eventName') something like that two years ago and find that fromEvent work not well with the Node.js EventEmitter.

I feel strange that why RxJS lack this support, and I talked with @andywer (creator of typed-emitter) and @devanshj (creator of rxjs-from-emitter) for some solutions with their great NPM modules, but still not a very easy-to-understand & out-of-the-box solution.

At the beginning of the last week, I feel it will not too hard to add support to the RxJS itself, and I finally realized that it's not easy after I have dived deep into it. The TypeScript system does not support this because of the design limitation, which turns my work into hard mode and I start to understand that why the RxJS does not support it for the past years. (I guess it's the same reason)

Futurnatelly, there's workarounds, like get return type when util.promisify a function with overloading definations and ReturnType of overloaded function, and finally I have made the CI green.

@benlesh Could you please review this PR and let me know what do you think? I'd like to keep improving it if you have any suggestions, and I'm looking forward to seeing this PR being merged and we can use the auto-infer fromEvent for Node EventEmitters! if you think it's good.

Thanks for reading, cheers!

[benlesh]

These sorts of tests we do with dtslint. They're under a different directory and are run with npm run dtslint.

[huan]

Moved to dtslint

[benlesh]

Same as my other comment. These should be dtslint tests. :)

[huan]

Moved to dtslint

[benlesh]

I'm not sure how these comments will affect our documentation system. Probably should just stick regular doc comments as seen elsewhere in the code base.

[huan]

I will remove them
Removed.

[benlesh]

It would be great if we used more descriptive names here.

[huan]

I agree that the descriptive name will be more readable, and at first, I was using the descriptive name.

However, the reason that I finally use the short name (L for Listener, N for Name, and D for Data) is that the below code will have lots of repeated those type names:

For example:

type EventNameDataPair9<AddRemoveListener> = AddRemoveListener extends L9<
  infer N1,
  infer N2,
  infer N3,
  infer N4,
  infer N5,
  infer N6,
  infer N7,
  infer N8,
  infer N9,
  infer D1,
  infer D2,
  infer D3,
  infer D4,
  infer D5,
  infer D6,
  infer D7,
  infer D8,
  infer D9
>
  ? [N1, D1] | [N2, D2] | [N3, D3] | [N4, D4] | [N5, D5] | [N6, D6] | [N7, D7] | [N8, D8] | [N9, D9]
  : never;

The above code will be very long and I feel the short version of the name is more readable, so I'd like to suggest that we can keep this short name because they are very to be understood:

1. L - Listener
2. N - Name
3. D - Data

On the other hand, I'm OK with using a more descriptive(long) name if you think the long version is better.

Please let me know your final decision and I'll follow it.

[benlesh]

Overall, this is a very interesting addition. I'll flag it for review by the core team, because I would really like @cartant's input here. I know this area is something we wanted to improve. There are a few things you'll need to adjust, @huan, and I think we're likely to bikeshed names and documentation a little bit, but overall, I really like the initiative here.

[huan]

Hi @benlesh ,

It's great to know that you like it!

I have followed your comments and fixed most of them, and will continue dealing with the "descriptive name" once get your new feedback.

Have a great day!

[benlesh]

Core Team: We feel this may add too much complexity to our already complex typings. This might work as a user-land package that adds type definitions though.

[benlesh]

Thank you for all the effort, @huan! Hopefully no hard feelings!

[huan]

No problem, @benlesh , I have learned a lot by building this PR, and understand the decision of the core team.

I'd like to publish this feature to a user-land package, could you please give me some advice for it?

For example:

1. should I create a from export to wrap the existing one in the RxJS?
2. or, just add typing to RxJS from by adding the typing file?
3. any other suggestions?

[benlesh]

@huan it seems like the cleanest solution would be to just add typings files to said package. @cartant, do you have any opinions on that?