fromEventPattern() should not magically return different results based on arguments.length

[felixfbecker]

Bug Report

Current Behavior
fromEventPattern currently checks args.length and if it's 1, emits the first item, otherwise it emits an array of all:

rxjs/src/internal/observable/fromEventPattern.ts

Line 153 in d3e7e3f

const handler = (...e: T[]) => subscriber.next(e.length === 1 ? e[0] : e);

This is problematic because it is not type safe and unpredictable.
It could be made slightly more type safe with a conditional type, but even then, if the parameter is optional TS would accept calling the listener with undefined or no parameter at all, which are different args.length and would result in one case in the emission of an array [undefined] and in the other case in undefined, which would not be checked by the type checker.

Checking arguments.length is generally considered bad practice.

What's worse is that you can't even reliably use resultSelector anymore to ensure it is always an array, because the magic switching is always done first, and resultSelector is just a shortcut now that checks if the element is an array. Which means that if the listener was actually called with an array as a single parameter, the resultSelector logic would think it was instead called with multiple parameters and spread the array out.

Expected behavior
fromEventPattern() should just emit a tuple of all parameters. It's trivial to destructure that array or map it to just the first parameter if desired.

Environment

• RxJS version: 6

[cartant]

Yeah. I agree that this behaviour is not ideal. This is something that could be looked at for v7, but not for v6 where it would be a breaking change.

[thorn0]

How about simply replacing e.length === 1 with e.length <= 1 for now? I'd call it a bug fix, not a breaking change.

[benlesh]

Maybe with all of the functions that do this (bindCallback and friends also do this), we could return the result selector, and just the result collector gets args[] but the default, without the result selector will always just be the first argument.

So, let's say your fromEvent source has 3 args: number, string, boolean: fromEvent(thing, 'whatever') would return Observable<number> always. BUT, fromEvent(thing, 'whatever', (a: number, b: string, c: boolean) => of([a, b, c])) would give you Observable<[number, string, boolean]>.

It's an idea. Of course, we'd need to wait a long time to introduce this, because it's a breaking change.

[benlesh]

Core Team Notes: Punting this change to version 8. We can't do this at this stage of the v7 release. This will also give us time to come up with a solid design for how to handle this.