Skip to content

Commit

Permalink
fix(defer): use overload sig for void factory (#4810)
Browse files Browse the repository at this point in the history
* test(defer): add failing dtslint test

* fix(defer): use overload sig for void factory

Closes #4804
  • Loading branch information
cartant authored and benlesh committed Jun 4, 2019
1 parent 9d823df commit 362d1d4
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 0 deletions.
4 changes: 4 additions & 0 deletions spec-dtslint/observables/defer-spec.ts
Expand Up @@ -15,3 +15,7 @@ it('should infer correctly with function return promise', () => {
it('should support union type returns', () => {
const a = defer(() => Math.random() > 0.5 ? of(123) : of('abc')); // $ExpectType Observable<string | number>
});

it('should infer correctly with void functions', () => {
const a = defer(() => {}); // $ExpectType Observable<never>
});
2 changes: 2 additions & 0 deletions src/internal/observable/defer.ts
Expand Up @@ -52,6 +52,8 @@ import { empty } from './empty';
* @name defer
* @owner Observable
*/
export function defer<O extends ObservableInput<any>>(observableFactory: () => O): Observable<ObservedValueOf<O>>;
export function defer(observableFactory: () => void): Observable<never>;
export function defer<O extends ObservableInput<any>>(observableFactory: () => O | void): Observable<ObservedValueOf<O>> {
return new Observable<ObservedValueOf<O>>(subscriber => {
let input: O | void;
Expand Down

9 comments on commit 362d1d4

@mhverbakel
Copy link

Choose a reason for hiding this comment

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

Note that this is breaking if you had the function defer(() => { if (condition) { return of(12);}})

@cartant
Copy link
Collaborator Author

@cartant cartant commented on 362d1d4 Jun 5, 2019

Choose a reason for hiding this comment

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

I don't think so. The return type of that arrow function will be:

Observable<number> | undefined

And the use of ObservedValueOf will see the return type of defer inferred as Observable<number>.

@mhverbakel
Copy link

@mhverbakel mhverbakel commented on 362d1d4 Jun 5, 2019

Choose a reason for hiding this comment

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

Ah yes, you are right. The only problem is that it now allows to return faulty values (non-observable input) which will then be inferred as being 'void' (see https://stackblitz.com/edit/rxjs-2cz1yr)

Also see: https://github.com/Microsoft/TypeScript/wiki/FAQ#why-are-functions-returning-non-void-assignable-to-function-returning-void

@cartant
Copy link
Collaborator Author

@cartant cartant commented on 362d1d4 Jun 5, 2019

Choose a reason for hiding this comment

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

Can you create an issue for this, so that this can be discussed? It seems that we have to choose between two shitty options.

@cartant
Copy link
Collaborator Author

@cartant cartant commented on 362d1d4 Jun 5, 2019

Choose a reason for hiding this comment

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

This seems okay, to me:

declare function _defer<R extends ObservableInput<any> | void>(factory: () => R): Observable<ObservedValueOf<R>>;
const fromVoid = _defer(() => {});
const fromInput = _defer(() => of(1));
const fromDepends = _defer(() => { if ({} === {}) { return of(1); } });
const fromInvalid = _defer(() => 12);

@mhverbakel
Copy link

Choose a reason for hiding this comment

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

Nice! That does seem to work. Can you create the issue for this?

@cartant
Copy link
Collaborator Author

@cartant cartant commented on 362d1d4 Jun 5, 2019

Choose a reason for hiding this comment

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

If you want to create a PR with the changes, that's fine by me. Usually, I'd ask for an issue to be created, but I'd be happy to see a PR with a description of the problem fixed and a link to this discussion.

@cartant
Copy link
Collaborator Author

@cartant cartant commented on 362d1d4 Jun 5, 2019

Choose a reason for hiding this comment

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

And if you could add a dtslint test to show that an error is effected for factories that return invalid values - like a number - that would be great. If you don't have time, I can create a PR later.

@cartant
Copy link
Collaborator Author

@cartant cartant commented on 362d1d4 Jun 5, 2019

Choose a reason for hiding this comment

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

Created a PR: #4835

Please sign in to comment.