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

pipe does not properly support rest parameters in some cases. #4177

Open
benlesh opened this issue Sep 24, 2018 · 10 comments
Open

pipe does not properly support rest parameters in some cases. #4177

benlesh opened this issue Sep 24, 2018 · 10 comments
Assignees
Labels
feature PRs and issues for features

Comments

@benlesh
Copy link
Member

benlesh commented Sep 24, 2018

I encountered this while updating Angular CLI.

In particular, the update to the base.ts file in there, where I'm adding as any was necessary because the spreading of an array of operators into pipe was not working.

EDIT: It seems that we can't support rest params without breaking inference enforcement from operator to operator in the pipe. So here's a proposal:

Proposed Feature

Add an overload of pipe that accepts OperatorFunction<any, any>[] and returns Observable<R>:

pipe<R>(operations: OperationFunction<any, any>[]): Observable<R>;

// usage

const operations = [map(x => x + x), filter(x => x < 100), mergeMap(n => timer(n))];

of(42).pipe(operations)
@benlesh benlesh self-assigned this Sep 24, 2018
@benlesh benlesh added the bug Confirmed bug label Sep 24, 2018
@kwonoj
Copy link
Member

kwonoj commented Sep 24, 2018

My memory's not clear, but didn't we replace those signature into else for some reason? 872b0ec#diff-eeb9d77f742ad0df7e932697062fc44e /cc @cartant

@cartant
Copy link
Collaborator

cartant commented Sep 24, 2018

See #3841 - adding this signature essentially removes type checking. #3945 was the fix the problems that signature introduced.

@benlesh
Copy link
Member Author

benlesh commented Sep 24, 2018

@kwonoj ... I think the issue is that TypeScript is not honoring line 321 there. It needs to be added to the signature.

@cartant
Copy link
Collaborator

cartant commented Sep 24, 2018

Regarding spreading, see #3989

@benlesh
Copy link
Member Author

benlesh commented Sep 24, 2018

Yup, I see the issue now. Adding a proposed change to the OP.

@benlesh benlesh added feature PRs and issues for features and removed bug Confirmed bug labels Sep 24, 2018
benlesh added a commit to benlesh/rxjs that referenced this issue Sep 24, 2018
@jgbpercy
Copy link

jgbpercy commented Sep 25, 2018

I'm probably missing something obvious, but over the weekend I had a play around with making the args to all the Observable pipe overloads optional, and as far as I could see that had the effect of restoring pipe spread arg typing that is at least as good as it was pre v6.3 (and not causing a regression on #3841 )

All the dtslint tests seemed to pass, and this wouldn't require special-casing an array argument to pipe, so for eg I believe that Angular ...sinks wouldn't need a change. If this does break typing in some way I'm not seeing, I'd be up for adding a PR for dtslint tests that cover this?

So it would look like

  pipe(): Observable<T>;
  pipe<A>(op1?: OperatorFunction<T, A>): Observable<A>;
  pipe<A, B>(op1?: OperatorFunction<T, A>, op2?: OperatorFunction<A, B>): Observable<B>;
  pipe<A, B, C>(op1?: OperatorFunction<T, A>, op2?: OperatorFunction<A, B>, op3?: OperatorFunction<B, C>): Observable<C>;
  pipe<A, B, C, D>(op1?: OperatorFunction<T, A>, op2?: OperatorFunction<A, B>, op3?: OperatorFunction<B, C>, op4?: OperatorFunction<C, D>): Observable<D>;
  pipe<A, B, C, D, E>(op1?: OperatorFunction<T, A>, op2?: OperatorFunction<A, B>, op3?: OperatorFunction<B, C>, op4?: OperatorFunction<C, D>, op5?: OperatorFunction<D, E>): Observable<E>;
// and so on

I didn't have time to turn this into a PR, and I didn't really test properly under TS 2.8 (only 3.0), but I got as far as making these notes:

import { of, Observable, OperatorFunction } from 'rxjs';
import { mapTo, map, switchMap, switchAll } from 'rxjs/operators';

const basic = of(0);

// Works same either way (type breaks down past 9 args, as expected)
const nonSpread = basic.pipe(
  mapTo('1'),
  mapTo(2),
  mapTo('3'),
  mapTo(4),
  mapTo('5'),
  mapTo(6),
  mapTo('7'),
  mapTo(8),
  mapTo('9'),
  // mapTo(10), // if uncommented, type of nonSpread becomes Observable<{}> as expected
  // mapTo('11'),
  // mapTo(12),
);

// Not allowed currently, allowed with optional args, and types correctly.
const maps = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12].map(x => map<number, number>(y => y * x));
const pipedByMaps = basic.pipe(...maps);

// Not allowed currently, allowed with optional args, and types as Observable<string | number>,
// which is about as good as you can expect
const maps2 = [1, '2', 3, '4', 5, '6', 7, '8', 9, '10', 11, '12'].map(x => map(y => x));
const pipedByMaps2 = basic.pipe(...maps2);

// In the general case, when spreading an array with various types of OperatorFunctions, the
// OperatorFunction type params can be the most specific common type, i.e. the best possible
// union type, that encompasses all input and output types.
// Obviously, this is less useful than the precise typing of pipe with less than 10 non-spread args.
interface WithId { id: number; }
type BestUnion = string | number | WithId;
const variousOperatorsArray: OperatorFunction<BestUnion, BestUnion>[] = [
  mapTo('1'),
  mapTo(2),
  mapTo({ id: 3 })
];
const pipedByVariousArray = basic.pipe(...variousOperatorsArray);

// If the "array" is typed as a tuple, however, then the correct type will result (under TS 3.x)
const variousOperatorsTuple: [
  OperatorFunction<number, string>,
  OperatorFunction<string, number>,
  OperatorFunction<number, WithId>
] = [mapTo('1'), mapTo(2), mapTo({ id: 3 })];
const pipedByVariousTuple = basic.pipe(...variousOperatorsTuple);

// This is allowed with or without optional args with non-strict TS. Believe it is currently an
// error with strictNullChecks TS, and would not be with optional args, but it's not a mistake
// that seems worth worrying too much about.
const pipedByUndefined = basic.pipe(
  undefined,
);

// While the above is an unlikely mistake, the following is a semi-plausible mistake that would
// not error under strictNullChecks
// But it's not currently possible at all without optional args, so you don't really lose much
const arrayThatCameFromSomewhere = [map(x => x.toString()), switchMap(x => of(x), undefined)];
const pipedBySpreadWithUndefined = basic.pipe(...arrayThatCameFromSomewhere);

// From https://github.com/ReactiveX/rxjs/issues/3841
// Still has hoped-for error with or without optional args
const input: Observable<number> = of(1, 2, 3);
const output = input.pipe(switchAll()); // Has expected error: Type 'number' is not assignable to type 'ObservableInput<{}>'.
output.subscribe(value => console.log(value));

@cartant
Copy link
Collaborator

cartant commented Sep 25, 2018

IMO, adding an array overload signature would be the preferred solution. It's simple and easy to understand and will effect errors for undefined values, etc.

And TypeScript 3.0 supports using the spread syntax in a fully type-safe manner. So eventually this will be a non-issue.

@cartant
Copy link
Collaborator

cartant commented Sep 25, 2018

Also, I'm not entirely sure why making them optional should effect such different behaviour. It's interesting, but I'd have some concerns that the behaviour could change. Especially, if the reason for it isn't easily explained.

@qm3ster
Copy link

qm3ster commented Apr 8, 2020

Maybe

X.pipe(
  ...some_adapters,
  an_adapter(),
  ...some_more_adapters,
)

should be currently solved by

X.pipe(
  pipeFromArray(some_adapters),
  an_adapter(),
  pipeFromArray(some_more_adapters),
)

?
Would the performance be atrocious?

sko-kr pushed a commit to sko-kr/corona-autochecker that referenced this issue May 31, 2020
* type reason: pipe operator with more than 9 operations lose type.
(ReactiveX/rxjs#4177)
* coronaCheckEpic receives POJO, ans returns observable hence is a callback passed to operator, rather than an operator itself.
@griest024
Copy link

Bumping this. I would at least like an overload that supports an array of single type operators:

pipe<T extends UnaryFunction<any, any>>(...fns: Array<T>): T

and the same for Observable#pipe.

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

Successfully merging a pull request may close this issue.

6 participants