Proposal: Statically typed fromEvent

[devanshj]

TLDR: I have implemented a fromEvent alternative that is type-safe and has some additional features. And I want to know if I can open a PR. :)

---

So, as you people might know fromEvent's types are not really great which is fair considering when they were written it wasn't possible to statically type it. But now we can have better types for fromEvent.

So in this light, I implemented rxjs-from-emitter. I recommend checking out the readme to see all it's features, but in short these are it's two major features -

1. Correct Observable inferences corresponding to listener's arguments.
2. Static checking if an event exists or not.

If this implementation is in alignment with RxJS's goals and interests, I would be more than happy to send a PR :) I have answers to all your questions. Also don't get distracted with the change in API, it's the types that are important. To put it in the perspective runtime code is ~100 LOC where as type-level code is ~600 LOC.

Also additional to the type-safety, according to me there are some (respectfully) design-level flaws of fromEvent that rxjs-from-emitter solves which I have documented here.

PS: In case this comes across a publicity stunt to promote my package or whatever, it's not. I love RxJS and I genuinely want to make it better :)
Also I will take this opportunity to thank everyone that constantly works on RxJS to make it better!

[devanshj]

@benlesh Is this any good? 🤔😅

[benlesh]

I think this is really cool. I'm not sure I agree with all of the design choices, but I like the spirit of it. If we did have a design we agreed on, we'd need to have a migration plan. How do we get millions of users there gracefully over a series of major releases? What does the deprecation track look like? Are there code transformations we can do? Etc. RxJS is unfortunately way too popular to make big, breaking changes to API design without careful considerations like that.

[devanshj]

I'm so happy that you find it cool! :D And I really appreciate that you asked these questions instead of just dismissing the proposal.

I think we can at least agree upon the fact that if we want type-safety fromEvent needs to go because it's just too dynamic. I mean consider this –

Let's say we have an emitter emitter1 that is node style and the allowed events are "a" and "b" both will have observable of types Observable<number>. We also have another emitter emitter2 that is dom style and the allowed events are "b" and "c" both both will have observable of types Observable<string> and since they are dom style they also have extra options after passing the listener.

Now imagine someone wrote fromEvent([emitter1, emitter2], ...). Now the second parameter should be of type "b" only because that is common in both. It should return Observable<number | string>. And I don't know what to do about the third argument.

The point I want to make is fromEvent is quite a mess. I'm not saying it's impossible to make it type-safe it's quite possible (I haven't fully tried) but the entropy is too much. And solving the problems that I mention about fromEvent is also quite difficult without changing the API. Also Problem #1 is definitely a bug that should be fixed.
Edit: And I just realised fromEvent takes ArrayLike instead of Array this makes type-safety pretty impossible to implement because we can't map over them.

As a matter of fact I started working on type-safety keeping the API same but when I realised fromEvent also takes array of emitters then things became a little too much. That is when I thought let me roll my own fromEvent-like thing.

And beyond all this what I like about fromEmitter is that it's fundamentally correct – unbiased, doesn't make assumptions and has types that are just strict enough to make the code work and not cause runtime error.

Regarding other design disagreements, if you can be specific we can come to a common ground.

Regarding migration, fromEvent can be replaced by fromEmitter in the following way depending on the usage.

// 01 ---

// before:
fromEvent(emitter, name);

// after:
fromEmitter(emitter).event(name);

// 02 ---

// before:
fromEvent<T>(emitter, name);

// after:
fromEmitter(emitter).event(name) as Observable<T>;

// 03 ---

// before:
fromEvent(emitter, name, options);

// after:
fromEmitter(emitter).event(name, options);

// 04 ---

// before:
fromEvent<T>(emitter, name, options);

// after:
fromEmitter(emitter).event(name, options) as Observable<T>;

// 05 ---

// before:
fromEvent(arrayOfEmitters, name);

// after:
merge(...
    arrayOfEmitters
    .map(e => fromEmitter(e).event(name))
);

// 06 ---

// before:
fromEvent<T>(arrayOfEmitters, name);

// after:
merge(...
    arrayOfEmitters
    .map(e => fromEmitter(e).event(name))
) as Observable<T>;

// 07 ---

// before:
fromEvent(arrayOfEmitters, name, options);

// after:
merge(...
    arrayOfEmitters
    .map(e => fromEmitter(e).event(name, options))
);

// 08 ---

// before:
fromEvent<T>(arrayOfEmitters, name, options);

// after:
merge(...
    arrayOfEmitters
    .map(e => fromEmitter(e).event(name, options))
) as Observable<T>;

// 09 ---

// before:
fromEvent(arrayLikeOfEmitters, name);

// after:
merge(...
    [...arrayLikeOfEmitters]
    .map(e => fromEmitter(e).event(name))
);

// 10 ---

// before:
fromEvent<T>(arrayLikeOfEmitters, name);

// after:
merge(...
    [...arrayLikeOfEmitters]
    .map(e => fromEmitter(e).event(name))
) as Observable<T>;

// 11 ---

// before:
fromEvent(arrayLikeOfEmitters, name, options);

// after:
merge(...
    [...arrayLikeOfEmitters]
    .map(e => fromEmitter(e).event(name, options))
);

// 12 ---

// before:
fromEvent<T>(arrayLikeOfEmitters, name, options);

// after:
merge(...
    [...arrayLikeOfEmitters]
    .map(e => fromEmitter(e).event(name, options))
) as Observable<T>;

This assumes that users are using fromEvent in such a way that fromEmitter doesn't give error like not listening to bogus events, etc. Even if refactoring to fromEmitter gives them errors it's an opportunity to fix the code.

In most cases users can and should remove as Observable<T>. We can't have that by default because it can break the code. Also instead of writing as Observable<T> I guess we should also offer an identity operator that users can use (not code transformers) like following if the runtime overhead is not too much.

const assertType =
    <T>() =>
        ($: Observable<unknown>) =>
            $ as Observable<T>

Also if the merge stuff is too verbose we can have fromEmitters but it will only take homogeneous array of emitters.

I don't know how difficult it will be to make code transformers but I have a feeling it shouldn't be much hard in case of TypeScript. In case of JavaScript I guess we can't really tell if it's a single emitter or array of emitters with 100% accuracy.

Also this refactoring will require spread version of merge which is now available in v7. So I guess we should deprecate fromEvent in v8? and I think we should keep fromEventPattern but have a lint rule that can detect if the code can be refactored to withMethods.

So first step now is to agree upon the design and API. If the current design is good, I can explore writing code transformers.

[Evertt]

Did this ever lead to a change in RxJS? Because I can't see it, but I also haven't seen anyone argue for why not to implement this in RxJS. Well I did see some argument that RxJS is supposed to work both in browsers (using the types from the dom library) and in node (using the types from node) and they argued that it was impossible for make it work for both, without forcing the developer using this function to have to import one of those TS type definition libraries when they don't need them.

Is that the reason this never worked out? When I read that argument I was wondering whether it might be possible to define in package.json to import a different files depending on the environment. So that if the developer write import { fromEvent } from "rxjs" in code that happens to also have lib.dom.d.ts loaded, that then package.json will take fromEvent from "rxjs/internal/observable/fromEvent.dom.ts" and otherwise it will take it from "rxjs/internal/observable/fromEvent.node.ts".

I honestly am just making stuff up here. I don't actually know whether something like that could be possible. But it seems like a feature that could exist, or something like it? Or that we could request to have implemented in a future version of Node and / or Typescript?