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

Initial operators/methods to add, and a few to remove #126

Open
benlesh opened this issue Mar 5, 2024 · 4 comments
Open

Initial operators/methods to add, and a few to remove #126

benlesh opened this issue Mar 5, 2024 · 4 comments

Comments

@benlesh
Copy link
Collaborator

benlesh commented Mar 5, 2024

Summary

Adding

  1. Observable switchMap(mapper)
  2. Observable catch(mapper)
  3. Observable finally(callback)
  4. Promise first()
  5. Promise last()
  6. Observable scan(reducer, initialValue?)

Removing

  1. Promise some(predicate)
  2. Promise every(predicate)
  3. Promise find(predicate)
  4. Promise toArray()
  5. Promise reduce(reducer, initialValue?)

Desire to followup later

  1. Observable tap(callback/Observer)

Reasoning: Why deviate from async iterator helpers?

reduce and toArray() don't make sense for events, scan does

While this does deviate from the async iterator helpers proposal, observables target a different set of use cases. reduce() and toArray() in particular make much more sense in an async iterable, where you might be iterating through an array of items, then performing some async action to get a new array of items; Where it doesn't make much sense to reduce mouse clicks or make an array of move events.

To that end, scan is a much more common task, where one might want to continuously emit reduced state (ala Redux or React's useReducer) as it's changed by events, and scan().last() is the same as reduce(). Similarly, in my many years working with observables via RxJS, I've found that toArray() is usually adjacent to poorly written code, that does something like take an observable of arrays, flatten it to single values, do something with them, then re-accumulate them as an array. An iterable or simple array methods would likely be a better choice for such a task.

catch and finally

It may be obvious, but iterators and async iterators simply have a different mechanism for communicating errors or finalization and handling them. Observable needs catch and finally methods that behave very similarly to the ones that promise provides.

catch has a common use case with flatMap for keeping source streams alive when child streams error:

// Without the catch statement below,
// ALL observation would stop if the observable
// returned by `startDataStream()` errored.

element.on('click').flatMap(() => {
  return startDataStream()
    .catch(err => {
      console.error(err);
      return []; // empty
    })
})
.subscribe(console.log);

finally has use cases around clean up that you want to occur on unsubscription, completion, or error, and you reproduce between subscriptions to the same observable.

// Without the `finally` method use below,
// the `socket` will not be cleaned up appropriately

const controller = new AbortController();
const { signal } = controller;

takeThreeButton.on('click')
  .flatMap(() => {
    const socket = new WebSocket(url);
    return socket.on('message')
      .take(3)
      .finally(() => socket.close())
  })
  .subscribe(console.log, { signal })

first and last

The ability to get the first or last value from an observable as a promise is an important tool for interoperability with promise-based approaches. last() with scan() allows users to create reduce() or toArray() functionality easily. first() also allows users to quickly get a promise from the new observable event API that is consumable in an async function:

async function waitForClick(button) {
  await button.on('click').first();
}

Where the alternative would be:

function waitForClick(button) {
  return new Promise((resolve) => {
    button.addEventListener('click', () => resolve(), { once: true })
  });
}

The need for switchMap

switchMap is a method that really takes the most advantage of observable's design, and is commonly the reason developers reach for observables: It has implicit unsubscription when it gets a new value from the source and then maps to a new observable stream it switches to.

It can be used for many dynamic use cases that are difficult to compose with out it

// An imaginary lookahead search, that queries for new search results
// as the search input changes, but only if the search text has a length
// 3 or longer.

searchInput.on('change')
  .map(e => e.target.value)
  .filter(queryText => queryText.length >= 3)
  .switchMap((queryText, i) => getResults(queryText))
  .subscribe(results => renderLookaheadOptions(results))
// A scenario where updating a url in a text box
// reconnects a socket stream to a new endpoint
// and automatically sends a message to start streaming values.

function getSocketStream(url, initialMsg) {
  const socket = new WebSocket(url);
  
  return socket.on('open').flatMap(() => {
    socket.send(initialMsg);
    return socket.on('message');
  });
}

urlTextInput.on('change')
  .map(e => e.target.value)
  .filter(url => validateUrl(url))
  .switchMap(url => getSocketStream(url, { start: 'stream' }))
  .subscribe(console.log);

Without switchMap all of the above scenarios would require doing some sort of weird dance with flatMap and takeUntil in order to get the proper outcome.

Why follow up on tap later

The reason to follow up on tap later, is simply that you can use map to deal with tap's most common use case, creating a side effect from a value. source.map(x => (sideEffect(x), x)) is the same as source.tap(sideEffect). tap does have many other use cases, and it is a broadly used thing, but if it was down to picking between tap and the operators listed above it, we should pick the others.

@domenic
Copy link
Collaborator

domenic commented Mar 6, 2024

Adding seems fairly reasonable, but I am frustrated by the regressions proposed here by removing useful functionality.

@domfarolino
Copy link
Collaborator

domfarolino commented Mar 6, 2024

I also think adding seems reasonable, although I think I lean towards putting scan() on the list of operators that we should consider adding as a follow-up, just to keep the initial list slim and mostly matching that of iterator & async iterator helpers.

As for removals, I was hoping we'd discuss this in a separate issue to keep the discussion of each more streamlined. But I've thought about this more and here are my thoughts. I am kind of torn between:

  1. Removing lots of the Promise-returning operators like toArray(), reduce(), every(), some(), find(), etc., because they don't have "clear" use cases for events (largely the Observables use case), and could theoretically be a footgun(?)
  2. Sticking with the precedent that both iterator and async iterator helpers set, which do include these operators.

Basically I'm not sure if (1) is a strong enough concern to override the precedent of (2), and how important it is to stick with the precedent of (2) overall.

Async iterator helpers' insistence on providing these operators actually makes me more confident that their inclusion in our proposal is appropriate and less problematic. Especially given our conversion operator from() that lets us go from arbitrary async iterables ➡️ Observables, it would feel weird if doing so came with a loss of familiar operator functionality that you might reasonably want to maintain.

One argument for the removal of these operators — largely in the event target use case — is that there is not clear "endpoint" of events, so things like reduce() & every() might not make as much sense. But operators like takeUntil() (and possibly more in the future) provide a clear endpoint even in the event target case. I can come up with a lot of fun toy-ish examples where these operators are actually useful. Whether these cases are likely to be replicated in real-world code is an interesting question that I'd like to hear from developers about, but I think we should default towards including these operators unless strong operator-by-operator concerns override this, instead of defaulting to remove all of them.

@bakkot
Copy link
Contributor

bakkot commented Mar 7, 2024

FWIW I think at least some / every / find / reduce are still natural things to want for events.

For some / every, you might want to check if a click-and-drag ever passes over a particular region, or was always within a particular region. For find, you might want to look for a key pressed while holding alt. For reduce, you might want want to find the maximum height of the cursor during a click-and-drag.

And sure, these are all pretty simple and can be implemented in other ways from other operators. But given that JS programers are likely to be familiar with these operators, and that they might reasonably want to reach for them, I would be inclined to keep them.

toArray I'm happy to defer to @benlesh's experience here and omit. I don't have an obvious use case for it which wouldn't better be served by some other operation, at any rate.

@domfarolino domfarolino changed the title Initial methods to add, and a few to remove Initial operators/methods to add, and a few to remove Apr 3, 2024
@benlesh
Copy link
Collaborator Author

benlesh commented Apr 4, 2024

I think it's okay if it's all additive. I wouldn't want to trade anything I've mentioned above for reduce, every, or some.

For some / every, you might want to check if a click-and-drag ever passes over a particular region, or was always within a particular region. For find, you might want to look for a key pressed while holding alt. For reduce, you might want want to find the maximum height of the cursor during a click-and-drag.

The tricky thing here is they return single values (as promises in this case, but as observables in RxJS), and most of the time what people want to set up is: "Every time someone enters a region, do X" or "every time someone enters a region do X, while they stay in it, do Y", or "Every time someone presses a key while ALT is held do X". Because of this I rarely see these being used. Instead I'd see things like:

// you might want to check if a click-and-drag ever passes over a particular region
draggable.on('dragstart')
  .flatMap(() => 
    region.on('dragover')
      .tap(e => e.preventDefault())
      .takeUntil(document.on('dragend'))
      .first()
  )
  .subscribe(() => console.log('dragged over the region'))
  
// For find, you might want to look for a key pressed while holding alt
input.on('keydown').filter(e => e.altKey).subscribe(e => {
  // Do something every time the a key is pressed while ALT is held
  console.log(`alt + ` + e.key);
})

I think of all of those mentioned, reduce might be the most useful... but it comes with the cumbersome thing of "what if there's no values from the source?" because it's promise-based. In the end reduce(fn, init) is the exact same thing as scan(fn, init).last().

// For reduce, you might want want to find the maximum height of the cursor during a click-and-drag

movable.on('mousedown')
  .flatMap(e => {
    return document.on('mousemove')
      .takeUntil(document.on('mouseup'))
      .scan((maxY, { clientY }) => Math.max(maxY, clientY), e.clientY)
      .last()
  })

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 24, 2024
This CL implements the following Observable operators:
 - some()
 - every()
 - find()

Their inclusion is to be consistent with Iterator helpers [1] and Async
iterator helpers [2], and is discussed in
WICG/observable#126 and to a lesser extent
WICG/observable#106.

See WICG/observable#137 for the specification.

[1]: https://github.com/tc39/proposal-iterator-helpers
[2]: https://github.com/tc39/proposal-async-iterator-helpers

For WPTs:
Co-authored-by: [email protected]

Bug: 40282760
Change-Id: I0fcbb9d716d6ef1727b050fd82a6fa20f53dea4b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 29, 2024
This CL implements the following Observable operators:
 - some()
 - every()
 - find()

Their inclusion is to be consistent with Iterator helpers [1] and Async
iterator helpers [2], and is discussed in
WICG/observable#126 and to a lesser extent
WICG/observable#106.

See WICG/observable#137 for the specification.

[1]: https://github.com/tc39/proposal-iterator-helpers
[2]: https://github.com/tc39/proposal-async-iterator-helpers

For WPTs:
Co-authored-by: [email protected]

Bug: 40282760
Change-Id: I0fcbb9d716d6ef1727b050fd82a6fa20f53dea4b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 29, 2024
This CL implements the following Observable operators:
 - some()
 - every()
 - find()

Their inclusion is to be consistent with Iterator helpers [1] and Async
iterator helpers [2], and is discussed in
WICG/observable#126 and to a lesser extent
WICG/observable#106.

See WICG/observable#137 for the specification.

[1]: https://github.com/tc39/proposal-iterator-helpers
[2]: https://github.com/tc39/proposal-async-iterator-helpers

For WPTs:
Co-authored-by: [email protected]

Bug: 40282760
Change-Id: I0fcbb9d716d6ef1727b050fd82a6fa20f53dea4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5426595
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1293776}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 29, 2024
This CL implements the following Observable operators:
 - some()
 - every()
 - find()

Their inclusion is to be consistent with Iterator helpers [1] and Async
iterator helpers [2], and is discussed in
WICG/observable#126 and to a lesser extent
WICG/observable#106.

See WICG/observable#137 for the specification.

[1]: https://github.com/tc39/proposal-iterator-helpers
[2]: https://github.com/tc39/proposal-async-iterator-helpers

For WPTs:
Co-authored-by: [email protected]

Bug: 40282760
Change-Id: I0fcbb9d716d6ef1727b050fd82a6fa20f53dea4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5426595
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1293776}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 2, 2024
…d()` Observable operators, a=testonly

Automatic update from web-platform-tests
DOM: Implement `some()`, `every()`, `find()` Observable operators

This CL implements the following Observable operators:
 - some()
 - every()
 - find()

Their inclusion is to be consistent with Iterator helpers [1] and Async
iterator helpers [2], and is discussed in
WICG/observable#126 and to a lesser extent
WICG/observable#106.

See WICG/observable#137 for the specification.

[1]: https://github.com/tc39/proposal-iterator-helpers
[2]: https://github.com/tc39/proposal-async-iterator-helpers

For WPTs:
Co-authored-by: [email protected]

Bug: 40282760
Change-Id: I0fcbb9d716d6ef1727b050fd82a6fa20f53dea4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5426595
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1293776}

--

wpt-commits: 8c6b479fae23badbb86e9c993f5b5fbb66e16905
wpt-pr: 45875
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue May 2, 2024
…d()` Observable operators, a=testonly

Automatic update from web-platform-tests
DOM: Implement `some()`, `every()`, `find()` Observable operators

This CL implements the following Observable operators:
 - some()
 - every()
 - find()

Their inclusion is to be consistent with Iterator helpers [1] and Async
iterator helpers [2], and is discussed in
WICG/observable#126 and to a lesser extent
WICG/observable#106.

See WICG/observable#137 for the specification.

[1]: https://github.com/tc39/proposal-iterator-helpers
[2]: https://github.com/tc39/proposal-async-iterator-helpers

For WPTs:
Co-authored-by: [email protected]

Bug: 40282760
Change-Id: I0fcbb9d716d6ef1727b050fd82a6fa20f53dea4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5426595
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Dominic Farolino <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1293776}

--

wpt-commits: 8c6b479fae23badbb86e9c993f5b5fbb66e16905
wpt-pr: 45875
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants