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

feat(observable): observable can be augmented with a then method and therefore awaitable #3331

Closed
wants to merge 1 commit into from

Conversation

david-driscoll
Copy link
Member

Description:
Add a new import targets rxjs/awaitable as well as rxjs/extensions/awaitable that will augment
the observable prototype with a then method, and also make Observable<T> extend PromiseLike<T>.

I realize we closed #556 but this keeps getting stuck in my head, imo if someone wants to shoot themselves in the foot let them. Right now, as designed, this would be an optional import that has to be made on the consumer side.

As far as naming 🚲🏘 away.

Related issue (if exists):
#556

@kwonoj
Copy link
Member

kwonoj commented Feb 20, 2018

I was originally in favor of this but thinking it lately, do we really want to expose this in core? It's one-liner anyway, so anyone want this behavior can opt in by having custom wrapper function. Especially exposing then into Observable prototype seems quite concerning. (even though it's optional import or not)

…therefore awaitable

Add a new import targets 'rxjs/awaitable' as well as `rxjs/extensions/awaitable' that will augment
the observable prototype with a then method, and also make Observable<T> extend PromiseLike<T>
@kwonoj
Copy link
Member

kwonoj commented Feb 20, 2018

So probably I object this, rather suggest to someone have 3rd-party module, or each consumer opt in by creating fn themselves.

@rxjs-bot
Copy link

rxjs-bot commented Feb 20, 2018

Messages
📖

CJS: 1359.9KB, global: 726.5KB (gzipped: 117.3KB), min: 140.3KB (gzipped: 30.6KB)

Generated by 🚫 dangerJS

@coveralls
Copy link

coveralls commented Feb 20, 2018

Coverage Status

Coverage increased (+0.03%) to 97.591% when pulling acc6302 on david-driscoll:awaitable into ddffecc on ReactiveX:master.

@david-driscoll
Copy link
Member Author

david-driscoll commented Feb 20, 2018

I'm honestly on the fence, but it was an easy change and a quick potential win (and it can sit for a while).

I know Rx.NET supports await on observables (as per related issue) so we at least have some history there.

I see it as related to the async iterable problem, if we support async iterables via await, it's get more confusing that all observables cannot be awaited, however big a footgun it might be.

for (const item of await o) {
// ...
}
// vs
const a = await o;

@mattpodwysocki
Copy link
Collaborator

mattpodwysocki commented Feb 20, 2018

@david-driscoll Correct, all Observables must be converted to an AsyncSubject in order for await to even make sense in that case. In Rx.NET, it's no different as our GetAwaiter() method turns us into an AsyncSubject. Failing that, we could always give them the opportunity to have firstAsync, lastAsync, singleAsync, reduceAsync which does all that work for them which take an Observable and converts to a Promise.

@benlesh
Copy link
Member

benlesh commented Feb 22, 2018

Yeah, we can't really do this. It's been discussed several times in the past. In fact, I think I was the last one to propose it. The problem is that Observables and Promises are fundamentally different in the fact that Observables are lazy and promises are not. Because promises aren't lazy, the expected behavior of promise.then(fn) or await promise is to have no side effects. On the other hand, subscribing to an Observable could have side effects. Basically await someObservable would be like implicitly calling a function . In this case it's probably more readable to be explicit: await someObservable.forEach(fn) or await someObservable.toPromise()

@benlesh
Copy link
Member

benlesh commented Feb 22, 2018

After we move most people over to using the pipeable operators, I wouldn't be opposed to renaming toPromise() to last() or lastValue() to make things more readable/expressive: await someObservable.last()

@drweb86-work
Copy link

drweb86-work commented Apr 5, 2018

But code-readability benefits much from promise await functionality. If you could do this in some way for observables.

Just see

someService1
	.call(arg1, arg2)
	.subscribe(response) {
		if (!response.invalid) {
			showMessage('Universe stopped.')
			return;
		}
		return someService2
			.call(response.data1, arg2)
			.subscribe(res => {
				doSomethingElse
			})
		})
	}

This code will be something like this

const response = await someService1.call(arg1, arg2);
if (!response.invalid) {
	showMessage('Universe stopped.')
	return;
}

const res = await someService2.call(response.data1, arg2)
doSomethingElse

Please, consider doing something like this for API calls. Those switchMaps and subscribes when all code shifts to the right is some kind of nightmare.

I know about calling
first().toPromise()
but that sounds like antipattern for me

@NinoScript
Copy link

@drweb86-work your use case looks like it could be improved by just using flatMap

@benlesh
Copy link
Member

benlesh commented Jul 26, 2018

Okay, after a bit of thinking about this, I don't think we can do it in this manner. It might be better to rename toPromise to lastValue or something, but here's the problem:

  • Observables are lazy and can have side effects
  • Promises are eager and can also have side effects.
  • If you await the same promise twice, the side effect will only happen once, because promises are eager (and by nature multicast)
  • If you await the same observable twice, SOMETIMES you'll get two side effects and sometimes you'll get one, depending on how the observable was implemented... because observables are lazy and usually unicast, but sometimes multicast.

The truth is that Observables are just a lower level construct.

Observables are basically functions. await someObservable is akin to await someFunction, you need to "call" the function or the observable to get it to do something.... await someFunction() or await someObservable.forEach() or await someObservable.toPromise()

I think we're going to close this one, because it's just not something we can do given the mental overhead and footguns it would put on our users.

@benlesh benlesh closed this Jul 26, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making Observable "awaitable"
8 participants