WICG / observable

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

do vs forEach #29

Closed jakearchibald closed 1 year ago

jakearchibald commented 1 year ago

https://github.com/domfarolino/observable#concerns

There's this example:

element.on('click').map(e => (e.preventDefault(), e)).first()

Why wouldn't it be:

element.on('click').forEach(e => e.preventDefault()).first()

As in, what's the difference between forEach and the proposed do method?

domfarolino commented 1 year ago

My understanding is that forEach() doesn't pay attention to the return value of the callback it consumes (it's a VoidFunction), so it can't pass any values along. So I don't think forEach() would give you something you can call first() on (see https://github.com/domfarolino/observable/issues/27).

do() on the other hand seems to be this thing that is now in Observables/RxJS world called tap/pipe, which is just a pass-through and can do stuff with each received value, and transparently pass it along without explicitly returning it: https://stackoverflow.com/a/60418872. Probably @benlesh has more context on do() in the Observable universe.

jakearchibald commented 1 year ago

Buuuut isn't the example here using map just to pass the same value back? Isn't that how forEach behaves?

domfarolino commented 1 year ago

No because forEach can't pass any value back if we're trying to match iterator helpers, right? Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach#description:

Unlike map(), forEach() always returns undefined and is not chainable. The typical use case is to execute side effects at the end of a chain.

So I think it and the callback it consumes both return undefined, meaning it has a subset of the functionality map() does? I'm really sorry if I'm missing the point you're making though!

jakearchibald commented 1 year ago

I'm sure, from experience, that I'm being the dumb one here. What does do do with the return value?

jakearchibald commented 1 year ago

Like, my instinct is that forEach ignores the return value, which is what you want if you're calling preventDefault. Like, the map example is deliberately passing back the same value, to sort-of fake a forEach.

Or, does forEach not pass-through? If so, could it? If that's the only difference between do and forEach

domfarolino commented 1 year ago

Or, does forEach not pass-through?

Yeah I believe this is correct. So forEach is indeed ignoring the return value as you mentioned in your first sentence, but that's because it always has no return value period — it's not passing the element (or anything) through at all.

If so, could it?

This is interesting. It could, but then I think we'd be diverging from how iterator helpers's forEach/Array.forEach is designed, which feels a little odd. I think this is what I originally had in mind, which is why it returns an Observable (to signal each value in the array) but @domenic sort of corrected me when pointing out that forEach should probably return undefined or a promise that resolves to it.

jakearchibald commented 1 year ago

Ahh ok, I get it now. So do is just forEach except it returns this. Hmm.

benlesh commented 1 year ago

It might be easier to explain if iterator helpers had an imaginary do method. (Iterable being a dual of Observable).

const myIterator = [1, 2, 3][Symbol.iterator]();

const iteratorWithSideEffects = myIterator.do(value => console.log(`do ${value}`));
// nothing happens as of this point.

iteratorWithSideEffects.forEach(console.log);
// Logs
// "do 1"
// 1
// "do 2"
// 2
// "do 3"
// 3
benlesh commented 1 year ago

Ahh ok, I get it now. So do is just forEach except it returns this. Hmm.

forEach causes subscription. Similar to how it causes iteration in iterator helpers. If that makes sense. do does not cause it, it just sets up a side effects.

benlesh commented 1 year ago

To that end, iterators, promises, et al, all are lacking a "do" type mechanism and therefor I think people will know how to work around it when they just need to have a side effect without transforming the value. I don't think do is requirement.

appsforartists commented 1 year ago

Perhaps the most common need for these sort of inline side effects is logging.

Onboarding/debugging Observables can be a lot, mentally speaking. It's really useful to be able to .tap(console.log) to see when/how data flows through the system. In fact, our observable library has a log(path?) method where path is a key on the upstream object; hence log('x') would be equivalent to tap(({ x }) => console.log(x).

I know extra operators are out-of-scope for the first round in this proposal, but I do think that the novelty of the reactive mental model might make the ability to quickly log data as it flows through the system more useful than in sync counterparts.

There are other ways to solve this (DevTools support, augmenting Observable.prototype in userland, etc.), but it's worth pointing out that this type of method (map but returns this) might be more valuable to this type than to its predecessors.

ljharb commented 1 year ago

imo if there's a compelling use case for tap, then it should be added to arrays, collections, iterator helpers, and potentially observables (and maybe even promises) all at once in its own proposal. I for one would love to see a compelling argument for it be made.

bakkot commented 1 year ago

There's a compelling argument for tap on iterator helpers and observables which isn't relevant to arrays or promises. I intend to propose tap for iterator helpers eventually. (I would not spell it do.)

Copying my comment over from that thread:


Java's Streams call this peek (which is a terrible name), and you can read some of the discussion around adding it in this thread. In particular, the motivation was explicitly to have a place to stick a breakpoint (or a print or whatever) when debugging.

That's not something you need on Array because the equivalent methods on Arrays are eager, so you can break up a chain of .filter().map().forEach() methods and breakpoint between them. But since iterators are lazy there's not a place to put the breakpoint; the iterator isn't consumed until the final forEach (or whatever). I find this motivation pretty compelling (for a followup proposal).

domfarolino commented 1 year ago

I agree @bakkot, that is pretty compelling, and thanks for spelling out some of the history there! It seems like we've certainly cleared up the difference between do()/tap() and forEach(), and it also sounds like we agree that something like tap() has a future. Since I think we're aligned that it need not be a part of this specific proposal, but could be a useful addition as a part of wider-reaching future work, I'll err on the side of closing this but please re-open if you want to continue discussion etc.