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

Compatibility with ES7 observables #154

Merged
merged 6 commits into from
Oct 3, 2015
Merged

Conversation

lautis
Copy link
Contributor

@lautis lautis commented Sep 25, 2015

Use ES7 observables as Kefir streams and allow Kefir streams to be converted to ES7 observables.

To initialise Kefir streams from ES7 observables, there is a new method Kefir.from. This could also be Kefir.fromObservable, but I've tried to conform with the ES7 observable spec.

To make Kefir streams compatible with observables, streams now have a new method for subscription: stream.subscribe. This takes an ES7-like observer as an argument. Unlike other methods, subscriptions initialised via subscribe will always end after first error. This is mandated by ES7 observable spec.

There is a similar PR for Bacon.js, and the upcoming version of RxJS will be also compatible with ES7 observables. Use ES7 observables as Kefir streams and allow Kefir streams to be converted to ES7 observables.

To initialise Kefir streams from ES7 observables, there is a new method Kefir.from. This could also be Kefir.fromObservable, but I've tried to conform with the ES7 observable spec.

To make Kefir streams compatible with observables, streams now have a new method for subscription: stream.subscribe. This takes an ES7-like observer as an argument. Unlike other methods, subscriptions initialised via subscribe will always end after first error. This is mandated by ES7 observable spec.

There is a similar PR for Bacon.js, and the upcoming version of RxJS will be also compatible with ES7 observables. As a bonus, these would be interoperable with Kefir.

@rpominov
Copy link
Member

Hey @lautis! Thanks a lot for the PR! I didn't follow the ES proposal discussion for some time and lost track a bit. I think there were some other inconsistencies beside ending on error. I will need to look closely at the proposal again.

I'll return to this soon. Also very curious what others think about it!

@rpominov
Copy link
Member

I think, at this point it makes sense to only make Kefir observables be convertible to/from ES observables, but not to make Kefir implement ES observable spec.

I mean we should not try to add methods from ES observable spec like Kefir.Observable.prototype.from() and Kefir.Observable.prototype.subscribe(). We should either add them all (the Observable constructor, forEach, of, etc. + any future methods), and maintain compatibility with the spec, or don't try to do this at all. The former looks like a lot of work and discussions.

So I think we should add only two following methods:

Not sure about fromESObservable name, although I don't like fromObservable because it can be confusing as we have something called Observable in Kefir already.

Also maybe it makes sense to add obs.toESObservable as an alias for obs[Symbol.observable].

@@ -0,0 +1,9 @@
module.exports = function(key) {
if (Symbol && Symbol[key]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably do typeof Symbol !== 'undefined' here, otherwise this will throw in case Symbol not defined as a global.

@lautis
Copy link
Contributor Author

lautis commented Sep 26, 2015

Extracted interoperability to separate class and changed the name of static method to consume ES7 observables to fromESObservable.

Re: Kefir.from, that might make more sense if it could be a general method to turn objects into a stream. But as Kefir doesn't (won't?) have an array/iterable to a stream method, there aren't really other use cases that could be easily handled. Kefir.fromESObservable seems to be the least confusing name.


toESObservable() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add this two methods to the prototype in index.js file, for consistency with how other methods are added.

@rpominov
Copy link
Member

Not sure yet what we should do with exceptions from observer methods.

And not 100% sure that it's ok to return a not zalgo-safe observable. Although you're probably right that spec doesn't require that.

I wonder what @Blesh and @zenparsing think about it. I'd wait for their answers in the Bacon issue.

@lautis
Copy link
Contributor Author

lautis commented Sep 27, 2015

I looked at the exception handling a bit more, especially on what happens in Kefir after an exception is thrown. As Kefir doesn't catch any exceptions (which does help a lot for performance), streams might end up in inconsistent state as the depending on whether the ES stream implementation unsubscribes properly or not.

The code for exception handling is in https://github.com/lautis/kefir/tree/es-observable-exceptions.

I'd definitely like to hear what the other implementations expect to happen on exception. It could also mean that Kefir.fromESObservable should unsubscribe on error.

@benlesh
Copy link
Contributor

benlesh commented Sep 27, 2015

In RxJS, all errors are caught and sent down the error handler path. If there is no error handler, it throws. Related: any error in a handler will also throw.

@rpominov
Copy link
Member

@Blesh Yeah, the problem is this is not the case in Kefir (and Bacon). In Kefir there are no try..catch blocks in the code. We intentionally don't catch exceptions and just allow an app to crash hard.

The question is should we still catch exceptions in the ES Observable adapter? Does spec require that? I mean exceptions thrown from observer.next(), observer.error(), and observer.complete().

@benlesh
Copy link
Contributor

benlesh commented Sep 28, 2015

They shouldn't throw. You should be okay.

rpominov added a commit that referenced this pull request Oct 3, 2015
Compatibility with ES7 observables
@rpominov rpominov merged commit 09aeea1 into kefirjs:master Oct 3, 2015
@rpominov
Copy link
Member

rpominov commented Oct 3, 2015

Ok, I think it makes sense to not catch errors at this point. We'll be able to change this behavior in the future after people start use it, and we'll get some feedback.

Gona release a new version with this a bit later.

Thanks a lot for the PR, @lautis!

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

Successfully merging this pull request may close these issues.

3 participants