fix(expand): operator function signature

[dilame]

Currently, expand operator have incorrect signature. From the description it applies the projection function to every source value as well as every output value, that means the project function can receive both – upstream observable output and it's own output. But based on typings it only receive upstream observable output, which is dramatically incorrect!
And it's the reason why current expand function body contains as type-casting.

This pull request addresses this issue. The changes made include:

• Corrected the function signature for the expand operator
• Removed hacky type-casting within the function body by fixing the high-level signature

These changes improve type safety and prevent potential issues related to incorrect type-casting in the expand operator implementation.

Related issue:

#7131

[benlesh]

@dilame lint failed.

[dilame]

Thanks for answer, @benlesh . Can you help me, how can i resolve it?
I executed "npm run lint" before the commit, but it was ok

[dilame]

oh, sorry, i found it

[dilame]

Well, one more attempt.
I've refined the patch, enhancing the type inference for the expand function in a more idiomatic TypeScript way. The update includes a new ExpandProject type definition:

export type ExpandProject<T, O> = (value: T | (unknown extends O ? never : O), index: number) => ObservableInput<O>;

This change doesn't fix every type inference issue with expand (it seems to be impossible), but it significantly improves the accuracy in several scenarios. Importantly, it does this without breaking any existing functionality.

For instance, let's take the following example:

function test(): Observable<string> {
  return of(1).pipe(
    expand(x => String(x))
    // ^? x is of type (number | string)
  );
}

The type of x will now be correctly inferred as (number | string) in this case because of explicit function return type (Observable<string>).

The same applies when you just manually specify the generics as follows:

of(1).pipe(
    expand<number, string>(x => String(x))
  );

A noteworthy aspect of this enhancement is the elimination of the need for complex data structures in generics, like ObservableInput. Instead, you can directly pass primitive types, simplifying the code and improving readability.

In all other cases, the behavior of expand remains unchanged, thus preserving backward compatibility.

[dilame]

By the way, @benlesh , I noticed that most of the RxJS operators' generics are based on extends ObservableInput<unknown> and : OperatorFunction<T, ObservedValueOf<O>>. However, it's entirely possible to simplify them with project: (value: T, index: number) => ObservableInput<O> and keep the generics straightforward.

What are your thoughts on refactoring all such cases? I could do it or at least assist with it.

This change would significantly improve readability while preserving backward compatibility, except in cases where users have already manually specified the generics in their userland code like this:

  mergeMap<string, ObservableInput<number>>()

I think it would be preferable to be able to write code like this:

  mergeMap<string, number>()

[benlesh]

@dilame IIRC, those changes were made because of cases where an inner observable return might have a union type like concatMap(() => Observable<string> | Observable<number>) which needs to result in Observable<string | number>. In older versions of TypeScript it couldn't figure that out without what you're currently reading. I'm unsure if that has changed.

[benlesh]

cc @cartant @kolodny

[dilame]

Dear RxJS maintainers,

I've come across a fundamental issue with TypeScript while trying to describe the expand function in a manner that would both reflect its actual behavior and allow for automatic type inference.

In its current state, TypeScript can infer a function's return type based on its argument type but cannot infer an argument type based on the return type (the chicken or the egg problem). As a result, if we have:

declare function expand<O>(project: (value: O) => O): O;

The type O would always become unknown upon inference because TypeScript is reasonably starting from (and ending up with) the type of the argument value, which remains unknown unless the generic is manually specified. On the other hand, if we had:

declare function expand<O>(project: (i: unknown) => O): O;

Here O is always correctly inferred because TypeScript now relies on the return value of project.

To circumvent this limitation, and align expand more closely with traditional recursion, I propose an important change to the expand operator's signature, which makes the project function's return type consistent with its input:

export function expand<T>(
  project: (value: T, index: number) => ObservableInput<T>,
  concurrent?: number,
  scheduler?: SchedulerLike
): OperatorFunction<T, T>;

With this change, the project function will now return the same type as it accepts.

The rationale behind this change is to align the expand function with the traditional understanding of recursion. It feels counter-intuitive for a recursive function to return a different type of data than it accepts as an argument, as this contradicts the fundamental concept of recursion, where an operation is applied repeatedly using the previous result.

This modification will cause a breaking change, however, it seems like the perfect timing to introduce such an alteration with the upcoming major release.

Despite this change, we have retained the flexibility of working with different types for input and output data. This can be achieved by manually specifying a union type generic, which allows for an idiomatic use of expand even with the modified type constraints. Here's an example illustrating this point:

of(1).pipe(expand<string | number>(x => typeof x === 'string' ? of(123) : of('test')));

In this scenario, the type string | number serves as the unified type for input and output, thereby preserving the possibility of having differing types while maintaining the new type constraints for expand.

As part of this change, I've removed the following test case:

it('should infer correctly with a different type as the source', () => {
  const o = of(1, 2, 3).pipe(expand(value => of('foo'))); // $ExpectType Observable<string>
  const p = of(1, 2, 3).pipe(expand(value => ['foo'])); // $ExpectType Observable<string>
  const q = of(1, 2, 3).pipe(expand(value => Promise.resolve('foo'))); // $ExpectType Observable<string>
});

This test case was inherently flawed, as it suggested that the observable type could be inferred to be of a type different from the source type. In reality, the actual return type from expand is a union type of the input and output values (e.g number | string for the first case). This misleading case could have caused confusion among users regarding the expected downstream data type. Therefore, its removal serves to eliminate this potential pitfall and aligns with the actual behavior of the expand operator.

However, I revised a test case to illustrate that union types can still be used with the modified expand function:

it('should support union types', () => {
  const o = of(1).pipe(expand<string | number>(x => typeof x === 'string' ? of(123) : of('test'))); // $ExpectType Observable<string | number>
});

I firmly believe this modification brings several benefits: it makes expand more intuitive, it aligns the function more closely with recursion conceptually, and it enhances type inference to be more reliable and consistent.

[dilame]

i think i'm done!

[kolodny]

@dilame IIRC, those changes were made because of cases where an inner observable return might have a union type like concatMap(() => Observable<string> | Observable<number>) which needs to result in Observable<string | number>. In older versions of TypeScript it couldn't figure that out without what you're currently reading. I'm unsure if that has changed.

The current version of TS seems to work as expected.

@dilame I think I'd prefer seeing something along these lines so as to not break existing code that specifies a generic and still allows the flexibility you're going for (unless I'm misunderstanding what you're trying to accomplish):

export function expand<T, O extends ObservableInput<unknown> = ObservableInput<T>>

[dilame]

unless I'm misunderstanding what you're trying to accomplish

@kolodny, thank you for your input. To clarify, the main aim of this PR is to ensure the expand function signature is accurate and truly reflective of its nature. This is not about adding extra flexibility, but rather aligning the signature with the actual function behavior.

The presence of a second generic in the function was a mistake, as expand, being a recursive function, inherently can't have an output type that differs from its input type. The misconception might be rooted in the function's recursive nature - it doesn't create a new type, but perpetuates its input type.

I understand the concerns about breaking existing code. However, the current incorrect typing could also potentially mislead developers and cause unexpected bugs in the future. Therefore, I believe it's justifiable to correct it in the new major version release.

Also, i don't fully understand what do you mean by

export function expand<T, O extends ObservableInput<unknown> = ObservableInput<T>>

Do you want me to just declare a dummy generic that is not used anywhere?

Regarding the union type inference in inner observables, as you've mentioned, the newer versions of TypeScript have significantly improved and can now handle these situations more accurately.

My another point, that is not directly related to this PR, is to simplify the RxJS operators generics all over the codebase, because right now it seems somewhat overcomplicated FMPOV.

For instance, let's consider the mergeMap declaration:

export function mergeMap<T, O extends ObservableInput<any>>(
  project: (value: T, index: number) => O,
  concurrent: number = Infinity
): OperatorFunction<T, ObservedValueOf<O>>

The generics here have a technical aspect - they work well for internal usage, but look cumbersome when you want to manually specify them in userland:

pipe(mergeMap<number, ObservableInput<string>>(x=> of(x.toString()) ))

I believe it is possible to simplify the signature like this

export function mergeMap<T, O>(
  project: (value: T, index: number) => ObservableInput<O>,
  concurrent: number = Infinity
): OperatorFunction<T, O>

And in userland, it becomes more straightforward:

pipe(mergeMap<number, string>(x=> of(x.toString()) ))

This adjustment enhances readability and maintains the accuracy of the function signatures.

I could help with refactoring, if you like the idea

[kolodny]

I think I may have been mistaken in what I wrote vs what I meant. I created a playground of the current, the suggestion, and the PR: playground

The advantage of what I'm suggesting is that the user doesn't really need to know anything about what the generics mean. They can just define the project functions arguments directly to be what they expect it to be based on the widening of the type. It doesn't take away the generic providing approach but in general it's an anti-pattern to have end users need to spend any mental load on what the generics are doing.

I would love to spend a cycle or two on rethinking generics across the library as long as it can be done in a non-breaking manner (for typical users, I'm not too worried about breaking folks who are already committing war crimes with the type system)

[dilame]

@kolodny I appreciate your feedback, but I'm not sure I fully understand what is your main suggestion – the section under the // Modified comment or under the // PR one?

Regardless, It seems like your primary concern is preserving the second generic for backward compatibility.
However, I would argue about the necessity of this backward compatibility.

In fact, the only breaking change will be applied to users who manually specified expand generics in userland.
For those who just use expand as is, nothing will change, except in cases where their project function return type differs from the input. But it's not a breaking change – it's a bug fix.

Another point is, if we leave the second generic, we open room for type errors. There's nothing stopping a user from specifying it as follows:

pipe(
  expand<string, number>()
)

This implies that the function accepts only strings and returns only numbers, which is logically impossible given the recursive nature of expand. Therefore, I'd say that the elimination of the second generic actually improves type safety, and brings breaking change to an insignificant subset of users, for most of which it will be a bug fix rather than a breaking change.

Anyway, i can make it as RxJS team wish, but i just need some more clarification, because i seem to not understand the requirements.

[dilame]

Here is an example of false-positive compilation success, which will cause runtime type errors.

Parameter x here infers to 4 | 2 while in runtime it will be 1 | 2 | 3 | 4

[kolodny]

The point I was trying to make is that when you can have the user specify a looser argument type over a generic, I'd opt for that. The issue of a user passing in some incorrect generic to the second placeholder doesn't seem to be a large concern for me

EDIT: This would be the modified PR which highlights how we'd prefer users use the operator

[dilame]

@kolodny, i just woke up, reviewed your modification, played with it a little bit, and, first of all, I must admit that you did quite an impressive job. The solution with 2 generics you proposed in a single evening is something I spent the first two weeks striving to achieve. I do admire that.

Maintaining backward compatibility is important, and I appreciate your commitment to not introducing breaking changes. However, I'm a strong advocate for leveraging static types to their maximum potential to prevent any possibility of writing incorrect code.

Your point about reducing the mental load for end users by allowing them to directly define the project function's arguments is valid and certainly adds appeal to your proposal. However, I find it somewhat contradictory when compared to the sentiment of

not worrying about breaking folks who are already committing war crimes with the type system

Aren't users who had manually specified generics for expand in userland exactly those committing such "type war crimes"? So, preserving backward compatibility in this case seems to cater to these undesired practices.

To illustrate this, consider an example of the existed "type war crimes" in current users codebases:

expand<number, Observable<string>>( x => of(`Hello, ${number}`) )

My breaking change mainly affects these types of cases. But aren't those who already have such code in their codebase exactly the ones committing "type war crimes"? Why, then, should we support them?

I'm particularly focused on this aspect because it's where I see a major opportunity to improve. With your modification, even well-meaning users who aren't intending to cause the war crimes with the type system could inadvertently introduce type inconsistencies due to the raised level of abstraction and increased cognitive load.

To illustrate, let's consider a little bit stupid, but simple example, where we don't directly specify generic values, but take it from another generics:

interface Parent<T, U> {
  expand: <I extends T, O extends ObservableInput<U>>(project: (value: I, index: number) => O) => OperatorFunction<I, ObservedValueOf<O>>
}

// Somewhere in the codebase
const instance: Parent<number, string> = getSomeInstance();

instance.expand(x => of(x.toString())) // $ExpectType OperatorFunction<number, string>

In this case, the conscientious user could still make a mistake, and it's exactly this type of situation that the type system should help to prevent.

After three weeks of research, I found an ideally fitting solution as simple as the classic

declare function recursion<T>(input: T): T 

But for RxJS. This signature I've proposed accurately reflects the function's behavior, significantly reducing the chance of user-induced type errors. It brings us closer to the true intention of a type system - accuracy, safety, and ease of use.

When it comes to the backward compatibility issue, I completely understand its importance and the compromises it sometimes necessitates

By its nature, a compromise implies accepting something less desirable for the sake of something beneficial. Here, we're leaving room for runtime bugs for the sake of backward compatibility.

But, doesn't the purpose of new major versions lie in freeing ourselves from such compromises? To keep only the best and leave behind the rest?

With the new major version, we have a perfect opportunity to make this improvement. I modestly ask you to seriously consider this breaking change.

[dilame]

oh, by the way, if declare expand signature as you suggested, we end up with compilation error in expand body:

[kolodny]

I hear the points you're making, however it still feels off to require the user to know about the generic. TypeScript provides smart heuristics when it comes to covariance and contravariance parameters and this is where we'd want to rely on the language itself dealing with this (I argue this is a language primitive while providing generics is a form of escape hatch).

Let's see how this is handled in the wild in other popular libraries. The closest parallel would be the difference between

Promise.resolve<number | undefined>(123).then(value => value /* value has type number | undefined */ )

// vs

Promise.resolve(123).then((value: number | undefined) => value /* value has type number | undefined */ )

We can use sourcegraph to roughly see which one is more common/idiomatic

The generic flavor doesn't have a lot of usage, contrast that with the parameter type declaration way, which is used within the TypeScript, WebDriver, and Angular repos

My suggestion doesn't remove what you had in mind with having the ability to provide generics if you want, it just also allows just typing the parameters which matches the Promise.resolve interface

[dilame]

@kolodny

I greatly appreciate your thoughtful approach to code and your detailed responses, which I believe have helped me find the perfect signature that simultaneously meets all your, undoubtedly correct, requirements, is fully type-safe, and allows the expand function body to compile without errors.

declare function expand<I, O = I>(
  project: (value: I | O, index: number) => ObservableInput<O>,
): OperatorFunction<I, I | O>;

You were absolutely right; two generics are indeed needed here. However, with my new suggestion, it's now possible to reduce the mental load on users by allowing them to extend the input type through directly by specifying the argument type, while not allowing them to shoot themselves in the foot by manually specifying incorrect generics, all in a completely type-safe manner.

of(1 as const).pipe(
  expand((x: 1 | 2 | 3) => of(x)) // Observable<1, 2, 3>
)

In this case, as you rightfully desire, the type is beautifully expanded.

And it's still possible to manually specify straightforward generics in 2 ways

expand<number | string>((x) => of(x)) // Observable<number | string>

expand<number, string>((x) => of(x.toString())) // Observable<number | string>

However, the second generic indicates the project function return type, so this will result with error

expand<number, string>((x) => of(x)) // Type 'Observable<string | number>' is not assignable to type 'ObservableInput<string>'

Which i think is correct, because the output of the project function is not the same as the output of the expand itself. Just perfect.

And the most important – the expand function body implementation compiles with no errors.

[kolodny]

Nice, I like the idea of not having the generic be bound to ObservableInput and only using that for the parameter itself. Good job!

[benlesh]

This should work without the explicit typing. This seems like a regression.

[dilame]

It works without explicit typings, it’s just a test case

[benlesh]

Basically, we have to make sure union types work without requiring explicit generics.

[dilame]

@benlesh thanks for the feedback. The topic you are talking about was already discussed with @kolodny in this pull request. I understand, it's long, but encourage you to read it.

In a few words, in order to make sure it works without explicit generics i implemented another test case, you can find it

it('should infer correctly specifying value argument type', () => {
  const o = of(1).pipe(expand((value: number | string) => of(value.toString()))); // $ExpectType Observable<string | number>
});

[dilame]

Is anybody out there?:)

[dilame]

@benlesh @kolodny @cartant @LcsGa 😃

[benlesh]

Plainly put: The obvious signature that everyone would reach for doesn't work all of the time.

Here is a TypeScript playground showing the proposed signature vs what we currently have

I'll put it inline here for the curious:

import { ObservableInput, ObservedValueOf, OperatorFunction, Observable, of, SchedulerLike } from 'rxjs';

// The new proposed signature
declare function expandNew<I, O = I>(
  project: (value: I | O, index: number) => ObservableInput<O>,
  concurrent?: number,
  scheduler?: SchedulerLike
): OperatorFunction<I, I | O>;

// The old signature
declare function expandOld<T, O extends ObservableInput<unknown>>(
  project: (value: T, index: number) => O,
  concurrent?: number,
  scheduler?: SchedulerLike
): OperatorFunction<T, ObservedValueOf<O>>;


declare const source: Observable<number>

// Observable<unknown>
const resultNew = source.pipe(
  expandNew((value) => value % 2 === 0 ? of(12345) : of('a string'))
                                     /*  ~~~~~~~~~~~~~~~~~~~~~~~~~~
                                        Type 'Observable<number> | Observable<string>' is not assignable to type 'ObservableInput<number>'.
                                          Type 'Observable<string>' is not assignable to type 'ObservableInput<number>'.
                                            Type 'Observable<string>' is not assignable to type 'Observable<number>'.
                                              Type 'string' is not assignable to type 'number'.(2322)
                                        input.tsx(5, 12): The expected type comes from the return type of this signature. */
);

// Observable<string | number>
const resultOld = source.pipe(
  expandOld((value) => value % 2 === 0 ? of(12345) : of('a string'))
)

Thank you for all of your effort here, @dilame. I was hopeful that TypeScript had fixed something and that we were able to move back to the simpler/more obvious solution, but it doesn't look like that will work.

[dilame]

Hi @benlesh ,

I appreciate your feedback, however, I'd like to revisit the example you mentioned, which you consider as valid in the current implementation:

const resultOld = source.pipe(expandOld((value) => 
  // Value inferred as "number" which is dramatically incorrect – it is (string | number)
  value % 2 === 0 ? of(12345) : of('a string')))

It introduces a bug where the inferred type of value (number) incorrectly represent the possible types (string | number). This is really a danger zone. TypeScript should be used to prevent TypeError's, but here we are literally introduce TypeError.

Yes, my proposal requires generics in some rare cases, but it guarantees full type-safety. Incorrect types are a much bigger problem than just requiring users to specify generics in rare corner cases.

Most likely my previous communications might have been overly verbose, potentially diluting the focus on the core issue at hand. With this message, my intention was to crystallize the critical nature of the problem addressed by the pull request.

I respectfully urge a reconsideration of the decision, with a renewed focus on the long-term robustness and developer experience provided by RxJS.

[dilame]

@benlesh i kindly ask you to read my previous message in this thread

[benlesh]

Hi. So, I just manually tested this change again and it doesn't solve the issue much better than what we currently have.

The solution proposed here still requires manual typing to make the example work, see this TypeScript playground

This PR also deletes a few assertions in our type checks that I think are still valid.

[dilame]

Thanks for the answer, @benlesh

Actually, I think this approach resolves the issue better (i would even say, in the only correct way) than the current implementation.

The main difference is that my proposal emits an error in the danger zone, prompting the user to consider their types and manually specify them. So technically, it’s not broken—it just requires some extra steps to make it valid.

The current implementation, on the other hand, silently swallows the type, leading the user to think everything is correct when it actually isn’t, which is a bug from my point of view.

Given these circumstances, I think it makes sense to approve my PR. If you agree, I can reintroduce type check assertions.

[dilame]

I think in order to resolve this PR, we need to reach a consensus on two questions:

1. Do you consider the current implementation to be a bug?
2. Do you consider my proposal, which requires manual type declaration in previously incorrect cases, to be a bug?

If you think 1 is not a bug, ok, makes no sense to merge this PR.
If you think that 2 is a bug, makes no sense to merge this PR.

Otherwise i think my PR makes sense

[benlesh]

While I can see your side of the argument, another problem exists. The problem is the scale of usership of RxJS.

If introduced there would be two new failures all of a sudden in code that was building fine before:

1. People who were blissfully unaware of having the incorrect type for the value inside of the projection function would suddenly be aware. (Good, maybe? They might catch something. but if the code was working before, we just created noise)
2. People who fixed the typings manually by typing the value in the projection function would get an incorrect error that their typings are wrong (Bad! Because now they have to use whatever TypeScript-foo they can muster to figure out what they need to manually type to get things correct)

If I were to release a patch of RxJS with the proposed solution, anyone that didn't have a lock file would see broken builds, and anyone that was developing and ran an install would see their typechecking start failing. It's an unreasonable amount of thrashing to cause to a lot of people for a solution that isn't definitively better.

If the new solution didn't cause the correct manual typing to fail, then it would be a different story. It's number 2 above that's the real issue.

[dilame]

@benlesh i see your point, and seems like i have a solution

This exactly reflects your second point. What do you think about it?