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

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

Open
staltz opened this issue Feb 21, 2018 · 10 comments
Open

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

staltz opened this issue Feb 21, 2018 · 10 comments

Comments

@staltz
Copy link

staltz commented Feb 21, 2018

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:

  • Make start and end explicitly "symmetric" names to contain the observable execution
  • Avoid misusing error for (impossible) multiple errors:
    • Often with RxJS, people want to call error multiple times. They interpret it as a sister to next, not as a sister to complete. The correct interpretation is "termination due to failure" but people often interpret it as "notify about an error".
  • Reducing the number of methods in Observer, from 4 to 3
  • The argument e for end(e) works like Unix process exit codes, where the argument of least information (in Unix, zero) means termination with nothing bad.
  • Aesthetic ordering: start has 5 characters, next 4, end 3.

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

@staltz staltz changed the title error/complete => end Rename "error/complete" to "end" Feb 21, 2018
@ljharb
Copy link
Member

ljharb commented Feb 21, 2018

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

@staltz
Copy link
Author

staltz commented Feb 21, 2018

Why?

@mattpodwysocki
Copy link
Collaborator

mattpodwysocki commented Feb 21, 2018

@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
Copy link
Author

staltz commented Feb 21, 2018

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

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

@ljharb
Copy link
Member

ljharb commented Feb 22, 2018

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
Copy link
Contributor

appsforartists commented Feb 22, 2018

@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
Copy link
Author

staltz commented Feb 22, 2018

@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
Copy link
Member

ljharb commented Feb 22, 2018

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
Copy link

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

@rdeforest
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants