tc39 / proposal-observable

Observables for ECMAScript
https://tc39.github.io/proposal-observable/
3.06k stars 90 forks source link

Rename "error/complete" to "end" #185

Open staltz opened 6 years ago

staltz commented 6 years ago

This is a minor cosmetic suggestion for the Observer interface. Combine error and complete into end.

type Observer<T> = {
  start: (cancelFn) => void;
  next: (x: T) => void; 
  end: (error?: any) => void;
};

end(e) corresponds to error(e) and end() corresponds to complete().

Reasoning:

Motivation for naming is normally cosmetic anyway, so this is not an exception.

ljharb commented 6 years ago

I’m not a fan of having a variadic method like end; I’d much prefer 2 simpler methods to 1 variadic one.

staltz commented 6 years ago

Why?

mattpodwysocki commented 6 years ago

@staltz I'm in the same boat as @ljharb on this one, I'd rather be explicit about complete or error in that null, undefined and others are perfectly valid errors that you can submit. I'd rather be explicit about which we're actually talking about, an error condition or normal termination.

I know for Node Streams, they had end(data) which had support for data or not, which would be the equivalent of next(x) and then complete() but that's not what we're having suggested here.

staltz commented 6 years ago

null, undefined and others are perfectly valid errors that you can submit

Are they? How would that be semantically different to complete?

ljharb commented 6 years ago

Why?

Having separate functions, like .then vs .catch, makes code much easier to reason about than if we had a single .fulfilled(value, reason) method (to use Promises as an example). Overloaded function signatures makes it harder, in my experience, to maintain code using those functions.

appsforartists commented 6 years ago

@staltz's point about the confusion about error being terminating is an important nuance that I think is getting lost in this conversation. No one would expect promise.catch to be called more than once, but it's easy for newcomers to expect all errors to go down observable.error, not just fatal ones. I don't know if fatalError or die would be more semantic names.

staltz commented 6 years ago

@ljharb I think it's hard to compare with Promises, because then operates on a value and so does catch operate on an error value. With Observers, error operates on a value but complete never does.

ljharb commented 6 years ago

Promises have .finally which don’t operate on a value.

I’m sensitive to the concern about naming; I’m only objecting to conflating two purposes under one name. Perhaps “.catch” is a better name than “.error” because it implies “once”?

benjamingr commented 6 years ago

I'd expect most people to use .forEach anyway since you can await it

rdeforest commented 6 years ago

Hello! Robert "wall of text" de Forest here...

Two issues under discussion

  1. Should there be a distinct message name for each way a stream can terminate (normal/abnormal/...l), or should the stream termination message include an optional argument specifying the termination mode/reason?

  2. What should the termination message(s) be named?

My position

Issue 1: message name describes termination mode

I agree with @ljharb that variadic methods can be a bad code smell. If the message consumer cares about whether there was an error or not they have to put a test in their handler of the termination message. This seems to go against the philosophy of Observables where everything is supposed to be a simple mapping from problem to solution.

Also, treating two messages identically is trivially easy:

weAreDone = (whoCares) -> cleanUpMyMess()
myObserver =
  aborted: weAreDone
  ended:   weAreDone

I also agree with @staltz's point about the the value of the symmetry of begin/end. Begin/next/end has an obvious life-cycle-ness to it compared to begin/next/(error|end). If I don't care why a stream ended, having one message to respond to is a nice simplification. My intuition says there will be far fewer cases when a stream consumer only cares about one but not the other reason for termination. The arguments around then/catch and try/catch/finally are compelling but do not convince me because streams are different. The life cycles of promises and try/catch blocks are smaller in scope than the life cycle of a stream.

It's trivial to map the single message protocol to a multi-message protocol:

separateErrorSignal = (observable) ->
  begin: observable.begin
  next: observable.next
  end: (error) ->
    if error
      observable.error error
    else
      observable.end()

Given how easy it is to map between the two answers, my motivation shifts towards the philosophical: how do we decide which information goes in the message name and which goes in the args?

Including data in the name of a message moves logic from the handler to the map which connects the message to its handler.

The Observer pattern is itself a specialization of the EventEmitter/Listener pattern in which the event name becomes a method name. We already know that Observers are listenting to events so instead of requiring them to .on every method they care about, they can just supply the Producer with a mapping from message name to handler. Instead of .on "next", processValue we have next: processValue. This transformation isn't motivated by keystroke optimization. It's an optimization for the ways we expect the interface will be used. Many Listeners will only care about data events. Some will only care about begin or end. How many will care about error but not end, or vice-versa? I wager not many, but I have no data to bring to bare.

Issue 2: what do we call it/them?

If we decide to have a distinct message for abnormal stream termination, it should be called something which makes it clear that this will only happen once per stream instance. The Perl programmer in me did a little dance @appsforartists proposed die, but the pragmatist in me thinks this is probably not a good name. While it's true the stream is dying, the message is coming from the dying stream and is not an imperative. Given begin, next and end, perhaps some better names for the error message might be aborted, collapsed, choked, broke, etc. For some reason I favor the past tense for this message?

If we decide to have a single message for all termination modes, I strongly support calling it end to match begin, or renaming them both if begin/end are for some reason inadequate.