Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pairs observable static function does not infers correctly #4468

Closed
dkosasih opened this issue Jan 14, 2019 · 5 comments
Closed

pairs observable static function does not infers correctly #4468

dkosasih opened this issue Jan 14, 2019 · 5 comments
Labels
bug Confirmed bug TS Issues and PRs related purely to TypeScript issues

Comments

@dkosasih
Copy link
Contributor

Bug Report

Current Behavior
pairs observable is incorrectly inferred. With the example below the type infers as Observable<[string, {}]>

var obj = {
  foo: 1,
  bar: 2,
  baz: 3
};

var source = pairs(obj);

Expected behavior
It should infer `Observable<[string, number]>

Environment

  • RxJS version: Latest (6.3.3)
@jgbpercy
Copy link

Had a bit of a play with this over my lunch break... It would be possible to add an overload to pairs with this signature:

export function pairs<T>(obj: T, scheduler?: SchedulerLike): Observable<[string, T[keyof T]]>;

This issue with this is that T[keyof T] gives you a union type over all types of properties of T, not just the enumerable, own properties that pairs actually returns. Of course, the more things in a union type the less you can do with it, so I think from a soundness point of view it's ok, but if you try passing an array into that signature you don't exactly get a friendly type.

I don't think there's a way for TS to do something similar to keyof T but only return keys of enumerable/own properties - please let me know if I'm wrong!

The following two overloads cover arrays and "plain" objects (like the one in your example):

export function pairs<T>(obj: { [key: string]: T }, scheduler?: SchedulerLike): Observable<[string, T]>;
export function pairs<T>(obj: T[], scheduler?: SchedulerLike): Observable<[string, T]>;

Maybe these on their own would be fine? But the signature with [key: string]: T falls over as soon as you do something like:

interface Base {
  thing: number;
}

interface Child extends Base {
  otherThing: string[];
}

const child: Child = {
  thing: 1,
  otherThing: ['yes', 'yeah']
}

const source = pairs(child); // Types to Observable<[string, {}]>

// Even though the identical-at-runtime
const otherSource = pairs({
  thing: 1,
  otherThing: ['yes', 'yeah'],
});
// Types to Observable<[string, number | string[]]> correctly

(I don't totally understand the reason why, and whether this is specifically to do with inheritance or not?)

So maybe the overloads could be this (the two simple ones first, followed by the more general one):

export function pairs<T>(obj: { [key: string]: T }, scheduler?: SchedulerLike): Observable<[string, T]>;
export function pairs<T>(obj: T[], scheduler?: SchedulerLike): Observable<[string, T]>;
export function pairs<T>(obj: T, scheduler?: SchedulerLike): Observable<[string, T[keyof T]]>;

That way you only fall back to the T[keyof T] overload if you're not passing an array or plain object.

I don't know what the standard is on typing to unions that are less specific than they "should be"? I'm not hugely read up on type theory, but my understanding is that the {} type you get back without the third overload is basically the same as the union of all types, so typing to some more specific union, that still includes all the correct possible types, and some extras, is still sound and at least in some sense better :)

Happy to put in a PR with d.ts tests if people think any/all of the above overloads are an improvement?

@jgbpercy
Copy link

Oh and @dkosasih or anyone else that comes across this, right now it looks like the current signature for pairs has no possibility for inference based on arguments, so in 6.3.3 you always need to specify the type param (in your case pairs<number>(obj))

@dkosasih
Copy link
Contributor Author

My understanding is that it should infer without explicitly define the type. It should be able to just infer.
About the overloads, it looks good to me. But, maybe @benlesh or @cartant would have better perspective on this.

@benlesh benlesh added bug Confirmed bug TS Issues and PRs related purely to TypeScript issues labels Jan 15, 2019
@BioPhoton
Copy link
Contributor

@cartant Is this now with Typescript 3+ possible?
If yes let's comment the next steps. If no let's close this issue

@cartant
Copy link
Collaborator

cartant commented Mar 14, 2021

Closing this because the types in v7 infer correctly:

var obj = {
  foo: 1,
  bar: 2,
  baz: 3
};
var source = pairs(obj); // Observable<["foo" | "bar" | "baz", number]>

And because pairs is now deprecated:

/**
* @deprecated To be removed in version 8. Use `from(Object.entries(obj))` instead.
*/
export function pairs<O extends Record<string, unknown>>(obj: O, scheduler?: SchedulerLike): Observable<[keyof O, O[keyof O]]>;

@cartant cartant closed this as completed Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

No branches or pull requests

5 participants