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

Observable should be a thenable #15

Closed
jhusain opened this issue May 29, 2015 · 58 comments
Closed

Observable should be a thenable #15

jhusain opened this issue May 29, 2015 · 58 comments

Comments

@jhusain
Copy link
Collaborator

jhusain commented May 29, 2015

This was an oversight. We should definitely spec this so that async await works on Observables out of the box.

the return value of the Observable will be come the resolution of the promise. Likewise errors become rejections.

@zenparsing
Copy link
Member

I considered this, but was concerned that someone might want to actually deliver an observable asynchronously through a promise. In other words, Promise<Observable>. If we give observables a "then" method, then we effectively can't have Promise<Observable>.

let promise = doSomethingAsync().then(_=> {
    return new Observable(sink => {
        sink.return(1);
    });
});

promise.then(x => console.log(x)); // "1", not an observable

I guess the question is, do we want to disallow Promise<Observable>, in exchange for directly working with async/await?

@stefanpenner
Copy link
Contributor

@jhusain what is the fulfillment value of the then. If this where to happen, we should avoid hilarious cycles, where the fulfillment is actually the thenable observable.

@jhusain
Copy link
Collaborator Author

jhusain commented May 29, 2015

The value returned to the Generator's return method.

JH

On May 29, 2015, at 10:44 AM, Stefan Penner notifications@github.com wrote:

@jhusain what is the fulfillment value of the then


Reply to this email directly or view it on GitHub.

@stefanpenner
Copy link
Contributor

The value returned to the Generator's return method.

sounds reasonable

@benjamingr
Copy link

Yes, when I suggested this in the composition functions thread we also considered a special symbol instead of making it a thenable - so that should also be considered.

I guess the question is, do we want to disallow Promise, in exchange for directly working with async/await?

I can definitely think of situations where you'd want a Promise<Observable> but in all honesty those can be solved with Promise<() => Observable> which works pretty well.

@zenparsing
Copy link
Member

Thinking out loud: what if the subscription were either a "thenable" or a promise subclass? If we allowed an empty argument list to "subscribe", we'd have:

async function af() {
    let observable = getObservableFromSomewhere();
    let answer = await observable.subscribe();
    console.log(answer);
}

Thenable assimilation is kind of weird, so the suggestion would be making subscriptions a subclass of promise:

class Subscription extends Promise {
    unsubscribe() : undefined;
}

Essentially a cancellable promise, with well-defined and reasonable semantics. : )

@zenparsing
Copy link
Member

If that suggestion works out, then it could potentially remove the need for a separate "forEach" and the need for me to think of a response to #20 : )

@domenic
Copy link
Member

domenic commented Jun 1, 2015

We'll hopefully be having full-on cancellable promises sooner rather than later (driven by fetch, so, I'm talking in terms of low-single-digit browser-releases). It might be reasonable to block observable unsubscription semantics until they ship.

@zenparsing
Copy link
Member

@domenic are the conversations surrounding cancellation happening somewhere? It would probably be a good idea for me to at least listen.

@domenic
Copy link
Member

domenic commented Jun 1, 2015

@zenparsing still booting up, maybe watch https://github.com/domenic/cancelable-promise but it's pretty empty right now.

@benjamingr
Copy link

@zenparsing the public conversation about cancellable promises got really noisy really fast. If you want to see an example of similar semantics you can see the bluebird 3.0 cancellation semantics - they're not the same but they're close: http://bluebirdjs.com/docs/api/cancellation.html

If you're used to Rx semantics this is similar to refCounted dispose.

Subscription as a thenable or a promise subclass sounds very reasonable - doesn't really matter which but I definitely see the merit in a subclass since it gets all the other useful methods as well as ones added to the API in the future.

@stefanpenner
Copy link
Contributor

Essentially a cancellable promise, with well-defined and reasonable semantics. : )

I think cancel and unsubscribe are different (although maybe overlapping)

To me:

  • cancel implies aborting the work that would otherwise realize the eventual value
  • unsubscribe implies remove me as a follower – which may have further side-affects (like disposing on refCount === 0)

cancel on Promises is actually much trickier to do safely then unsubscribe

For example, given:

Promise.all([
  fetch('/user/1'),
  fetch('/user/2'),
  Promise.reject(new Error('...'))
])

We could augment Promise.{all,race} to unsubscribe from fetch('/user/1') and fetch('/user/2') as the final promise is detected to reject. This wouldn't cause fetch to be cancelled, but it would eagerly release, which has some benefits.

I believe what this means, is cancel is a superset of unsubscribe.

@domenic
Copy link
Member

domenic commented Jun 1, 2015

This is going to turn into another promise cancellation thread, I can tell :(

@jhusain
Copy link
Collaborator Author

jhusain commented Jun 1, 2015

Agree with Stefan. Although I agree it would be a nice unification, Cancellation and unsubscription are semantically different from one another. They provide different guarantees.

JH

On Jun 1, 2015, at 8:28 AM, Stefan Penner notifications@github.com wrote:

Essentially a cancellable promise, with well-defined and reasonable semantics. : )

I think cancel and unsubscribe are different (although maybe overlapping)

To me,:

cancel implies aborting the work that would otherwise realize the eventual value
unsubscribe implies remove me as a follower.
cancel is actually much trickier to do safely then unsubscribe

For example, given:

Promise.all([
fetch('/user/1'),
fetch('/user/2'),
Promise.reject(new Error('...'))
])
We could augment Promise.{all,race} to unsubscribe from fetch('/user/1') and fetch('/user/2') as the final promise is detected to reject. This wouldn't cause fetch to be cancelled, but it would eagerly release.


Reply to this email directly or view it on GitHub.

@zenparsing
Copy link
Member

@jhusain What do you think about the general idea of making the subscription object thenable or a promise subclass, rather than the observable?

@benjamingr
Copy link

This is going to turn into another promise cancellation thread, I can tell :(

Not a chance.

Agree with Stefan. Although I agree it would be a nice unification, Cancellation and unsubscription are semantically different from one another. They provide different guarantees.

For what it's worth, some promise libraries have a notion of disposers (http://bluebirdjs.com/docs/api/resource-management.html). Observable dispose is in-between promise cancellation and promise dispose, with the big difference that an observable is lazy and disposing is done on the subscription itself.

@zenparsing

@jhusain What do you think about the general idea of making the subscription object thenable or a promise subclass, rather than the observable?

I think it would be very nice to be able to await an observable itself without needing a subscription, this might be a weak argument but if we force everyone to .subscribe it won't be fun for users. It's also entirely possible to do both (the observable is thenable and the subscription is a promise subclass).

It's also worth mentioning that if we're discussing async/await it might be nice to add semantics that allow hooking on a single value basically acting like async iterators (that is, a thenable .next for the next value of a subscription rather than the whole sequence) although backpressure issues arise.

@jhusain
Copy link
Collaborator Author

jhusain commented Jun 1, 2015

Agree with Benjamin that there is an ergonomic issue with awaiting an Observable by first calling subscribe. I hadn't considered the pain of this:

await obs.subscribe();

I think users will expect to await Observables directly as they do in .NET. I also think this expectation would be correct, because thening an Observable is a subscription. You're kicking off the side effects and waiting for the result. The tradeoff is that you cannot cancel as long as Promises remain uncancellable. However a future proposal manages to fix that then we'll have the best of all worlds.

JH

On Jun 1, 2015, at 8:56 AM, Benjamin Gruenbaum notifications@github.com wrote:

This is going to turn into another promise cancellation thread, I can tell :(

Not a chance.

Agree with Stefan. Although I agree it would be a nice unification, Cancellation and unsubscription are semantically different from one another. They provide different guarantees.

For what it's worth, some promise libraries have a notion of disposers (http://bluebirdjs.com/docs/api/resource-management.html). Observable dispose is in-between promise cancellation and promise dispose, with the big difference that an observable is lazy and disposing is done on the subscription itself.

@zenparsing

@jhusain What do you think about the general idea of making the subscription object thenable or a promise subclass, rather than the observable?

I think it would be very nice to be able to await an observable itself without needing a subscription, this might be a weak argument but if we force everyone to .subscribe it won't be fun for users. It's also entirely possible to do both (the observable is thenable and the subscription is a promise subclass).

It's also worth mentioning that if we're discussing async/await it might be nice to add semantics that allow hooking on a single value basically acting like async iterators (that is, a thenable .next for the next value of a subscription rather than the whole sequence) although backpressure issues arise.


Reply to this email directly or view it on GitHub.

@zenparsing
Copy link
Member

Do we normally think of "await" as kicking off side effects?

@domenic
Copy link
Member

domenic commented Jun 1, 2015

No, await does not kick off side effects normally. E.g. in await doSomething(), it's the doSomething() part that has side effects.

@benjamingr
Copy link

I think "kicking off the side effects" is probably better worded as "kick off the operation" in case of a lazy construct. As Domenic said when awaiting eager constructs (like promises) the () function call is what's calling the operation. You can await p for an existing promise which should not cause any side effects at all.

@domenic
Copy link
Member

domenic commented Jun 1, 2015

If observables were not lazy, then await observable would make sense. But assuming you want to keep them that way (i.e. as a function-class, see #11), then I think you need to do await observable.subscribe(), or perhaps better yet (again, #11) await observable().

@benjamingr
Copy link

In essence both in await promise and await observable all that happens is a subscription is being added - the difference is that in the promise case that subscription forces the observable to start working since it's a lazy construct.

I'm not sure I understand what's the problem with await observable causing a side effect (where await promise does not). Are you arguing that the fact that an operation happens when you await observable in the background is confusing to developers or bad semantics?

@domenic
Copy link
Member

domenic commented Jun 1, 2015

I guess it seems like both to me. A type conversion (what happens when you do await thenable) should not cause side effects---bad semantics. And awaiting observables causing side effects while awaiting promises does not is confusing to developers.

@benlesh
Copy link

benlesh commented Jun 1, 2015

Is it going to be clear that await myObservable cannot be cancelled?

it seems like adding this so observable can be used with await isn't a good idea, if it ends up being an anti-pattern when it's used. i.e. "Yes, you can use await myObservable, but you shouldn't because then you can't cancel it". Then again, this might be true for for..on syntax, and the forEach() method as well?

I think I'm with @domenic here in that you should have to call a method that returns a thennable object. Perhaps the subscription (generator) object that is returned from the subscribe method should be thennable. That way you can unsubscribe, and using await doesn't necessarily become an anti-pattern:

var subscription = myObservable.subscribe();
var returnValue = await subscription;
subscription.error('cancel with an error');

As I stated above, you could already use Observable.prototype.forEach() for the same thing, but I think using forEach is generally going to be bad practice, because of the lack of cancellation semantics.

@benlesh
Copy link

benlesh commented Jun 1, 2015

I guess the shorter version of what I'm saying is: Encouraging use of Observable in any way that doesn't allow you to cancel it seems like encouraging bad practice.

I can totally be convinced otherwise, I guess.

@benjamingr
Copy link

That's an interesting point @Blesh about cancellation.

There is a great deal of things you don't want to or need to cancel in server-side code, typically DB requests for example, or web requests (on the server, not the client) and so on.

In fact I've found that in the vast majority of the times I need cancellation I need concurrency anyway since the reason I'm cancelling is because what I need became irrelevant - in which case await isn't something I could use anyway.

(All that said, it's totally possible to do cancellation with await using .return which can in turn cancel the subscription it created - just have then return a cancellable promise - afaik those are the semantics anyway.)


I also tend to agree with Domenic, we need to think of a way to make the syntax less confusing to people and still short - an observable() instead of observable.subscribe() would already be a big ergonomic improvement but as we noted we can't do that (not unless we have function classes, and given how Jafar coined that term a few threads away and there is no proposal yet - those are not coming to ES any time soon ;))

I think in the meantime awaiting subscriptions which become thenable or subclasses of promise is the best we can do right now. This also unifies forEach and subscribe.

@domenic
Copy link
Member

domenic commented Jun 1, 2015

(not unless we have function classes, and given how Jafar coined that term a few threads away and there is no proposal yet - those are not coming to ES any time soon ;))

I think they are likely to come to ES exactly as fast as observable is likely to come to ES.

@stefanpenner
Copy link
Contributor

in which case await isn't something I could use anyway.

just had a thought, what about:

try {
  let something = await somethingAwaitable;
} catch (e) {
  // cancel/unsubscribe somethingAvaitable ?
}

and:

var subscription = myObservable.subscribe();
var returnValue = await subscription;
subscription.error('cancel with an error');

try {
  var subscription = myObservable.subscribe();
  var returnValue = await subscription;
} catch (e) {
  // would e === 'cancel with an error' ?
}

subscription.error('cancel with an error');

@benjamingr
Copy link

@stefanpenner

try {
  let something = await somethingAwaitable;
} catch (e) {
  // cancel/unsubscribe somethingAvaitable ?
}

I'm going to need a more concrete example to say how I would address it.

@stefanpenner
Copy link
Contributor


this just auto-unsubscribes on failure thanks to:
1. https://github.com/zenparsing/es-observable/blob/dd15829cb757dd8559101745935883e499798279/src/Observable.js#L180
2. https://github.com/zenparsing/es-observable/blob/dd15829cb757dd8559101745935883e499798279/src/Observable.js#L83

@stefanpenner
Copy link
Contributor

@stefanpenner right, but how do I manually cancel an Observable that is being used with await? If I'm awaiting some AJAX call, and I"m about to change my view to something that doesn't care about it, what do I do if I've used await?

@Blesh I think we need a real example. This would help the conversation.

I believe the concern is the current work of promises, no back-propagation of un-interest exists. So as soon as you insert a promise at a "joint", you lose the ability to declare the lack of interest. Thereby losing the fidelity we have raw observables. That is, without introducing some side-channel.

@benlesh
Copy link

benlesh commented Jun 1, 2015

@stefanpenner A real example would be where you have an AJAX request wrapped in an observable, you kick if off when you're loading your view in a client-side app, then the user navigates to another view before the observable returns, and you want to cancel the observable. In that case, using myObservable.forEach() or await myObservable would be bad, because they're not at all cancellable. The only way you can get cancellation magic is with subscribe or @@observe.

Another would be the typical "autocomplete" text box, where you're getting some search results based on the last value entered, if the user hits a new key in the text box before the previous query returns, you want to cancel the previous query and await the next one.

@benjamingr
Copy link

In concurrent cases and combinations await shouldn't be used (the autocomplete example), await is only for sequencing things one after the other. I'm all for a flatMapLatest and I'm not sure how this can be expressed without clever combinators (or implementing them) - let alone with await.

As for ajax wrapped request - if the thenable returned from an observer is cancellable, the async function will call cancel on it when it is itself cancelled (I think?) in which case the loading-your-view example will work.

@benlesh
Copy link

benlesh commented Jun 1, 2015

@benjamingr @stefanpenner to clarify, I'm making the case that:

await myObservable;

...is not cancellable, because I've gotten no reference to anything I can call return() or error() on to cancel the subscription it creates.

Therefor, I think it is more desirable that whatever is returned from subscribe be "then-able". Likewise, I think whatever is returned from forEach should be cancellable (and perhaps the same thing?).

@domenic
Copy link
Member

domenic commented Jun 1, 2015

I agree cancelable promises are an important building block here.

(and perhaps the same thing?).

Yes, currently this proposal contains three methods all of which basically do the same thing (subscribe, [Symbol.observer], and forEach). In general any argument that applies to one of them applies to all. Although there is probably some room for trimming the fat.

@benlesh
Copy link

benlesh commented Jun 1, 2015

@domenic but is it a cancellable promise or a then-able subscription that is necessary here? I'd argue the latter, since having a next() method on the returned type also has value.

@domenic
Copy link
Member

domenic commented Jun 1, 2015

Having a next() method on the return type is explicitly not part of this proposal (and never was).

@benjamingr
Copy link

Therefor, I think it is more desirable that whatever is returned from subscribe be "then-able". Likewise, I think whatever is returned from forEach should be cancellable (and perhaps the same thing?).

I think you should be able to .then .subscribe, the question is whether or not you should also be able to .then the observable itself is what we're discussing at the moment. Mainly, whether or not if not having to type .subscribe() over and over is worth not being 100% logically correct, or being confusing to JavaScript developers (see Dom's points above).

...is not cancellable, because I've gotten no reference to anything I can call return() or error() on to cancel the subscription it creates.

You're not the one that is cancelling it though, when the async function gets cancelled the inner thenable gets cancelled too which would in turn unsubscribe from the observable.

@benlesh
Copy link

benlesh commented Jun 1, 2015

@domenic ... ah, you're right it was never part of this proposal. I keep getting this confused with the original spec that this was modeled after, in which subscribe returned a generator.

@zenparsing
Copy link
Member

@Blesh We'll get that repo updated soon.

@jhusain
Copy link
Collaborator Author

jhusain commented Jun 1, 2015

It's worth mentioning that await has never guaranteed no side effects. Promise.resolve calls "then" on non-Promises and there's nothing about the thenable contract that guarantees no side-effects. I think developers think about await in terms of sequencing ("then" is pretty appropriate here). The definition of "then" as a subscribe on an Observable seems pretty intuitive to me. It's worth pointing out that in .NET you can await an Observable. Developers seem to be able to intuit the behavior.

JH

On Jun 1, 2015, at 2:39 PM, Benjamin Gruenbaum notifications@github.com wrote:

Therefor, I think it is more desirable that whatever is returned from subscribe be "then-able". Likewise, I think whatever is returned from forEach should be cancellable (and perhaps the same thing?).

I think you should be able to .then .subscribe, the question is whether or not you should also be able to .then the observable itself is what we're discussing at the moment. Mainly, whether or not if not having to type .subscribe() over and over is worth not being 100% logically correct, or being confusing to JavaScript developers (see Dom's points above).

...is not cancellable, because I've gotten no reference to anything I can call return() or error() on to cancel the subscription it creates.

You're not the one that is cancelling it though, when the async function gets cancelled the inner thenable gets cancelled too which would in turn unsubscribe from the observable.


Reply to this email directly or view it on GitHub.

@domenic
Copy link
Member

domenic commented Jun 1, 2015

It's worth mentioning that await has never guaranteed no side effects.

Agreed.

there's nothing about the thenable contract that guarantees no side-effects.

Strongly disagree. The thenable contract is about type conversion between promise implementations, and fulfilling that contract means behaving like a promise---i.e. no side effects. It's true that Promises/A+ is phrased in terms of "you must do this" and not "you must do this and nothing else." But everyone would agree that "perform the usual Promises/A+ steps and then delete your C:\ drive" is not really fulfilling the thenable contract. Similarly, "perform an arbitrary action, then create something that performs the usual Promises/A+ steps" is not in the spirit of the contract either.

@benlesh
Copy link

benlesh commented Jun 1, 2015

The thenable contract is about type conversion between promise implementations, and fulfilling that contract means behaving like a promise---i.e. no side effects.

@domenic (This is an honest question, I can't think if a better way to phrase it, sorry, don't read into it) How can a contract enforce no side-effects in JavaScript?

@stefanpenner
Copy link
Contributor

How can a contract enforce no side-effects in JavaScript?

it can't strictly enforce, rather by convention it can be enforced.

@benlesh
Copy link

benlesh commented Jun 2, 2015

Ah... I guess when I read "guarantee" I think "enforcement".

@jhusain
Copy link
Collaborator Author

jhusain commented Jun 2, 2015

Regardless of the original intent of the original spec, we should
realistically discuss what potential hazard implicit Observable/Promise
might create. I fail to see what hazard is prevented by disallowing
stateful Observable/Promise adaptation when the side-effects will just end
up being executed immediately prior to an await. Forcing someone to call
await on an observable.subscribe() that promptly deletes their hard drive
doesn't seem like an improvement to me over simply executing the
side-effects during adaptation. I also want to preserve the ability to
await something lazy.

I think we should be focused on giving developers what they expect. We have
some evidence that they expect this implicit conversion from the .NET
platform where async/await has been vetted.

On Mon, Jun 1, 2015 at 4:40 PM, Stefan Penner notifications@github.com
wrote:

How can a contract enforce no side-effects in JavaScript?

it can't strictly enforce, rather by convention it can be enforced.


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

@domenic
Copy link
Member

domenic commented Jun 2, 2015

I've covered what the hazard is above:

A type conversion (what happens when you do await thenable) should not cause side effects---bad semantics. And awaiting observables causing side effects while awaiting promises does not is confusing to developers.

I think we should be focused on giving developers what they expect.

I definitely agree. Which is why I think awaiting anything should not cause side effects, since that is what JavaScript developers will expect from using promises.

@jhusain
Copy link
Collaborator Author

jhusain commented Jun 2, 2015 via email

@jhusain
Copy link
Collaborator Author

jhusain commented Jun 2, 2015

@domenic any objection to observable.returnValue()? Would like to have some solution for async/await integration before Babel inclusion.

@benjamingr
Copy link

If it's .returnValue it might as well be toPromise and describe what it is doing - it's also less to type.

@zenparsing
Copy link
Member

@jhusain wouldn't returnValue() be equivalent to subscribe(), (with zero arguments) if the returned subscription were promise-y?

@benjamingr
Copy link

@zenparsing that would require .subscribe to allow being called with 0 arguments, I was not aware this is the case at the moment.

If Type(O) is not Object, throw a TypeError exception.

@zenparsing
Copy link
Member

@benjamingr it's not the case at the moment, but we could certainly allow that if we wanted to.

@jhusain
Copy link
Collaborator Author

jhusain commented Jun 2, 2015

Commented in other thread about hazard of making subscribe a promise. The reason to call it final value is that there is likely to be other combinators along the same lines: firstValue(), lastValue(), and so on.

@domenic
Copy link
Member

domenic commented Jun 2, 2015

My objection to .returnValue() is that we are adding a fourth almost-does-the-same-thing method. I would much prefer the direction discussed in #20 (comment) to pare things down to one method instead of adding a fourth.

@benjamingr
Copy link

@zenparsing could you clarify what was settled on here? How are subscriptions going to be awaited?

@benlesh
Copy link

benlesh commented Jul 2, 2015

@benjamingr agreed, I'm actually a little confused about this. Last I talked to @jhusain he had said when as opposed to then, at least for the RxJS vNext library. ... but that's not to say it's a part of this spec.

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