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.

Sign up for GitHub

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

Jump to bottom

fix(startWith): add empty signature #4034

Merged
merged 4 commits into from
Aug 27, 2018
Merged

fix(startWith): add empty signature #4034

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
229f06a
fix(startWith): add empty signature
timdeschryver Aug 18, 2018
4416884
test(dtslint): add startWith tests
timdeschryver Aug 19, 2018
a852868
feat(startWith): allow different types
timdeschryver Aug 21, 2018
3ad2cff
fix(compat): add typings to startWith
timdeschryver Aug 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions spec-dtslint/operators/startWith-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { of, asyncScheduler } from 'rxjs';
import { startWith } from 'rxjs/operators';

it('should infer correctly with one value', () => {
const o = of(1, 2, 3).pipe(startWith(4)); // $ExpectType Observable<number>
});

it('should infer correctly with multiple values', () => {
const o = of(1, 2, 3).pipe(startWith(4, 5, 6)); // $ExpectType Observable<number>
});

it('should infer correctly with no value', () => {
const o = of(1, 2, 3).pipe(startWith()); // $ExpectType Observable<number>
});

it('should infer correctly with a value and a scheduler', () => {
const o = of(1, 2, 3).pipe(startWith(5, asyncScheduler)); // $ExpectType Observable<number>
});

it('should infer correctly with only a scheduler', () => {
const o = of(1, 2, 3).pipe(startWith(asyncScheduler)); // $ExpectType Observable<number>
});

it('should enforce types with one value', () => {
const o = of(1, 2, 3).pipe(startWith('foo')); // $ExpectError
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this should be // $ExpectType Observable<number|string>

});

it('should enforce types with multiple value', () => {
const o = of(1, 2, 3).pipe(startWith(4, 'foo')); // $ExpectError
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, although this might not be possible in this case without the overloads getting out of control.

});
2 changes: 1 addition & 1 deletion spec/operators/startWith-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ describe('startWith operator', () => {
const e1subs = '^ !';
const expected = '-a-|';

expectObservable(e1.pipe(startWith<any>(rxTestScheduler))).toBe(expected);
expectObservable(e1.pipe(startWith(rxTestScheduler))).toBe(expected);
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});

Expand Down
1 change: 1 addition & 0 deletions src/internal/operators/startWith.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { isScheduler } from '../util/isScheduler';
import { MonoTypeOperatorFunction, SchedulerLike } from '../types';

/* tslint:disable:max-line-length */
export function startWith<T>(scheduler?: SchedulerLike): MonoTypeOperatorFunction<T>;
Copy link
Member

Choose a reason for hiding this comment

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

I think we're going to need to differentiate between what's being passed into the startWith and what the source observable is to really get this right. 🤔

export function startWith<T, R>(...args: R[], scheduler?: SchedulerLike): OperatorFunction<T, T|R>;

Copy link
Collaborator

Choose a reason for hiding this comment

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

@benlesh Yeah, you are right. This is very similar to the changes that were made regarding the default parameters in first and last - in 04f5f0b - that effect a union type if a non-T default is specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT this won't be work because it's only possible to define a rest parameter as the last argument in a function.
I think it should be doable if we solve it the same way as in the commit @cartant mentioned.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@timdeschryver You'd need to use a union type as the rest parameter type. Have a look at concat.

Copy link
Collaborator

@cartant cartant Aug 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I just noticed that there is already a rest parameters signature. I think the T in all of the signatures should be replaced with an R - where R defaults to T, as in the referenced commit. And the return type should be OperatorFunction<T, T | R> which will collapse to OperatorFunction<T, T> when R is T.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I agree that's what I have right now.
The only difference is that I have a type for each argument, I'll commit and push so you can take a look and give your opinions on it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I guess if R doesn't have to be T, having one type parameter per parameter makes sense.

export function startWith<T>(v1: T, scheduler?: SchedulerLike): MonoTypeOperatorFunction<T>;
export function startWith<T>(v1: T, v2: T, scheduler?: SchedulerLike): MonoTypeOperatorFunction<T>;
export function startWith<T>(v1: T, v2: T, v3: T, scheduler?: SchedulerLike): MonoTypeOperatorFunction<T>;
Expand Down