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

refactor(merge): support N-args with scheduler #5859

Merged
merged 3 commits into from Oct 30, 2020

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Oct 27, 2020

Description:

This PR refactors the signatures of the merge and mergeWith operators to use the variadic tuple technique that was employed in the concat PR - see #5857.

Hopefully, it should serve as a template for what needs to be done elsewhere whenever there are trailing arguments. It seems that when there are, the correct behaviour of the inference is extremely sensitive to the ordering of the signatures.

IMO, we should also be moving to using unknown[] as the type parameter constraint - along with ObservableInputTuple - instead of using ObservableInput in the constraint and ObservedValueOf in the return type. The former is, IMO, idiomatic TypeScript and the latter is not.

Related issue (if exists): None

Initial description (when it was still broken):

This PR attempts to reduce the number of signatures for the merge operator using the approach used for the static concat function. It seems that TypeScript does weird things and it's necessary to have a bunch of sigs that reflect no inputs being passed and another that reflects a single input being passed. Specifically, the signature that requires the single-input signature to be present is this one:

export function merge<T, A extends unknown[]>(
  ...args: [...ObservableInputTuple<A>, number, SchedulerLike]
): OperatorFunction<T, T | A[number]>;

If the single input signature is not present and the above signature is not commented out, the inference for a single argument will fail:

const result = of(1).pipe(merge(of('two')); // Infers Observable<unknown> instead of Observable<number | string>

IDK what is going on, but I'm done with this for now. I've opened this PR as it goes some way towards showing that this almost works and that we might need some seemingly unnecessary signatures just to stop TypeScript from going down the wrong path. I would have hoped that the variadic input tuples would work with zero inputs - like they do with concat. 🤷‍♂️🤬

@cartant
Copy link
Collaborator Author

cartant commented Oct 27, 2020

@benlesh @kolodny So much for being done with this, I wanted to create a minimal repro, so here is a playground that shows when/where it breaks:

type Thing<T> = {
  value: T;
}

declare const t1: Thing<1>;
declare const t2: Thing<2>;

type Things<T> = {
  [K in keyof T]: Thing<T[K]>;
};

type Scheduler = {
  now(): number;
};
declare const scheduler: Scheduler;

// declare function g<T extends readonly unknown[]>(
//   ...thingsAndNumberAndScheduler: [...Things<T>, number, Scheduler]
// ): [...T, number, string];

declare function g<T extends readonly unknown[]>(
  ...thingsAndNumber: [...Things<T>, number]
): [...T, number];

declare function g<T extends readonly unknown[]>(
  ...things: [...Things<T>]
): [...T];

const l = g(t1);        // [1], but with Scheduler sig: unknown[]
const m = g(t1, 1);     // [1, number]
const n = g(t1, t2, 1); // [1, 2, number]
const o = g(1);         // [number], but with Scheduler sig: unknown[]

As soon as the sig with the Scheduler is uncommented, the inference for the first and last statements breaks.

@cartant
Copy link
Collaborator Author

cartant commented Oct 27, 2020

Success. If the order of the signatures ☝ is reversed, it's all good, AFAICT. 🎉

@cartant cartant changed the title WIP refactor(merge): support N-args with scheduler refactor(merge): support N-args with scheduler Oct 27, 2020
@cartant cartant marked this pull request as ready for review October 27, 2020 07:36
@benlesh
Copy link
Member

benlesh commented Oct 27, 2020

Is it possible to get it to error if at least one observable input isn't supplied? That was the problem we hit with my PR for the merge creation function. [ObservableInput<unknown>, ...ObservableInput<unknown>[]] didn't really work.

const r = of('test').pipe(mergeWith(1, scheduler)); // $ExpectError

@cartant
Copy link
Collaborator Author

cartant commented Oct 27, 2020

@benlesh

Is it possible to get it to error if at least one observable input isn't supplied?

I suspect there would be, but I don't think that's something that we should do. For two reasons:

  1. It's a lie. The implementation handles the case of no observable sources just fine.
  2. Requiring at least one observable source will break callers who want to spread an array - not a tuple - into the arguments. See this playground.

I interpreted your change that introduced a minimum of one source as a workaround for the variadic tuple problems and considered it to be a necessary evil.

FWIW, the inferred type - if there are no sources - should be Observable<never>, so it will still effect type issues if the caller tries to use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants