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

disposing an observable should dispose its rxObservable #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gilesbradshaw
Copy link

No description provided.

@gilesbradshaw
Copy link
Author

just looked a bit more at this, it would also be good if the knockout observable only subscribed to the underlying rx observable when it is subscribed to.
to do that I did..

    function addSubscribeDispose(observable, subscribe, dispose) {
        var subscriptions = [];
        var oldSubscribe = observable.subscribe.bind(observable);
        observable.subscribe = function (callback, callbackTarget, event) {
            var oldDispose, ret;
            ret = oldSubscribe(callback, callbackTarget, event);
            if (!subscriptions.length) {
                subscribe();
            }
            subscriptions.push(ret);
            oldDispose = ret.dispose;
            ret.dispose = function () {
                subscriptions.splice(subscriptions.indexOf(ret), 1);
                oldDispose.bind(this)();

                if (!subscriptions.length) {
                    dispose();
                }
            };
            return ret;
        };
    }
    function rx2koObservable(initialValue) {
        var observable = ko.observable(initialValue);
        var rxSubscription;
        var _this = this;
        addSubscribeDispose(observable,function(){
            rxSubscription = _this.subscribe(observable);
        },
        function () {
            return rxSubscription.dispose();
        });
        return observable;
    }

I'm guessing the same applies vice versa too

@Igorbek
Copy link
Owner

Igorbek commented Sep 22, 2014

Thanks for PR, you're the first! 👍

  1. Can't accept PR, because .js is not a source file here, it has been generated from TypeScript source.
  2. I've got your concern about disposing, but there's a difference in behavior between Rx's observable and Knockout's one.

KO's observable doesn't provide dispose method to dispose it the whole object, it only has ability to dispose distinct subscribables. But the problem, that KO's observable contract requires accessor to last value of observable. So here's a problem: if we stop observing source Rx's observable even if mapped KO's observable doesn't have any subscription, we can't provide last value by the accessor:

var rxObs = new Rx.Subject();
var koObs = rxObs.toKoObservable();

assert.ok(koObs() === undefined, "value wasn't set yet");

rxObs.onNext(1);

assert.equal(koObs(), 1);

Your code will fail the test, because there's no KO's subscriptions yet.

  • Rx's observable can be represented by KO's subscribable object. And the library have such one: rxObservable.toKoSubscribable(). It counts subscriptions and keeps Rx's subscription only if it have any in KO.
  • KO's observable can be represented by Rx's BehaviorSubject. So, BehaviorSubject should be convertible to KO's subscribable with smart subscribing. And I'm going to implement this.

Igor

@gilesbradshaw
Copy link
Author

ah that's true - however if the rx observable is a cold observable and it isn't subscribed to until the ko observable is subscribed to then it won't produce any values until the ko observable subscribes. I'm not sue about subjects tbh - I don't use them just rx.Observable.create. Maybe it's a special case but it works for what I am doing where the underlying rx observable listens to a web api and subscribes to signalr endpoint when ko observable is active and then unsubscribes to signlar when disposed.

its a bit llike ko.computed vs ko.purecomputed

I wonder if there could be an alternative to rx2koObservable say coldRx2koObservable which do0es the above.

best wishes

@gilesbradshaw
Copy link
Author

ps my first foray into typescript (only hacking your code :P)

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.

2 participants