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

feat: add (optional) defaultValue parameters to firstValueFrom and lastValueFrom #6204

Merged
merged 5 commits into from
Apr 16, 2021

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Apr 3, 2021

Description:

This PR adds an optional defaultValue parameter to firstValueFrom and lastValueFrom. The parameters are added using an overload to avoid this issue (for some usage, at least).

The first and last operators both support defaults and I see no reason why firstValueFrom and lastValueFrom should not do the same. Sure, you could apply defaultIfEmpty to the source, but you say the same for first and last. And, IMO, including defaultValue in the signatures serves as a reminder that the functions expect a value and that not having an available value is an unexpected/error situation.

Also, supporting default values would make implementing a user-land equivalent that resolves with an Option straightforward. (IMO, introducing a Option isn't something we should do while there are bunch of APIs that throw for empty sources.)

Related issue (if exists): Nope

@cartant cartant requested a review from benlesh April 3, 2021 01:10
Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

@cartant I think this should probably be in a config/options object. I say this because there's a strong possibility that this will support AbortSignal at some point:

const ac = new AbortController();

firstValueFrom(source$, { defaultValue: 2, signal: ac.signal });

@cartant cartant force-pushed the cartant/value-from-defaults branch from a218d77 to 1d05122 Compare April 9, 2021 07:36
@cartant
Copy link
Collaborator Author

cartant commented Apr 9, 2021

@benlesh

I think this should probably be in a config/options object.

Nice catch. Done.

src/internal/lastValueFrom.ts Outdated Show resolved Hide resolved
@cartant cartant requested a review from benlesh April 15, 2021 22:49
@benlesh benlesh merged commit df51b04 into ReactiveX:master Apr 16, 2021
@cartant cartant deleted the cartant/value-from-defaults branch April 18, 2021 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants