Kefir subclasses can't return same type

[mAAdhaTTah]

I have an ActionObservable that extends the built-in Kefir.Stream and adds an ofType method to it. When I have an instance of said ActionObservable, if I map over its values, it currently cannot return an instance of ActionObservable. I would return an instance of whatever I extended off of (in this case, Kefir.Stream).

It doesn't seem like there's an easy way to do this by even mucking about in the internals, so I'm wondering if this is a use-case we want to enable and if so, how would we go about doing so. Thoughts?

[Macil]

I'm not a big fan of encouraging sub-classing of Kefir classes. In my view, one of the pluses of Kefir over Bacon.js was the fact that Property and Stream have the same methods. In Bacon, it's not always very clear to users (and maybe maintainers) what type of class a multi-source transformation will return when different classes are used. In Kefir, you don't have to fret so much about what types of transformations will quietly take a Property and give a Stream. It was way too easy in a codebase using Bacon to swap out a Property for a Stream instance or vice-versa without realizing it even though the same kinds of values come through, with the swap possibly only happening in some conditional, and then you get bitten later when the code tries to use a method only present on one. A codebase introducing sub-classes of Kefir classes will reintroduce that pothole.

I'd be surprised if there was a way to do it that wasn't very invasive or a performance hit, but if there is a good way to do it then I wouldn't block it. Though I think it's better to encourage users to make functions that take Kefir streams as input rather than subclass (or monkey-patch) Kefir streams; maybe that's just a tip for the docs if we do have this later.

[mAAdhaTTah]

FWIW, I do think we have that problem a little bit (see #243, which I think is a result of merge returning a stream, not a property), although I think you're right that it's less of a problem. At a minimum, the behavior around whether a current value should flow is not always clear, although I think Kefir generally does a good job making that transparent to the user.

I'd be surprised if there was a way to do it that wasn't very invasive or a performance hit, but if there is a good way to do it and you're still interested then I wouldn't block it.

Yeah, I can't really see a way that isn't invasive at least; dunno from a performance perspective.

Though I think it's better to encourage users to make functions that take Kefir streams as input

Generally, I agree; the quirk is using the methods in combination with the streams. Writing cod like this:

doSomething(stream$.map(x => x + 1)).filter(x => x < 2);

can gets cumbersome if there are a number of functions involved here. The other thing I had previously considered but never pitched is something like the thru method from most.js, which is a really simple operator that takes a function that gets called with the stream itself. Literally this:

proto.thru = function (fn) {
  return fn(this);
};

So the ofType method I'm trying to add would become a function that gets passed into thru. Maybe that would be a better way of solving the problem?

[rpominov]

I like to consider classes as an internal implementation detail of Kefir that is used mainly for performance reasons. We don't technically expose classes as public API, I mean there is no new in our docs.

On the other hand maybe we could dispatch to this.constructor... in all methods that are supposed return 'values of same type', but not sure it's a good idea.

I like the thru method!

[Macil]

One possible solution to custom function chaining would be the javascript bind operator proposal (available as a babel plugin), but there doesn't seem to be real progress on it lately, so thru sounds like a convenient idea.

[mAAdhaTTah]

We don't technically expose classes as public API, I mean there is no new in our docs.

We do export Observable, Stream, Pool, & Property on the public Kefir instance, and there was some discussion in #177 about documenting this.

I'm not a huge fan of the bind operator, personally, so I'm kinda glad it stalled, but that's just me.

Sounds like we have consensus on thru, since it's pretty simple, so I'll take a crack at that + tests + Flow types for it. It's certainly easier than trying to figure out how make the extension process work!

Also, just to throw it out there, this project has a couple neat ideas and is extending off of Property: https://github.com/calmm-js/kefir.atom So we certainly should be aware of and treat the exposed "classes" as public.