fix(toPromise): correct toPromise return type

[jchv]

Description:
As described in #4196, the typing of toPromise does not match reality as it may return undefined in the case that no value is ever emitted. We have hit this bug in practice and believe it would be very useful to fix the actual typing.

(This does not implement the default argument as suggested in #4196 - while this seems like a reasonable addition, there are cases today where returning undefined is in fact desired, and it would be nice if the typing was correct for this case.)

Related issue (if exists):

• Typing of toPromise is wrong #4196 - Typing of toPromise is wrong

(cc @sadarby)

[cartant]

LGTM. Do you think you could add some dtslint tests? They would need to go into this file: https://github.com/ReactiveX/rxjs/blob/master/spec-dtslint/Observable-spec.ts

Also, whilst I agree with this change, there will need to be some discussion about it before it's merged. Although it fixes a problem, it is a breaking change. Whether or not it's okay for v7 or will have to wait until v8 will have to be determined.

[jchv]

Thanks, done. As a note, I had to run mkdir "$HOME/.dts" for dtslint to succeed, I am not sure why or if that's intended.

Also, whilst I agree with this change, there will need to be some discussion about it before it's merged. Although it fixes a problem, it is a breaking change. Whether or not it's okay for v7 or will have to wait until v8 will have to be determined.

Understood. There's definitely other approaches with varying pros/cons, but I think this one is pretty straightforward so I hope it or a compatible variant of it ends up working out.

[cartant]

LGTM

[cartant]

As a note, I had to run mkdir "$HOME/.dts" for dtslint to succeed, I am not sure why or if that's intended.

Yeah, that's happened to me, too. dtslint has been an ongoing source of frustration. It's a really useful tool and has helped improve the codebase, but some of the things it does are so annoying.

[benlesh]

I think the real fix for toPromise is that it should reject if no value arrives with a known error, that's obviously a big breaking change. The point is that a Promise is a "promise" that a value will arrive. If it doesn't arrive, the promise is broken, IMO, and we should reject.

I know this has been discussed before, and I'm not saying we should break this for v7, but we should figure out if this is a change we want to make, given that it will make the change I'm suggesting even more breaking when it lands.

I think we should discuss this at the next meeting.

[jchv]

That also SGTM. I think that solution, though incompatible, basically serves the same purpose.

[benlesh]

Approved, but as a BREAKING CHANGE... (from Core Team meeting)