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

Should "complete" accept an argument? #77

Closed
benlesh opened this issue Jan 29, 2016 · 49 comments
Closed

Should "complete" accept an argument? #77

benlesh opened this issue Jan 29, 2016 · 49 comments

Comments

@benlesh
Copy link

benlesh commented Jan 29, 2016

I noticed that the README and the spec show a value being passed to complete on the Observer. But that doesn't mesh with any of the examples in the README, and while it's implemented in the impl under src, it's not at all used.

I can't at all figure out a use-case for a completion value that seems valid. We've excluded it from RxJS 5 mostly because it muddies the water for people new to Observable. Generally, they're never going to want to use a completion value.

They don't really "compose" well. What does a merged stream do? drop them? keep the last one? What about zip? An array of completion values? Do I need a selector for completion values as well?

Do we have use-cases or prior art that shows us why a completion value is a good idea?

@erykpiast
Copy link

Does completion value of Observable differ conceptually from value returned by generator?

@benlesh
Copy link
Author

benlesh commented Jan 29, 2016

@erykpiast it doesn't come out of next as it would in a generator/iterator, because each emission is not wrapped in an IteratorResult-like object. Further, the return values from generators don't compose well for similar reasons. Most of the time the return value is thrown out. Using it is almost an anti-pattern. Almost.

@erykpiast
Copy link

I just try to figure out how subjects may work in case when observable complete is only value-less signal. Subject is an observer, like the generator is, and it implies possibility to return a value from it. But it the same time observable part of Subject would not pass this value to its subscribers? Doesn't seem consistent.

@benlesh
Copy link
Author

benlesh commented Jan 29, 2016

But it the same time observable part of Subject would not pass this value to its subscribers

Well, that's the tricky bit: Most things that "subscribe" to a generator/iterator will ignore or drop the return value altogether.

Because most of the time it works like this:

{ value: 1, done: false }
{ value: 2, done: false }
{ value: 3, done: false }
{ done: true }

And some of the time it works like this:

{ value: 1, done: false }
{ value: 2, done: false }
{ value: 3, done: false }
{ value: 4, done: true }

So with something like for(let x of iter) you're going to see 1, 2, 3 and NOT 4.

A further deviation from generator return value is the fact that this is really a Generator going the opposite direction. With a generator, you can push a value in and use it with next, you can push a value in and use it with throw, but with return trying to push a value in doesn't do anything, it just throws it out.

function* myGen() {
  try {
    let n = 0;
    while (true) {
      let value = yield n++;
      // values pushed in via next here
    }
    return 'wee!';  // you can't get a value "out" of return and do something with it.
  } catch (err) {
    // errors pushed in via throw here
  }
}

Since the API of Observable is basically a generator "turned inside out" then complete probably shouldn't have a value.

/cc @jhusain

@zenparsing
Copy link
Member

My point of view is that generators allow you to create sequences that have a return value, which you can see if you iterate manually (instead of using next). In principal, observables should allow you to express that as well. On the other hand, I have seen some confusion about generator return values and I'm not surprised that an argument in the complete signature might cause similar confusion.

@benlesh
Copy link
Author

benlesh commented Jan 29, 2016

I'm not surprised that an argument in the complete signature might cause similar confusion.

It definitely does. Users will try to use it and then be confused about how it composes through combinators. Why was it dropped? When should I use it? etc. The answer to "when should I use it" is "basically, never". Which leads me to believe that Observable is more like the internals of a Generator than it is the externals.

@benjamingr
Copy link

For a data point: coroutines implemented on top of generator use the return value:

function pump(gen){
    const it = gen(); 
    return Promise.resolve().then(function next() {
        let {value, done} = it.next(value);
        if(done) return value; // here
        return Promise.resolve(value).then(next, it.throw.bind(iter));
    });
}

So it definitely adds something to the expressiveness.

@zenparsing
Copy link
Member

I think we should leave the spec algorithm as is, with "complete" (optionally) taking one argument. Any objections before I close this?

@benlesh
Copy link
Author

benlesh commented Mar 2, 2016

I still object, honestly. I don't think it should have a completion value.

For what @benjamingr was saying above, I'm not talking about a generator sending out a value with return, I'm talking about it receiving one... which it can't.

For the "duality" side of it, I think:

Observer Generator
next(value) let value = yield null;
error(err) catch (err) { /* do something */ }
complete(wocka) let wocka = return null;

Clearly, wocka above isn't going to get anything. In fact, that's a SyntaxError, but you get what I'm saying. Observer is the dual of the client side of the coroutine generator.

@zenparsing
Copy link
Member

@benjamingr Still, (for better or worse), in ES the iteration protocol allows us to model a sequence with a completion value, and I don't see how we can ignore that. And you can always ignore the feature without loss if you want.

In any case, I don't mind leaving this open.

@zenparsing zenparsing changed the title completion value? Should complete accept an argument? Mar 2, 2016
@zenparsing zenparsing changed the title Should complete accept an argument? Should "complete" accept an argument? Mar 2, 2016
@benjamingr
Copy link

@zenparsing was that last comment directed at me or @Blesh ?

@Blesh

I'm talking about it receiving one... which it can't.

I'm not sure I understand - you can both send and receive a value to an iterator in its return.

 it.return("SomeValue"); // {done: true, value: "SomeOtherValue"} // done can also be false here

@zenparsing
Copy link
Member

@benjamingr it was directed to @Blesh , sorry : )

@benlesh
Copy link
Author

benlesh commented Mar 2, 2016

@benjamingr I'm of the mind that Observer is the "dual" of the inside of a generator. The inside of the generator can't receive a value via return. You can send a value in through yield via next('foo'), and you can send an error in to a catch block via gen.throw(new Error('bad')) (throw would be error in Observer), but you can't send a value in through return via gen.return('whatever'), you can hit a finally block, but not with a value. At best you'd hit it with the last value you got through the previous yield.

I'm not the expert on duality, perhaps @headinthebox could set me straight.

EDIT: I'm crossing out "dual" I don't mean "dual", it's more of the mirrored behavior.

@benlesh
Copy link
Author

benlesh commented Mar 2, 2016

I don't know... I can kind of see the other side of it too, @zenparsing ... It's just that I'd almost label it an anti-pattern to even use the completion value for anything. If you have a final value, it should arrive via next(), or it should maybe be aggregated and emitted in a different observable.

My two biggest issues in a nutshell:

  1. I think the use-cases are dubious.
  2. I think the complexity it adds to even the simplest of operators is telling. What do I do with the completion value in merge? how about concat? etc. What about map? Do I need a special mapComplete operator for it?

@zenparsing
Copy link
Member

@Blesh I think the same applies to returning a value from a generator, and I think the general answer is basically the same: just don't use it. But even so, I don't think that we want to make observable sequences less powerful than generator sequences.

@benlesh
Copy link
Author

benlesh commented Mar 3, 2016

make less powerful? or make less foot-gunny?

@zenparsing
Copy link
Member

make less foot-gunny?

You mean like having Observable.from/of synchronously emit? :P

Point taken though. I'd be fine changing it depending on more feedback, but I'm going to leave it alone for now.

@benlesh
Copy link
Author

benlesh commented Mar 4, 2016

You mean like having Observable.from/of synchronously emit? :P

I have a way I'd like to have Observable.from/of in RxJS 5 emit async by default, and be able to opt-in to synchronous emission... but it's been a hard sell to the community, which is the only reason it isn't in.

@zenparsing
Copy link
Member

That would be 🤘

@Artazor
Copy link

Artazor commented Mar 27, 2016

I think, it's inconsistent to have a completeValue in current design.

I could imagine other world where Observer/Observable are defined like:

type Result<T> = T | PromiseLike<T>;

interface Observer<T, S, R> {
       next(item: T): Result<S>; // Observable will await on the result of `next`
       error(e: any): void;
       complete(value: R);
}

declare class Observable<T> {
       constructor(
             subscribe: <S, R>(
                   observer: Observer<T, S, R>,
                   // every implementer should take into account that 
                   // on subscribe async reducer and initial value may be specified
                   // and should use them to calculate the completeValue
                   // and send it to the observer.complete
                   reducer: (acc: R, item: S) => Result<R>,   
                   initialValue: Result<R>
             ) => void | Function,
       );

       subscribe<S, R>(
            observer: Observer<T, S, R>, 
            reducer: (acc: R, item: S) => Result<R>
            initialValue: Result<R>
       ): Subscription;

       filter(fn: (item: T) => boolean): Observable<T>;
       map<U>(fn: (item: T) => U): Observable<U>;

       forEach<S, R>(
            loop: (item: T) => S,
            reducer: (acc: R, item S) => Result<R>,
            initialValue: Result<R>
       ): Promise<R>

}

This design is much more powerful than the current design. But I think it is too late... (?)
Thus, no completeValue in complete();

@benjamingr
Copy link

How is that even remotely more powerful?

@Artazor
Copy link

Artazor commented Mar 27, 2016

@benjamingr it allows smoothly interchange asynchronicity between producer and consumer (without overwhelming the consumer with values). Here consumer can specify the algorithm for composing (reducing) the final value, and if this algorithm is asynchronous, then Observable will not emit any further .next() until the reduction step is performed.

(I'll provide an example below)

@Artazor
Copy link

Artazor commented Mar 27, 2016

@benjamingr Here is an example:
(it is not intended to be super-optimal, I've preferred a code clarity)

function fromArray(arr) {
      return new Observable((observer, reducer, value) => {
             let cancelled = false;
             async function run() {
                  for (let i = 0, n = arr.length; !cancelled && (i < n) ; i++) {
                      value = reducer(await value, await observer.next(arr[i]));
                  }
                  return await value;
             }
             run().then(::observer.complete, ::observer.error);
             return () => cancelled = true;
      });
}

async function apiCallSample(value)  {
     await Promise.delay(Math.random()*1000);
     return value + 1;
}

async function usage() {
      let list = [0, 1, 2];
      let result = await fromArray(list).forEach(
             async (value) => {
                     let x = await apiCallSample(value);
                     return x * 2;
             },  (a, item) => a.concat(item), []
      )
      console.log(result) // [2, 6, 4] in exactly same order 
}

Do you understand what I'm trying to achieve?

@Artazor
Copy link

Artazor commented Mar 27, 2016

By the way, I strongly believe, that in better world types for iterators are discriminated unions:

interface Iterator<T,S,R> {
     next(pass: T): { done: false, value: S } | { done: true, value: R }
}

It means that the final result has distinct semantics from the yielded intermediate results.

function *progressive<T>(): Iterator<T, number, T[]> {
       var result = [];
       for(var i = 0; i < 10; i++) {
           result.push(yield i);
       }
       return result;
}    

@zenparsing
Copy link
Member

Javascript has no static typing

@Artazor
Copy link

Artazor commented Mar 27, 2016

@zenparsing surely I know!
I've used types only for semantics annotation to denote what to be expected from the implementation.

@Artazor
Copy link

Artazor commented Mar 27, 2016

The following text may look like an offtopic, however it is deeply connected with the completion problem.

Current design of Observable implies that the only source of the asynchronicity is the Producer. That is not true in general case: both the Producer and Consumer can be asynchronous. Consumer can asynchronously block Producer.

Imagine the following (unsupported) case from withdrawn proposal:

async function apiCall(i) {
      await Promise.delay(Math.random()*1000);
      return i*2;
}

async function *progressive() {  // UNSUPPORTED
       let result = [];
       for(let i = 0; i < 10; i++) {
           var x = await yield i; // Async backflow from the consumer (!)
           var y = await apiCall(x); // Async in generator;
           result.push(y); 
       }
       return result;
} 

async function usage() {
    let gen = progressive();
    let res = await gen.next();
    while (!res.done) {
       var i = res.value;
       res = await gen.next(apiCall(i))
    }
    return res.value; // [0,4,8,12,16,20,24,28,32,36]
}

This is what I call true async generators.
@zenparsing, @benjamingr Could you express this using Observable?

@jhusain I would enhance your proposal with an ability to yield back to the async generator with yield on clause (only one yield on is allowed per for...on)

async function usage() {
    let res = await for(let i on progressive()) {
          await yield on apiCall(i); 
    }
    return res // [0,4,8,12,16,20,24,28,32,36]
}

@Artazor
Copy link

Artazor commented Mar 27, 2016

Hmmm... sorry. Looks like that blocking consumer is bad idea.

@zenparsing
Copy link
Member

@Artazor Check out the async iterator/generator proposal: https://github.com/tc39/proposal-async-iteration

Is that what you're trying to express?

@Artazor
Copy link

Artazor commented Mar 28, 2016

@zenparsing - exactly!

@dead-claudia
Copy link

I'll note that I've taken advantage of generator return values for a byte-emitting streaming HTML generator, where I needed to keep track of whether a child element emitted children (for SVG/MathML). It basically translated into a bunch of return yield* generate() and if (yield* generate()) hasChildren = true. I haven't yet found a use case for observables, but the above is an example of where it's useful for generators.

@jhusain
Copy link
Collaborator

jhusain commented Nov 30, 2016

The dominoes continue to fall. In light of #119, I think we need to reconsider the completion value. If #119 is accepted, we have made Observable sequences less expressive than generators. Under the circumstances I want to revisit the completion value - particularly because I have no idea how operators are supposed to handle it. As soon as we go through a fan out and collapse operator like flatten, it's not clear how to handle completion values. Should operators like flatten accept a reducer? Seems like it's simpler to just have a last() operator on Observable, and get rid of the completion value.

@jhusain
Copy link
Collaborator

jhusain commented Nov 30, 2016

More briefly, given that the whole point of Observable is to enable composition, what good is a return value if it gets lost during composition?

@jhusain
Copy link
Collaborator

jhusain commented Nov 30, 2016

Worth noting that the proposal for iterator combinators seems to do nothing with the return value: https://github.com/leebyron/ecmascript-iterator-hof @leebyron

@zenparsing
Copy link
Member

If #119 is accepted, we have made Observable sequences less expressive than generators.

Because we would only allow one-way data flow? Yes, certainly.

Another point of view is that the completion value isn't necessarily about making observables as powerful as generators, it's more about making making sure that observable (push-only) sequences can represent any (pull-only) iterable sequences. There might be some value to that, even if both iterator and observable combinators (and for-of) just drop the completion value.

It would seem to be a nice property if we could represent a sequence like the following with an observable:

function* g() {
  yield 1;
  yield 2;
  return 3;
}

But the practical value is probably pretty minimal.

Is there any harm in allowing it?

@acutmore
Copy link

The complications a completion value add to composition sound like a very solid reason against them.

If there are use cases for observables with a final value they could be fulfilled by wrapping up the values sent to next in an iterator style object.

{ 
  value: T;
  done: Boolean
}

An operator could ensure that the stream is unsubscribed after receiving a value where done is true.

@zenparsing
Copy link
Member

Alternatively, there is no benefit in JS to disallowing a completion value. If you don't want to use it, don't use it.

@jhusain
Copy link
Collaborator

jhusain commented Dec 22, 2016 via email

@zenparsing
Copy link
Member

This isn't a "feature". This only concerns whether the first argument is allowed to pass from the SubscriptionObserver to the Observer, and it should be allowed so that we have parity with iterator.

@jhusain
Copy link
Collaborator

jhusain commented Dec 22, 2016 via email

@benlesh
Copy link
Author

benlesh commented Dec 22, 2016

It would seem to be a nice property if we could represent a sequence like the following with an observable:

function* g() {
  yield 1;
  yield 2;
  return 3;
}

const source$ = new Observable(observer => {
  const _g = g();
  while (true) {
    const { value, done } = _g.next();
    observer.next(value);
    if (done) {
      observer.complete();
      break;
    }
  }
})

Seems fine. Just like you have to when you consume an iterator with this { value: importantThing, done: true } semantic, you end up having to check for the last value. (You know, it's not as easy as a for..of when you're dealing with that completion value from an iterator any more, similarly with an observable where the last value is also different). It's strangely congruent in that manner.

source$.subscribe({
  last: null,
  next(x) { this.last = x; },
  complete { handleLast(this.last); }
});

If for some reason that isn't your thing, you can still make an Observable of IteratorResult to model your iterator:

const source$ = new Observable(observer => {
  const _g = g();
  while (true) {
    const result = _g.next();
    observer.next(result});
    if (result.done) {
      observer.complete();
      break;
    }
  }
})

There are advantages and disadvantages to both of these approaches, but they're both valid. The nicest thing about them though is they'll compose cleanly with any operator, where with some operators, flatMap (aka mergeMap) for example, it's not readily apparent what to do with the completion values. Combine them? Just take the last one? From the parent? from the children? from whichever is the last one to complete? It's just hard to design around that. Most likely I'd drop it for most operators in RxJS.

@zenparsing
Copy link
Member

@Blesh that is not a lossless transformation. It doesn't seem fine to me.

@benlesh
Copy link
Author

benlesh commented Dec 22, 2016

@zenparsing honestly, the second example is more on-par with JavaScript iterators. Since JavaScript iterators are synchronously bound between value and completion, you'd have to pass both values through next in order to really model it with an Observable.

Granted that makes consuming them slightly more cumbersome, but hey, consuming Iterators with completion values is cumbersome in JavaScript as well (for..of and Array.from don't work). This is all okay because using a completion value in either an Iterator or Observable should really be an edge case anyhow. Making it more ergonomic in Observable would just encourage people to try to leverage it more, and I don't think that's going to have good outcomes. I think that's just going to confuse users of combinator libraries even more than they're already confused.

This is an old argument from me, though, haha. I'm against the completion value because it's very edge casey, and I don't think it buys anyone anything meaningful. I'm actually against it's use in generators and iterables, too, but that ship has sailed.

@jordalgo
Copy link

@zenparsing - Is it incorrect to say that if completion values were removed from the Observable spec and the JS community really found a use for them in the future that they could be added in a later version and this wouldn't really hurt backward compatibility as pretty much all operators (I can think of) wouldn't handle this value anyway? It seems like if they were in the spec, it would be nearly impossible to remove them later.

@benlesh
Copy link
Author

benlesh commented Dec 22, 2016

As for modeling Observable after Generators. I feel like that's iffy.

Observables are used as functional bridges between an end observer (the consumer) and a source producer (usually some form of subject). They're basically a compositional piece added to the GoF Observer Pattern. So instead of Subject -> observer, you end up with Subject -> mapobserver -> filterobserver -> observer etc. Where the middle two observers are actually set up by an Observable, which acts as a template to compose a chain of observation. Generators on the other hand seem more like the "head" of the chain sort of concept. Generators usually have some sort of state as well, where Observables usually do not have any state.

In particular when it comes to the coroutine use case of Generators (which demand bi-directionality). Coroutines also rely on a behavior where the consumer synchronously requests data from the producer while passing the producer new data. Moreover, with the generator coroutines, the consumer generally has some idea what the temporal nature of the data returned from the producer is. When the roles are reversed in a push-based scenario, like with an Observable capable of coroutines, this becomes difficult. The producer can't, or at least shouldn't, know what temporal shape to expect back from the consumer. That means that calls to Observer.prototype.next would probably have to return a promise to a value in order for the coroutine aspect of observables to compose at all.

It's interesting, I suppose, as a type. I just don't think it's practical or that it fits in most people's applications.

It seems like an async type that was the "dual" of a generator, would just be an "async generator" because of the bidirectional nature of generators. (I haven't kept up on the async iterator proposal) But it seems like coroutines in both directions would be built over promises, and that would compose better.

Similarly, if you wanted a synchronous reverse of a generator, it would be a generator again.

I'm no @headinthebox, but I feel like the "dual" of fn(v1): v2 would be fn(v2): v1. (or in generator's case const v2 = yield v1 vs const v1 = yield v2). Therefor the dual of something bidirectional in this manner would basically be the same structure as itself.

I don't think we can model Observable off of generators. I mean we can try. I just feel like we're going to hit some problems with the math of the thing.

@staltz
Copy link

staltz commented Dec 22, 2016

They're basically a compositional piece added to the GoF Observer Pattern. So instead of Subject -> observer, you end up with Subject -> mapobserver -> filterobserver -> observer etc. Where the middle two observers are actually set up by an Observable, which acts as a template to compose a chain of observation.

Beautiful explanation.

@benjamingr
Copy link

@Blesh to sum it up: observable is the dual of iterable - not of generator. I thought we were past "model observer/observable after generator's dual" a year ago. It's worth mentioning that in LINQ - observables are duals of enumerables and the duality works. It you take just that interface and dualize it you do get observables.

While duality is beautiful - I think what most users really need is compositional push streams. From what I understand that's the direction this proposal has taken and that's the direction Rx variants in languages other than C# have (rightfully) taken too. It's a bit silly to talk about duality in the context of Rx when JS iterables don't have a map operator :)

@benlesh
Copy link
Author

benlesh commented Dec 23, 2016

It's a bit silly to talk about duality in the context of Rx when JS iterables don't have a map operator :)

Not really, you can create user-land compositional operators for JS iterables because of their implementation. I'd hope for the same sort of outcome with JS observables. It's a huge point of the power of this type.

@jhusain
Copy link
Collaborator

jhusain commented Dec 31, 2016

With the conclusion of this issue (#119 (comment)), Observable will now suppress the completion value of observer notifications. In other words, Observable do not attempt to replicate generator semantics anymore, but rather the narrower Iterable semantics instead. Iterable semantics are easy to define: iteration semantics are those semantics required to support for...of. The completion value maps to the return value of the generator - which for...of always drops. Therefore the removal of the completion value is consistent with the decision to strictly match Iterable semantics.

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

10 participants