-
Notifications
You must be signed in to change notification settings - Fork 3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(defer): use overload sig for void factory (#4810)
* test(defer): add failing dtslint test * fix(defer): use overload sig for void factory Closes #4804
- Loading branch information
Showing
2 changed files
with
6 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
362d1d4
There was a problem hiding this comment.
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);}})
362d1d4
There was a problem hiding this comment.
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:
And the use of
ObservedValueOf
will see the return type ofdefer
inferred asObservable<number>
.362d1d4
There was a problem hiding this comment.
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
362d1d4
There was a problem hiding this comment.
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.
362d1d4
There was a problem hiding this comment.
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:
362d1d4
There was a problem hiding this comment.
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?
362d1d4
There was a problem hiding this comment.
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.
362d1d4
There was a problem hiding this comment.
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.
362d1d4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a PR: #4835