Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Strawman Proposal for toPromise replacement #5295

Merged
merged 1 commit into from
Mar 30, 2020

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Feb 3, 2020

Edit: I've updated the API to use lastValueFrom and firstValueFrom and added tests.

The idea here is to add at least one static method to convert an Observable to a Promise. Given that toPromise is flawed (citation from elsewhere in the repo needs to go here). The idea is to use static methods and deprecate toPromise.

The basic API is this:

async function test() {
   const data = await lastValueFrom(source$);
}

and additionally this features this idea:

async function test() {
   const data = await firstValueFrom(source$);
}

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Feb 3, 2020
@kwonoj
Copy link
Member

kwonoj commented Feb 3, 2020

what about settleFirst / settleLast? named from promise's settle (https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/Promise/allSettled)

@kwonoj
Copy link
Member

kwonoj commented Feb 3, 2020

technincally, first / last both can settle without value, right? (i.e can reject) for that I hope it could be name without value somewhere, but that's my 5c feeling.

@benlesh
Copy link
Member Author

benlesh commented Feb 3, 2020

My argument for finalResult is simply that "lastValue" makes it sound like you'll somehow magically get the value when it arrives, when you simply can't know it's the last value until it completes. But I'm malleable in my stance.

@benlesh
Copy link
Member Author

benlesh commented Feb 3, 2020

what about settleFirst / settleLast

@kwonoj Interesting! I feel like that sounds like it's going to settle the first or last promise of a collection of promises, personally. But it's a good idea to look for language from the spec we can work with. I'll think about that one.

@kwonoj
Copy link
Member

kwonoj commented Feb 3, 2020

along with naming something rolls my eye is having first/last pair, would like to have single surface only but also don't have great suggestion how to get rid of it.

@cartant
Copy link
Collaborator

cartant commented Feb 3, 2020

My preference would be last/first, as I think last is the more natural antonym for first.

@lacolaco
Copy link

lacolaco commented Feb 3, 2020

usually I create the similar utility as await useFirstValue(source$). I've never hit a usecase that I want something for last, but maybe useful.

@ambirdsall
Copy link

I think last is the more natural antonym for first.

I agree that "last" is the most natural antonym for "first" in a vacuum; in this specific context, I think that "final", which connotes completing more strongly than "last" does while being synonymous with regard to ordering in a sequence, is better.

@rapzo
Copy link

rapzo commented Feb 3, 2020

How about endOf? After all, there's no more stream flow there, right?
final could also make its case, since that value won't change again, but might seem get weird with other language ports.

@cartant
Copy link
Collaborator

cartant commented Feb 3, 2020

... that "last" is the most natural antonym for "first" in a vacuum; ...

It's definitely not in a vacuum; last is already an operator with the same "final" context. And there is a first operator, too.

first / last and firstValue / finalValue just seems incongruous, to me.

@cartant
Copy link
Collaborator

cartant commented Feb 4, 2020

How about endOf?

I kinda like startOf and endOf, but I don't think endOf conveys that the stream has to emit a value or the promise will reject. Streams that complete without emitting a value still end.

@kolodny
Copy link
Member

kolodny commented Feb 4, 2020

I think it's going to be weird to have something like this

import {last} from 'rxjs';

const something = last(ob$)

And assume the user thinks they'll get a Promise as opposed to every other static method imported from rxjs which returns an observable. I'd lean towards keeping the name of the method toPromise and allowing the user to specify a second isLast argument for that specific use case. This feels icky but ignoring the existence of a kinda broken method with the same name, it best describes what the function is

// rxjs.ts
export const toPromise<T>(ob$: Observable<T>, isLast = false): Promise<T> {
  if (isLast) return internalLast(ob$);
  else return internalFirst(ob$);
}

@chaosmonster
Copy link

Okay maybe this is me being nitpicking. Last is (for me) obviously the last value before the stream completed. But first must not be the first value pushed in the stream. It’s not that I don’t like first, but something feels „off„

@cartant
Copy link
Collaborator

cartant commented Feb 4, 2020

nextValue instead of firstValue and completedValue instead of finalValue?

@KylerJohnson26
Copy link

My vote is for finalFrom.

const users = finalFrom(users$);

@benlesh
Copy link
Member Author

benlesh commented Feb 4, 2020

Okay, I'm back on board with firstX and lastX, given their usage in the rest of the library.

I'm not sure I like just lastValue though. It's not a great function name. Given that it will be used mostly with await, I think getLastValue doesn't seem right either: const x = await getLastValue(source$)... yuck..

I think I like const x = await lastValueFrom(source$). And conversely: const x = await firstValueFrom(source$).

Thoughts?

@cartant
Copy link
Collaborator

cartant commented Feb 6, 2020

I think I like const x = await lastValueFrom(source$). And conversely: const x = await firstValueFrom(source$).

Thoughts?

SGTM. Matching the terminology of existing operators and reading nicely with async/await syntax are the most important aspects, IMO.

@rapzo
Copy link

rapzo commented Feb 6, 2020

Warning: no complain here!

Isn't *ValueFrom? a bit redundant?
Assuming function firstValueFrom<T>(source$: Observable<T>): Promise<T>
Since no value means error, and no other operation has the Value appendix, wouldn't it be cleaner without?
function firstFrom<T>(source$: Observable<T>): Promise<T>

Kinda makes me think, as is, should come out as:
function firstValueFrom<T>(source$: Observable<T>): Promise<Value<T>>

Just asking...

@benlesh
Copy link
Member Author

benlesh commented Feb 6, 2020

@cartant @kwonoj @kolodny @jwo719 consider this a PR then. Please review.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM

source$.subscribe({
next: value => {
resolve(value);
subs.unsubscribe();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that this will handle the degenerate case of an infinite synchronous source. To handle that, I think this would have to unsubscribe via the next callback's this instead. Thoughts?

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Feb 12, 2020
@benlesh benlesh merged commit e69b765 into ReactiveX:master Mar 30, 2020
@benlesh
Copy link
Member Author

benlesh commented Mar 30, 2020

@rapzo ... we don't want it to conflict with the first and last operators.

@backbone87
Copy link
Contributor

backbone87 commented Apr 15, 2020

when used with a hot observable firstValueFrom is actually nextValueFrom. i think this naming is confusing.

what if i want the index, too?
const { value, index } = await firstFrom(of(1, 2, 3));
const { value, index } = await lastFrom(of(1, 2, 3));

@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants