Skip to content

refactor(merge): Create better typings#5844

Closed
benlesh wants to merge 4 commits intoReactiveX:masterfrom
benlesh:refactor/merge-create-better-typings
Closed

refactor(merge): Create better typings#5844
benlesh wants to merge 4 commits intoReactiveX:masterfrom
benlesh:refactor/merge-create-better-typings

Conversation

@benlesh
Copy link
Copy Markdown
Member

@benlesh benlesh commented Oct 21, 2020

  • Now refers n-arguments in all cases, including arguments with tailing concurrency and Scheduler
  • Adds dtslint tests

Please note the "unhappy path" in the dtslint tests. It's not ideal, but everything is much better than it was before. I just struggled with how to ensure merge(a$, 23, b$) didn't match the signature for (pseudo-code here:) merge(...args: ObservableInput<T>, concurrency: number)

- Now refers n-arguments in all cases, including arguments with tailing concurrency and Scheduler
- Adds dtslint tests
@benlesh benlesh requested a review from cartant October 21, 2020 18:30
Comment thread src/internal/types.ts Outdated
/**
* Used to help deal with rest arguments of many observable inputs that are tailed by schedulers, etc.
*/
export type OneOrMoreUnknownObservableInputs = readonly [ObservableInput<unknown>, ...unknown[]];
Copy link
Copy Markdown
Collaborator

@cartant cartant Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: AFAICT, this makes things worse. The ...unknown[] will match anything, so these don't effect errors:

const o5 = merge(a$, 1, 2, 3, 4, 5); // $ExpectError
const o6 = merge(a$, 1, 2, 3, 4, 5, asapScheduler); // $ExpectError

Note that the first of these is essentially untyped normal usage of merge.

And, changing ...unknown[] to ...ObservbleInput<unknown>[] just makes things worse still - breaking things that ought to work. I'll have another look at this after I've had a coffee, but ATM ... 🤷‍♂️

Copy link
Copy Markdown
Collaborator

@cartant cartant Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a decent, actionable suggestion for this problem. I still think it's a TS bug. If it weren't for the concurrency parameter, we could keep the ugly overloads for the deprecated scheduler sigs and use variadic tuples for the non-deprecated sigs. But we have the trailing concurrency, so ... 🤷‍♂️

Copy link
Copy Markdown
Member Author

@benlesh benlesh Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh... you're right. I tried ...ObservableInput<unknown>[] first, but it didn't work and I couldn't see why.

@DanielRosenwasser, do you have any suggestions?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I've fixed your example, @cartant, here

declare function f<S extends readonly [string, ...string[]]>(
  ...stringsAndNumber: readonly [...S, number]
): [...S, number];

const a = f("a", 1);
const b = f("a", "b", 1);
const c = f(1);
const d = f(1, 2); // works fine now (with an error)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... but it still doesn't work in our case, once there's a generic inside of that declaration... farts.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here

class Blah<T> {
  constructor(public readonly blah: T) {}
}

declare function f<S extends readonly [Blah<unknown>, ...Blah<unknown>[]]>(
  ...stringsAndNumber: readonly [...S, number]
): [...S, number];


const blah1 = new Blah('a');
const blah2 = new Blah([]);

const a = f(blah1, 1);
const b = f(blah1, blah2, 1);
const c = f(1);
const d = f(1, 2);
const e = f(blah1, blah2, 1, 2, 3); // wtf?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've pushed an update with a commit that should work (I think), but doesn't. Marking this as blocked until we hear from the TS team about whether they're going to support that or not. It looks like a simple bug, but maybe there's some other approach they want people to take.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TypeScript playground link that I added above was for what is, IMO, the fundamental bug.

The solution you have above will work if the tuple of type parameters to Blah is extracted and mapped, like this:

class Blah<T> {
  constructor(public readonly blah: T) {}
}

type Blahs<T extends readonly unknown[]> = {
  [K in keyof T]: Blah<T[K]>;
};

declare function f<S extends readonly [unknown, ...unknown[]]>(
  ...stringsAndNumber: readonly [...Blahs<S>, number]
): [...Blahs<S>, number];

See this playground snippet

Whether that will work in the codebase, IDK. I'm pretty fed up with TS, TBH.

Comment on lines +213 to +216
export declare function merge<O extends ObservableInput<unknown>>(sources: O[], concurrent: number, scheduler: SchedulerLike): Observable<ObservedValueOf<O>>;
export declare function merge<O extends ObservableInput<unknown>>(sources: O[], scheduler: SchedulerLike): Observable<ObservedValueOf<O>>;
export declare function merge<O extends ObservableInput<unknown>>(sources: O[], concurrent: number): Observable<ObservedValueOf<O>>;
export declare function merge<O extends ObservableInput<unknown>>(sources: O[]): Observable<ObservedValueOf<O>>;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: We need a consistent approach to this. Elsewhere, ObservableInputTuple is used instead of ObservedValueOf.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are meant to solve different issues? One is to get A | B | C out of ObservableInput<A | B | C>, and the other is used to get [A, B, C] out of [ObservableInput<A>, ObservableInput<B>, ObservableInput<C>]. Unless I'm mistaken?

@benlesh
Copy link
Copy Markdown
Member Author

benlesh commented Oct 23, 2020

Blocked on microsoft/TypeScript#39595

@devanshj
Copy link
Copy Markdown

devanshj commented Oct 27, 2020

Twitter brought me here. If I understand correctly y'all basically want to achieve something like f(...a: [string, ...string[], number]), here's a way you can do it. Notice the errors are pretty friendly as you would want them to be. If you like it, I can go deep and explain how it works or answer questions if y'all want.

Playground

declare function f<A extends Cast<Identity<StringStringsNumber<A>>, any[]>>(...a: A): A;

f('blah1', 1);
f('blah1', 'blah2', 1);
f(1);
f(1, 2);
f('blah1', 'blah2', 1, 2, 3);
f();
f('a');
f('a', 'a');
f(1, 'a');

type StringStringsNumber<A extends any[]> =
  HeadBodyTail<A, string, string, number>;

type HeadBodyTail<A extends any[], H, B, T> =
  First<A> extends H
    ? Last<A> extends T
        ? Pop<Shift<A>> extends B
            ? A
            : [First<A>, ...Elements<Pop<Shift<A>>, B>, Last<A>]
        : Last<A> extends H 
            ? Push<A, T>
            : Push<Pop<A>, T>
    : First<A> extends undefined
        ? [H, T]
        : Unshift<Shift<A>, H>;

type Elements<A extends any[], B> =
  Lazy<A, Cast<{ [K in keyof A]: B }, any[]>>

type First<A extends any[]> = Lazy<A, A extends [] ? undefined : A extends [infer H, ...infer _] ? H : never>;
type Last<A extends any[]> = Lazy<A, A extends [] ? undefined : A extends [...infer _, infer T] ? T : never>;
type Shift<A extends any[]> = Lazy<A, A extends [] ? [] : A extends [infer _, ...infer S] ? S : never>;
type Unshift<A extends any[], X> = Lazy<A, [X, ...A]>;
type Pop<A extends any[]> = Lazy<A, A extends [] ? [] : A extends [...infer P, infer _] ? P : never>;
type Push<A extends any[], X> = Lazy<A, [...A, X]>;
type Cast<T, U> = T extends U ? T : U;
type Lazy<A extends any[], T> = any[] extends A ? any[] : T;
type Identity<T> = { [K in keyof T]: T[K] }

Not to mention no pressure to go ahead with this, I can understand it might come across as "hacky" or whatever (altho it's not imo).

Also please feel free to ping me henceforth if you want some help with some really tricky TypeScript problem, I'll try my best! :) Not sure if y'all can tell but I'm also the guy behind this :P

@benlesh
Copy link
Copy Markdown
Member Author

benlesh commented Oct 30, 2020

Closed in favor of #5859

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants