fix(startWith): add empty signature #4034

timdeschryver · 2018-08-18T11:49:21Z

Description:
This PR adds an overload to startWith that only takes a scheduler.

Related issue (if exists):
Closes #3947

coveralls · 2018-08-18T12:02:46Z

Coverage remained the same at 96.783% when pulling 3ad2cff on timdeschryver:pr/empty-startwith into 040ec2f on ReactiveX:master.

cartant · 2018-08-19T01:20:47Z

Hey, Tim. Thanks for the PR.

One of the things I'd like to do - now that we have the dtslint infrastructure in place - is to ensure that changes to the typings are accompanied by dtslint tests, so that we can build out the testing of the package's typings.

Would you be able to add a few dtslint tests? Basically, we'd want one to ensure that an observable of the expected type is effected. And another to ensure that an error is effected if the value passed to startWith is not compatible with the source observable's value.

For an example, have a look at spec-dtslint/operators/concatAll-spec.ts.

And to run the tests, use this command:

npm run dtslint

timdeschryver · 2018-08-19T07:56:43Z

I totally missed those, thanks for pointing them out Nicholas 😅 .
You can expect a commit later today/tomorrow.

timdeschryver · 2018-08-19T10:05:33Z

Something like this this, right?
Should I add a test for each type definition?

cartant · 2018-08-19T10:10:50Z

Yeah, that's great.

If you want, you can add a test for each signature, but as all of the value types are of type T, I don't think it's that big a deal. pipe is different, as the parameters chain together.

If you want to add another, maybe you could add one to show that a scheduler can be passed as the last parameter?

benlesh · 2018-08-20T20:35:47Z

spec-dtslint/operators/startWith-spec.ts

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


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

benlesh · 2018-08-20T20:35:59Z

spec-dtslint/operators/startWith-spec.ts

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


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

benlesh · 2018-08-20T20:38:34Z

src/internal/operators/startWith.ts

@@ -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>;


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>;

@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.

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.

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

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.

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.

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

cartant · 2018-08-21T07:08:58Z

src/internal/operators/startWith.ts

@@ -49,7 +49,7 @@ export function startWith<T>(...array: Array<T | SchedulerLike>): MonoTypeOperat
 * @method startWith
 * @owner Observable
 */
-export function startWith<T>(...array: Array<T | SchedulerLike>): MonoTypeOperatorFunction<T> {
+export function startWith<T, D>(...array: Array<T | SchedulerLike>): OperatorFunction<T, T | D> {


I think the rest parameter signature should be made the same as the implementation signature. And I think both should be:

export function startWith<T, D = T>(...array: Array<D | SchedulerLike>): OperatorFunction<T, T | D>

Yea I'll add it, but I didn't had the time to adjust and test it. Currently a bit busy, but I'll add it soon.

timdeschryver · 2018-08-21T16:38:27Z

compat/operator/startWith.ts

@@ -26,5 +26,5 @@ export function startWith<T>(this: Observable<T>, ...array: Array<T | SchedulerL
 * @owner Observable
 */
 export function startWith<T>(this: Observable<T>, ...array: Array<T | SchedulerLike>): Observable<T> {
-  return higherOrder(...array)(this);
+  return higherOrder<T, T>(...array)(this);


Is this OK 😅 ?

I seem to always forget the compat signatures. They should be updated, too.

I need to make myself a checklist of things to do upon making a typings change.

Done, thanks for your help!

cartant

LGTM

felixfbecker · 2018-08-23T10:55:43Z

I love that this finally allows starting with a different type than T. I often need to write code like

values.pipe(
  switchMap(value =>
    fetchSuggestions(value).pipe(
      startWith('LOADING')
      catchError(err => [err])
    )
  )
)

Which should be Observable<'LOADING' | Suggestions | Error>, but it was not possible to do before with startWith (only with concat()).

fix(startWith): add empty signature

229f06a

timdeschryver force-pushed the pr/empty-startwith branch from a9dee5e to 4e73844 Compare August 19, 2018 11:36

test(dtslint): add startWith tests

4416884

timdeschryver force-pushed the pr/empty-startwith branch from 4e73844 to 4416884 Compare August 19, 2018 12:00

cartant approved these changes Aug 20, 2018

View reviewed changes

benlesh requested changes Aug 20, 2018

View reviewed changes

cartant reviewed Aug 21, 2018

View reviewed changes

feat(startWith): allow different types

a852868

timdeschryver force-pushed the pr/empty-startwith branch from 2a8224a to a852868 Compare August 21, 2018 16:23

timdeschryver commented Aug 21, 2018

View reviewed changes

fix(compat): add typings to startWith

3ad2cff

timdeschryver force-pushed the pr/empty-startwith branch from 6c727cd to 3ad2cff Compare August 22, 2018 07:20

cartant approved these changes Aug 22, 2018

View reviewed changes

benlesh approved these changes Aug 27, 2018

View reviewed changes

benlesh merged commit b7866a0 into ReactiveX:master Aug 27, 2018

timdeschryver deleted the pr/empty-startwith branch August 27, 2018 17:59

This was referenced Sep 24, 2018

StartWith operator should allow widening the type of the Observable #4181

Closed

endWith should widen the type if necessary #4183

Closed

lock bot locked as resolved and limited conversation to collaborators Sep 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(startWith): add empty signature #4034

fix(startWith): add empty signature #4034

timdeschryver commented Aug 18, 2018

coveralls commented Aug 18, 2018 •

edited

Loading

cartant commented Aug 19, 2018

timdeschryver commented Aug 19, 2018

timdeschryver commented Aug 19, 2018

cartant commented Aug 19, 2018

benlesh Aug 20, 2018

benlesh Aug 20, 2018

benlesh Aug 20, 2018

cartant Aug 20, 2018

timdeschryver Aug 21, 2018

cartant Aug 21, 2018

cartant Aug 21, 2018 •

edited

Loading

timdeschryver Aug 21, 2018

cartant Aug 21, 2018

cartant Aug 21, 2018

timdeschryver Aug 21, 2018

timdeschryver Aug 21, 2018

cartant Aug 21, 2018

timdeschryver Aug 22, 2018

cartant left a comment

felixfbecker commented Aug 23, 2018

fix(startWith): add empty signature #4034

fix(startWith): add empty signature #4034

Conversation

timdeschryver commented Aug 18, 2018

coveralls commented Aug 18, 2018 • edited Loading

cartant commented Aug 19, 2018

timdeschryver commented Aug 19, 2018

timdeschryver commented Aug 19, 2018

cartant commented Aug 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartant Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartant left a comment

Choose a reason for hiding this comment

felixfbecker commented Aug 23, 2018

coveralls commented Aug 18, 2018 •

edited

Loading

cartant Aug 21, 2018 •

edited

Loading