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

Should first() and last() support a default value? #132

Open
domfarolino opened this issue Apr 2, 2024 · 7 comments
Open

Should first() and last() support a default value? #132

domfarolino opened this issue Apr 2, 2024 · 7 comments
Labels
possible future enhancement An enhancement that doesn't block standardization or shipping

Comments

@domfarolino
Copy link
Collaborator

Just as an initial note to this issue: resolution of this issue is not required for the shipping of either first() or last() APIs.

It came to my attention through looking at the original WPTs for these APIs that there was an expectation of a "default" value for these APIs, in case they complete without emitting any values. RxJS appears to support this too (see https://rxjs.dev/api/operators/first & https://rxjs.dev/api/operators/last).

Personally I don't see this as necessary, since you could easily catch the case where completion happens without any values being emitted, and handle it yourself. But maybe the ergonomics is useful enough to include this? Either way, support for a default value could come at any point in the future too I think.

@domfarolino domfarolino added the possible future enhancement An enhancement that doesn't block standardization or shipping label Apr 2, 2024
@benlesh
Copy link
Collaborator

benlesh commented Apr 4, 2024

The analog in RxJS (because promises) is actually firstValueFrom and lastValueFrom, which do support default values. As if they're empty they should reject, and that's not always desirable.

IMO, I think it's a strong add, and anecdotally, I've seen it used frequently

@domenic
Copy link
Collaborator

domenic commented Apr 4, 2024

Back when I did .NET, their enumerables had LastOrDefault and FirstOrDefault, which I thought were clear names and I remember using them often.

I do feel wary about getting ahead of iterator helpers for things like this, though.

@bakkot
Copy link
Contributor

bakkot commented Apr 4, 2024

Iterator helpers are unlikely to get first or last methods at all, IMO. They don't seem clearly motivated to me for iterators and I don't think anyone's expressed interest in pushing for them.

@domfarolino
Copy link
Collaborator Author

I do feel wary about getting ahead of iterator helpers for things like this, though.

Do you mean "getting ahead" by including these methods at all, in the first place?


Separately, I imagine first() (maybe less-so last()) would be used quite a bit for one-off events and would rarely depend on a default value being supplied, so changing the name to firstValueFrom or firstOrDefault feels kinda clunky for most really simple, cute use cases. Just from reading that I feel like I would double take upon every usage, wondering if I need to supply some input value, and having to cognitively process the rest of the words besides "first" to know if I'm doing the right thing. Maybe I'm just lazy though.

@domenic
Copy link
Collaborator

domenic commented Apr 5, 2024

Do you mean "getting ahead" by including these methods at all, in the first place?

Yes. Although if @bakkot thinks we're just not going to have this family for iterators, then maybe it's fine. I think in some ecosystems we'd push for consistency by including even useless methods, as long as they make sense. But oh well.

so changing the name to firstValueFrom or firstOrDefault feels kinda clunky for most really simple, cute use cases

Sorry, my suggestion was four methods: first(), firstOrDefault(defaultValue), last(), lastOrDefault(defaultValue). In JS you actually get a nice bonus where firstOrDefault() with no argument is equivalent to firstOrDefault(undefined), which seems helpful.

@domenic
Copy link
Collaborator

domenic commented May 4, 2024

I've thought about this a slight bit more and wanted to capture my thoughts.

Potential functionality you might want:

  • first/last/find, or undefined if empty/not found
  • first/last/find, or user-supplied default if empty/not found
  • first/last/find, or throw if not found

What iterator helpers/Array.prototype provides: find, or undefined if not found. So, let's stay consistent with that for Observable's find().

This leads to two choices to get the full functionality:

  1. If you want first/last to be consistent with find:

    • first() etc. return undefined if empty/not found.
    • first({ defaultValue }) etc., or firstOrDefault(defaultValue) etc., return defaultValue if empty/not found.
    • firstOrThrow() etc. throw if empty/not found
  2. If you don't want first/last to be consistent with find:

    • first() and last() throw if empty.
    • firstOrDefault(defaultValue) and lastOrDefault(defaultValue) return defaultValue if empty. (And automatically, firstOrDefault() returns undefined if empty.)
    • find() returns undefined if not found.
    • find({ defaultValue }) or findOrDefault(defaultValue) return defaultValue if not found.
    • findOrThrow() throws if not found.

We don't have to add anything beyond the first bullet point to start. Realistically, unless you are commonly dealing with observables containing undefined as a value, it is quite easy to get the other functionality by using fallback code. (E.g. observable.first().map(v => v ?? defaultValue).) But we should consciously choose between (1) and (2) as potential future paths. And those paths might spill back into iterator helpers or even Array.prototype.

From this perspective, I like (1) more.

@domfarolino
Copy link
Collaborator Author

If you want first/last to be consistent with find:
[...]
From this perspective, I like (1) more.

Oh yeah, I'm personally very much sold on the path of being consistent with find(). For one, having to think of which methods throw when "the thing" is not found vs. not (or less scary, which methods respect defaults or otherwise have "default-consuming" counterparts) seems tricky to me.

I also like (1), and starting with the first bullet point is my preference for now. That basically means simplifying #131 and #144 to not use RangeErrors, and then keeping this issue open for future consideration of adding either a default-consuming counterpart method, or an optional default value parameter for first() and last(). So I think the "possible future enhancement" label for this issue is sufficient while work is done in the other PRs.

Thanks for the feedback, it's much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
possible future enhancement An enhancement that doesn't block standardization or shipping
Projects
None yet
Development

No branches or pull requests

4 participants