Rename fromCallback to factoryFromCallback

[benlesh]

Since Observable.fromCallback doesn't really create an Observable, but rather creates a function, the name is a little confusing. Observable.factoryFromCallback makes a little more sense, as suggested by @staltz.

Related discussion in #729 is around whether or not it should be in Rx "core". It adds a lot of weight for limited value, IMO. I personally have never used this method, but perhaps it's more popular than I know.

• Should we rename it to factoryFromCallback?
• Should it be core?

[kwonoj]

+1 for renaming, I was also hit confusion first time try to use fromCallback, thought it directly returns observable.

[staltz]

I vote for factoryFromCallback and not in core.

[luisgabriel]

+1 for renaming and should not be in core.

I don't like the new name very much, but I don't have a better suggestion right now.

[mattpodwysocki]

👎 on both renaming and removing it from core. It is essential for both Node programming but also binding to things such as jQuery etc. The from should also stay as is. After all toPromise isn't consistent either but yet I don't see any outrage over that. That and I hate the word Favtory since it sounds too Java-like.

[robwormald]

We could call it observablifyCallback (like Bluebird's "promisify" method) ;-)

[staltz]

@mattpodwysocki fair enough, about core membership. But the name? Is Java-likeness really the problem? Plenty of people have tripped over on fromCallback. The current documentation is really bad, describing it simply as "Converts a callback function to an observable sequence." which is not true.

If not a rename, we could change it to actually return an Observable, not a function. E.g.

var source = Rx.Observable.fromCallback(fs.exists, 'file.txt');

// Can be subscribed
source.subscribe(...)

If anyone needs the factory function, then simply:

function exists(filename) {
  return Rx.Observable.fromCallback(fs.exists, filename);
}

[benlesh]

Well more interesting than that, @staltz, it returns a function that returns a stateful Observable that caches it's result, similar to fromPromise. It's very different from all of its like-named siblings.

[staltz]

Hmm, indeed https://github.com/Reactive-Extensions/RxJS/blob/master/src/core/linq/observable/fromcallback.js#L16
I didn't know about that. What are the problems of implementing fromCallback like I described above (stateless, with Observable.create)?

[benlesh]

No problem other than compatibility concerns, AFAICT.

[staltz]

So, let's keep the name but fix the functionality...

[benlesh]

I'm inclined to agree with @staltz, we should let users compose the behavior they want here, and with consistent tools.

[staltz]

A problem with changing the functionality is the signature starts getting really complicated:

static fromCallback: <T>(callbackFunc: Function, args: any[], ctx?: Object, selector?: Function, scheduler?: Scheduler) => Observable<T>;

args should be an array, so if callbackFunc takes 3 arguments, it would be

Observable.fromCallback(callbackFunc, ['blish', 'blesh', 'blosh'], {value: 42}, function (err, datum) { return datum; }, Scheduler.nextTick);

Should we drop ctx and selector, or do something else?

For instance perhaps

static fromCallback: <T>(callbackFunc: Function, ...argsOrScheduler: Array<any | Scheduler>) => Observable<T>;

Observable.fromCallback(callbackFunc, 'blish', 'blesh', 'blosh', Scheduler.nextTick);

[benlesh]

@staltz ... We wouldn't want to drop selector. It's an important feature for these types of creation methods because it generally saves people from creating a second subscription to map the values they're getting back. I'm unsure about the ctxt argument, but then again, I'm in favor of removing all of the thisArg-type arguments in general.

[mattpodwysocki]

@Blesh context is absolutely necessary, for example, if I were to call a method on a stateful object, it would lose its this immediately for example:

const $book = $('book');

const animate = Rx.Observable.fromCallback($book.animate, $book);
animate({
    opacity: 0.25,
    left: "+=50",
    height: "toggle"
  }, 5000
).subscribe(x => console.log(done))

[mattpodwysocki]

I would be perfectly willing to rename to names such as bindCallback or bindNodeCallback but the from implied that we are converting it (eventually) to an Observable.

[benlesh]

but the from implied that we are converting it (eventually) to an Observable.

I think what bites people is that Observable.fromCallback reads as "Observable from callback". As in, it's going to return an observable from that call like from, fromArray, fromPromise, fromEvent, fromEventHandler, etc.

[mattpodwysocki]

@staltz there is a reason here for using an AsyncSubject as we do not want to continue to fire the callback time and time again, when once is good enough for something with a callback such as:

const mkdir = Rx.Observable.fromNodeCallback(fs.mkdir);

const createDirectory = mkdir('tmp');
createDirectory.subscribe(x => console.log('done'))

To "share" that behavior you would either have to publish/refCount or worse leading to worse performance.

[staltz]

bindCallback sounds like a better name than fromCallback, but I'm not sure yet what's the best overall solution. Currently there isn't even an AsyncSubject in RxJS 5, and I also think in some cases people want to continue firing the callback time and time again, in the weird cases* the callback does some side effect which people want to repeat.

* because JavaScript and hundreds of thousands of packages. Y'know...

[benlesh]

and I also think in some cases people want to continue firing the callback time and time again, in the weird cases* the callback does some side effect which people want to repeat.

... this is why I was suggesting that we make the caching behavior of this something that we can compose in.

[mattpodwysocki]

@Blesh @staltz I don't get why AsyncSubject is not in RxJS v5 since its wide use in RxJS v4, especially with data access and caching. Yes, for the most part it was an anti-pattern to use Subjects explicitly but had many uses such as publishLast, start, startAsync among others. After all, it is part of RxJava and all other implementations of RxJS

[benlesh]

I don't get why AsyncSubject is not in RxJS v5

Honestly, the need hasn't arisen. That's probably the only reason it's not in here.

[staltz]

Yeah, never used AsyncSubject neither start neither startAsync neither publishLast myself either.

[antoineol]

Hi, jumping into the discussion,

In an Angular 2 application, I have cases when I need AsyncSubject: getting some async result and making it available for other services of the app. There can be multiple subscriptions, the result is got only once. This is similar to what @mattpodwysocki described.

AsyncSubject or any alternative to reproduce this behavior (what we have with promises when always returning the same promise) would be welcome :)

Thanks!

[benlesh]

@architruc you're in luck: #850

I'd expect that to land soon.

[benlesh]

bindCallback sounds like a better name than fromCallback, but I'm not sure yet what's the best overall solution.

I agree here. It might be better to be able to compose the behavior: Observable.fromCallback(someFn).publishLast().refCount()

We can add bindCallback as a helper for the above, of course.

@Blesh context is absolutely necessary, for example, if I were to call a method on a stateful object, it would lose its this immediately for example:

@mattpodwysocki wouldn't your example behave the same with Rx.Observable.fromCallback($book.animate.bind($book))?

Or even nicer with ES function bind: Rx.Observable.fromCallback(::$book.animate)

Since bind is in every runtime we're targeting, and since it's idiomatic to JavaScript, and therefore more readable, I think we should avoid thisArg type arguments in our methods. If I have someFunc(obj.fn, obj) it's not readily clear what passing the second argument is doing as opposed to someFunc(obj.fn.bind(obj)) or someFunc(::obj.fn) where it's explicit what's being done.

[benjamingr]

+1 for core, this is very fundamental and important in getting more people on board with Rx. Anything that means less complexity for people get started and less mistakes in code they write is good IMO.

+1 for renaming, this name implies it returns an observable and doesn't make a lot of sense.

I think observify and a prototype method might be good (very widely used in bluebird) but I assume we disagree here and I'd settle for factoryFromCallback or liftCallbackFn.

(btw, I think ES function bind at that current syntax is dead, just FYI). Also @Blesh as far as I recall bind is slow and I'd like to have as little overhead as possible in terms of performance in a method (the output of this factory method) that might be called hundreds of thousands of times on the server.

[benlesh]

Also @Blesh as far as I recall bind is slow and I'd like to have as little overhead as possible in terms of performance in a method (the output of this factory method) that might be called hundreds of thousands of times on the server.

If that were a major concern, would you not just implement your own bind method and compose it in? It would be a LOT more readable than the thisArg arguments. takesAFunc(bind(someFunc, someObj)) vs takesAFunc(someFunc, someObj). I mean, it would be much more clear that you're binding the function to the object then at least. The mythical "self documenting code" and all of that.

I think ES function bind at that current syntax is dead, just FYI)

That's tragic. Hopefully obj::whatever() is still alive.

[mattpodwysocki]

@Blesh I would side with @benjamingr on this one as Function.prototype.bind is incredibly slow, so why not give them the ability to do it inline without them having to implement their own custom bind. We can do it for them with little to no cost. And yes, it is a major concern...

[benjamingr]

Also, they can't really do it themselves without it. They'd have to allocate a closure.

[benlesh]

They'd have to allocate a closure.

Technically our impl allocates a closure.

And to use a trick I learned from you, @benjamingr, I think we can provide a module (since we're all about modularity) that gives them a simple bind function that doesn't allocate a closure:

export function bind(fn, thisArg) {
  const bound = function boundFn() {
    return boundFn.fn.apply(boundFn.thisArg, arguments);
  };
  bound.fn = fn;
  bound.thisArg = thisArg;
  return bound;
}

[IgorMinar]

FWIW, on Angular we found thisArg to be problematic when it comes to readability. We removed it from our apis long time ago and rejected any attempts to bring them back in.

In our experience this code is much more readable:

var nameOfaContext = this; // sometimes we use `self`
someApi(function callback() {
  nameOfaContext.doSomething();
});

just my 2c

// cc: @petebacondarwin

[benlesh]

Okay, after an offline discussion with @mattpodwysocki, I think we can remove the thisArg from methods that have it providing that we:

1. provide a small custom bind module. It shouldn't be included in core Rx, but perhaps Rx.KitchenSink. This will allow users to compose bound functions in a readable and performant way.
2. provide documentation on performance optimization for RxJS that includes a section about bound functions, and describes using the above custom bind module, and/or using something like lodash's _.bind.

[benjamingr]

I'm not sure I understand this. For client side code this sounds fine - but then again for client side code performance of async primitives is almost never an issue - for this reason @IgorMinar 's comment is mostly irrelevant here if we're talking about performance.

In terms of consistency I think it might be worth adding since all native methods have it. That's not the issue here again.

The more expensive an abstraction is the more tempted users will be to make shortcuts and undermine it. For server side use allocating an extra closure here is a performance killer. It will make me avoid fromCallback altogether or promisify methods and let Rx consume that.

What I'd really want is an observifyAll that lets me convert an entire callback API to an observable API in one go. Like Bluebird's promisifyAll this would already make context a non-issue since we'll still be making regular method calls.

If we want to convince people to use observables here, we better give them close to zero overhead abstractions - or they'll take shortcuts. If I have a server and I call 5 async methods every time a user makes a request and I expect to support 20000 users this means 100000 observables in flight - and 100000 fromCallback calls. This is a very real use case. If we don't give users a way to avoid the extra callback allocation (or better yet, support observifyAll or something similar) people will simply opt out of observables - as has been the case with promises until the problem was solved there.

I ask you not to repeat our mistakes.

[benlesh]

In terms of consistency I think it might be worth adding since all native methods have it.

That's fine, I'm willing to keep it on the methods that match native signature names: filter, map, reduce, forEach, only for consistency's sake.

What I'd really want is an observifyAll

I agree this would be a very cool feature, and I'd also like to see this. Make an issue (or better, submit a PR) and we'll get it in.

[benlesh]

... it should be noted that I'm not sure an observablifyAll would work with the current fromCallback implementation, because it returns an Observable that caches the value. I think we need a fromCallback that just returns a plain observable (not a function) that doesn't do any caching.

[david-driscoll]

It cache's the result from the callback for the invocation, I don't think it caches the result based on the invocation arguments. The fact the resulting Observable caches it's value doesn't mean anything to the next invocation, it's basically acting like a Promsie, but you could easily use it as the projector function in mergeMap for example.

So observablifyAll would basically just update all the methods to return an Observable when called instead of their normal return value.

[benlesh]

This issue now discusses too much, I've broken the issue into three separate issues based on the decisions that have come out of this.

Closing in favor of #876 #877 #878

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.