toPromise should reject if no value is emitted.

[benlesh]

Current Behavior

toPromise will resolve with undefined if the source observable simply completes without emitting a value.

EMPTY.toPromise().then(x => console.log(x)); // logs "undefined"

The problem

Promises are a guarantee to a future value. With toPromise() we should be saying "this is the last value we've gotten prior to completion". But what we're actually saying is "this might be the last value we've gotten prior to completion". It's semantically unsound, IMO.

If the developer wants a guaranteed value from a source, and that value might be undefined, there's currently no way for that developer to discern whether or not the undefined that toPromise resolved with was the undefined from the source, or just happenstance from completion without a value.

const promiseTheresAValue1 = of(1, 2, 3, undefined).toPromise();

const promiseTheresAValue2 = EMPTY.toPromise(); // Oops, there wasn't actually a value

Proposal

Have toPromise reject with an EmptyError if no value is emitted prior to completion of the source.

EMPTY.toPromise()
  .then(() => console.log('not hit'))
  .catch(err => console.log(err instanceof EmptyError)); // logs "true"

Optionally we could introduce a last() or lastValue() method to Observable and deprecate toPromise to reduce confusion while migrating to this change, while also being able ot introduce this better semantic sooner.

Related: #4993

[benlesh]

Related #5072

[cartant]

we could introduce a last() or lastValue() method to Observable

I'd be wary of introducing anything else to Observable whilst there any chance the TC39 proposal might be resurrected. IMO, toPromise should be deprecated for eventual removal from the Observable prototype and a static function should be exported instead.

[benlesh]

@cartant, that's an interesting proposal. As far as the observable proposal goes, it might still be a moonshot, so I think RxJS, as a library, should work towards making sure our developer ergonomics make sense.

await lastValueFrom(source$);
await source$.lastValue();
await source$.toPromise();

Honestly, they're all a little unpalatable. lol

[cartant]

One reason for my wanting it removed from the protoype, is that I cannot recall using it outside of some tests that I wrote years ago.

It seems a little weird to have switched to static, pipeable, tree-shakeable operators, but still have (what ought to be, IMO) a rarely-used method on the prototype.

[kwonoj]

lastAsync(source)? Just borrowed .net binding semantic (last/firstorDefaultAsync). Doesn't look great too though. Or as we discussed, start from __unsafeSomething and collect opinion maybe.

I tend to agree making this as static method, mostly I would prefer observable itself to be thin enough.

[benlesh]

Decided that we want to go with static methods and deprecate toPromise. Since we would be adding new features and not introducing breaking changes, we can do this in 7.1,7.2 timeframe.

[OliverJAsh]

I appreciate this is an old thread but I wanted to raise my concerns with using exceptions/rejections for this, especially before v7 hits stable and people begin to rely on these new promise operators (lastValueFrom/firstValueFrom).

It's very easy to forget that some code might throw/reject. Ideally, the API would be designed so that the type system won't allow you to forget, so that code path is always handled. The undefined behaviour described above achieves this, but as mentioned, it has its own problems.

I think there is another solution, though. In fp-ts, the Array type has a last operator. (This is similar to Observable.toPromise except it operates synchronously so it doesn't need to return a Promise.)

https://github.com/gcanti/fp-ts/blob/2f21fa53bdf908c0ffa8e5e4ae1d4a882316f647/src/ReadonlyArray.ts#L461-L475

This function will return Option<T>—Option is just a box around the value T: { tag: 'Some'; value: T } | { tag: 'None' }.

Could we do something similar to this? This solution doesn't have the drawbacks of the other two solutions (returning undefined or throwing/rejecting). Options can be nested, unlike T | undefined. We wouldn't need to bring in a large abstraction to do this—Option is just a simple object wrapper, and we could call it something else to avoid confusion.

Going further, fp-ts has a NonEmptyArray type which also has a last operator. In this case, the last operator can just return T instead of Option<T>, because the type system has already been satisfied that the array contains at least one item. I would love to see something like this in Rx, e.g. a NonEmptyObservable type with a toPromise operator that returns Promise<T> rather than Promise<Option<T>>.

[cartant]

IMO, this is something that would be better handled in userland - with a function that wraps firstValue. There are other parts of RxJS that throw rather than return something like Option - e.g. the first operator - and I think API consistency is important.

And regarding NonEmptyObservable, I can say that - based on the experimentation that I did with rxjs-traits - we just don't have time for anything like that ATM. Getting the types sorted/fixed for v7 needs to happen first. Maybe in the future.

[OliverJAsh]

There are other parts o f RxJS that throw rather than return something like Option - e.g. the first operator - and I think API consistency is important.

Consistency is important, agreed! Maybe we could change first as well though, seeing as v7 is a major version—or would this require too many changes? I'm curious what other operators use exceptions in this way.

[cartant]

seeing as v7 is a major version

My understanding is that we want to limit breaking changes to types and incorrect behaviours only. Introducing something like Option would break many people.

first will throw, as will single. There was some discussion about forkJoin, too, but I'd need to look at the source to see what came of that.

[OliverJAsh]

Ah, fair enough. I do wonder though if it would better to avoid exceptions for these new operators instead of digging ourselves deeper into that hole for the sake of consistency—it's a trade off. If we use exceptions then it will make the migration more difficult later on, should we decide to move away from using exceptions/rejections in operators. If we use something like a (very tiny) Option then we will have a little bit of inconsistency but there would be fewer operators to migrate and the API would be less error prone (you can't forget to handle an Option). Given these are brand new operators, maybe this is a good chance to try something new.

[cartant]

Another thing is that with many non-trivial observable sources, there will always be the possibility that the source itself could error and that will need to be dealt with regardless of whether or not an Option is used.

[OliverJAsh]

Exceptions can happen anywhere, e.g. if the programmer makes a mistake resulting in a TypeError and somehow it's escaped the type checker—but that doesn't mean it's a good idea to throw them intentionally.

Consider this pseudo code where as is an array:

try {
  const a = as.map(fn).first();
} catch (error) {}

The programmer could make a mistake in fn which would result in an exception. As a result, running this expression would throw.

Similarly, the same thing could happen with observables (except of course the "exception" would be an error received by the observer):

as.map(fn).first().subscribe(
  a => {},
  error => {}
)

In both cases (array and observable), if first threw an exception, it would be very easy for the programmer to forget they need to handle this.

Programmer mistakes should bubble up as exceptions—but forgetting to handle the case of an empty array/observable when using first is a common mistake and one that's easily preventable if we design the API correctly so that it doesn't use exceptions.

(I'm focusing on first here for brevity.)

[benlesh]

@cartant I'm not sure if this conflicts with any previous statements I've made, but I'm not sure we can do that with forkJoin at least not in this version. I assume there are probably people who're using it in conjunction with a takeUntil or something to short-circuit things in some cases. Maybe I'm wrong.

I mean, it's totally debatable. It's a matter of principal about what we wanted to say forkJoin is. If we wanted to say it's a guarantee of the last value from each observable, then yeah, I guess it should error. If we wanted to say it's primarily a signal that all observables have completed, and we're happening to give you their last values, then it shouldn't error.

[benlesh]

first, without a default, has always thrown. FWIW. Same for single, although it's different there. That's a "one and only one" scenario.

If you don't want first to throw, it's first(undefined, 'I got nothing') (We should change this to a configuration object, BTW)

[OliverJAsh]

Just to clarify, I used first as a quick example to address @cartant's comment. I appreciate we won't be able to change first anytime soon, but these ideas equally apply to the new operators originally discussed in this issue (firstValueFrom, lastValueFrom, etc.). I should have used firstValueFrom in my example instead of first.

[cartant]

Closing this because this is the behaviour in the now-released v7.

@OliverJAsh I'm not opposed to an approach that does away with throwing/emitting errors, but I think it will have to encompass more of the API than just first/lastValueFrom. When I say that sources could error, I'm not just talking about exceptional circumstances. There are a whole bunch of ways in which a source observable could be composed that could see it emit an error to indicate that it's empty or something, so if first/lastValueFrom were to return an Option, it would need to test for the errors that imply it should be returning None. Otherwise, it could return None for some sources and reject for others. A nightmare.

Anyway, the API now allows for a default value to be returned for empty sources, so it ought to be a little more ergonomic.

So I'm closing this.

[OliverJAsh]

if first/lastValueFrom were to return an Option, it would need to test for the errors that imply it should be returning None. Otherwise, it could return None for some sources and reject for others

In my mind we would just switch out throw new EmptyError() for return Option.none (pseudo code). These operators wouldn't be responsible for checking the source to see if it already contains an EmptyError and converting that to a None. I agree it would be confusing if someone mixed operators which both threw (emitted an error) when empty and emitted None when empty, but I just see that as more reason to migrate all operators and completely do away with exceptions from all operators. (Of course we would have to consider how to handle the migration so we don't cause too much disruption for users.)

How about I open a new issue to track the idea of migrating operators away from exceptions for non-exceptional scenarios like when an observable is empty?