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

Motivate subscribe vs forEach #20

Closed
benjamingr opened this issue May 31, 2015 · 19 comments
Closed

Motivate subscribe vs forEach #20

benjamingr opened this issue May 31, 2015 · 19 comments

Comments

@benjamingr
Copy link

Hey, can we get an example motivating why both subscribe and forEach are needed?

@jhusain
Copy link
Collaborator

jhusain commented Jun 2, 2015

Async/Await Observation.

async function getPriceHigh(stock) {
    let delta,
        high = Number.MIN_VALUE;

    let socket = new WebSocket(/dailyPrices/" + stock);
    await Observable.from(socket).forEach(price => {
          if (price > high) {
          high = price;
      }
    });

    return high;
}

@zenparsing
Copy link
Member

An interesting question on the table is if we could merge subscribe and forEach by making the subscription object a promise (or thenable).

So in the example, you could just replace the forEach with subscribe and it would work fine:

await Observable.from(socket).subscribe(price => {
  if (price > high) {
    high = price;
  }
});

It would be a nice simplification, all other things being equal.

@benjamingr
Copy link
Author

Yes, this definitely seems like a good idea to merge the two - there is nothing preventing subscribe from being a thenable/promise.

@jhusain
Copy link
Collaborator

jhusain commented Jun 2, 2015

Not sure why this is attractive. Promise essentially duplicates semantics of return() and throw() callbacks passed to subscribe. Would also like to understand performance implications.

@jhusain
Copy link
Collaborator

jhusain commented Jun 2, 2015

what a happens if I do this:

try {
await obs.subscribe({ throw(e) { /* error handling / }, return(v) { / error handling */ } })
}
catch(e) {
// other error handling
}

Gives the developer of the opportunity to duplicate error handling logic two places. furthermore the Observer callbacks may or may not be executed synchronously, whereas the promise error will only be executed asynchronously. This seems like a big foot gun to me.

@jhusain
Copy link
Collaborator

jhusain commented Jun 2, 2015

Big mistake to build observable with promises. There is too much semantic overlap, coupled with impedance mismatch. adapting between them is unambiguous.

@domenic
Copy link
Member

domenic commented Jun 2, 2015

what a happens if I do this:

Yes, agreed. It seems this proposal could be greatly simplified by removing subscribe and [Symbol.observe] in favor of forEach---at least, assuming we get cancelable promises. With that assumption there's no loss in power and a great increase in interoperability and compositionality.

@zenparsing
Copy link
Member

I agree as well and withdraw the suggestion : )

@jhusain
Copy link
Collaborator

jhusain commented Jun 2, 2015

Withdrawing Symbol.observe and subscribe on the assumption that Cancellable
Promises arrive would couple these two proposals. Seems ill-advised. However having
forEach around means that we could simplify the proposal at a later stage
if Cancellable Promises gains traction.

On Tue, Jun 2, 2015 at 1:35 PM, zenparsing notifications@github.com wrote:

I agree as well and withdraw the suggestion : )


Reply to this email directly or view it on GitHub
#20 (comment)
.

@domenic
Copy link
Member

domenic commented Jun 2, 2015

Yeah, agreed. I like the plan of keeping both for now in the hope of removing one if we get cancellable promises shipping soon. Nice.

I still think having both [Symbol.observe] and subscribe is a bit redundant but I haven't dug into it much and we can move that to another bug.

@zenparsing
Copy link
Member

Any objections to closing this one?

@benjamingr
Copy link
Author

Resolution sounds good to me 👍

@benjamingr
Copy link
Author

Re-reading this, why would subscribe returning a thenable mean we'd have to have cancellable promises in order for observers to be cancellable?

Can't subscribe return a thenable/Promise which is also a subscription and we get to have the best of both worlds?

@benlesh
Copy link

benlesh commented Jun 30, 2015

why would subscribe returning a thenable mean we'd have to have cancellable promises in order for observers to be cancellable?

@benjamingr because subscribe should return a mechanism for notifying the producer to stop pushing values (unsubscription/cancellation)

@benjamingr
Copy link
Author

Yes, but why not both? Why can't it have then and unsubscribe

@benlesh
Copy link

benlesh commented Jun 30, 2015

Because at that point someone will argue that it should be a cancelable promise, which marries the two proposals. I don't think cancelable promises are a great idea, personally.

@benjamingr
Copy link
Author

Because at that point someone will argue that it should be a cancelable promise, which marries the two proposals. I don't think cancelable promises are a great idea, personally.

I agree (with not marrying the proposals). What I'm asking is - why can't we return a promise which is a subscription that can be disposed?

That way if in the future cancellable promises make it we'll also make the returned promise cancellable to benefit from facilities but we get a subscription with a then and an unsubscribe which means we can dispose it and await it. Wouldn't that be worth it?

@benlesh
Copy link

benlesh commented Jul 1, 2015

@benjamingr at this point I honestly think the subscribe function should return a generator type that also has the dispose method that is sorely missing from Generator. If there were a proposal to marry it to, it would be the proposal of putting a dispose method on Generator that only called the generator's finally block. cc/@jhusain

@benjamingr
Copy link
Author

The generator subscription proposal is starting to grow on me.

Since it is now a sequence, I wonder if await* can be used instead of await, this also has the nice property of showing that it's in fact a sequence that's being awaited.

It would require next on the returned subscription, that would have to return promises somehow in order to work with await* though. Do you think a new thread should be opened?

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

5 participants