WICG / observable

Observable API proposal
https://wicg.github.io/observable/
Other
543 stars 12 forks source link

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

Open domfarolino opened 3 months ago

domfarolino commented 3 months ago

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.

benlesh commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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

Potential functionality you might want:

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 commented 2 months ago

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!