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

Notify subscribers of previous value #520

Closed
wants to merge 1 commit into from
Closed

Notify subscribers of previous value #520

wants to merge 1 commit into from

Conversation

anishpateluk
Copy link

Hi Steve,

I've modified observable behaviours to pass the previous value of an observable into subscribe methods as well as adding a few tests.

Anish

@jamesfoster
Copy link
Contributor

+1 This is great.

@gvt
Copy link

gvt commented Jun 11, 2012

I like the idea; I've wished for this many times!

It looks like jQuery is now passed in to Knockout, does that make it depend on jQuery now?

@mbest
Copy link
Member

mbest commented Jun 12, 2012

Thanks for submitting this. It's definitely useful sometimes to have the previous value. But I think having all change notifications include the previous value is overkill. I'd rather have a separate subscribe function that does this. That way only those subscriptions that need the extra information will incur the extra overhead.

A few months ago, someone else uploaded a similar idea (#320) and I suggested the following function, which he was happy with:

ko.subscribable.fn.subscribeChanged = function(callback) {
    var previousValue;
    this.subscribe(function(_previousValue) {
        previousValue = _previousValue;
    }, undefined, 'beforeChange');
    this.subscribe(function(latestValue) {
        callback(latestValue, previousValue);
    });
};

Use it like this:

observable.subscribeChanged(function(latestValue, previousValue ) {
    // dosomething
});

@mbest
Copy link
Member

mbest commented Jun 12, 2012

It looks like jQuery is now passed in to Knockout, does that make it depend on jQuery now?

No. It's just so that we keep a copy of the version of jQuery that was current when Knockout was included. It's passed in as window.jQuery which will be undefined if jQuery isn't included.

@jamesfoster
Copy link
Contributor

Hi @mbest

That looks fine for primitives but not for arrays, etc.

var array = ko.observableArray([1,2,3]);
array.subscribeChange(function(newValue, oldValue){
  console.log(oldValue);
  console.log(newValue);
});

array.push(4)

Also with this, the overhead is done once for every call to subscribeChange. I guess an ideal solution would be something more like:

ko.subscribable.fn.subscribeChanged = function(callback) {
  this.subscribe(callback, null, "changeWithPrevious");

  if(this is the first time through) {
    var previousValue;
    this.subscribe(function(_previousValue) {
      previousValue = a shallow clone of _previousValue;
    }, undefined, 'beforeChange');

    this.subscribe(function(latestValue) {
      this.notifySubscribers([latestValue, previousValue], "changeWithPrevious");
    });
  }
};

You should get the idea from the above code. You only ever add 2 subscribers no matter how many times you call subscribeChange.

The problem is notifySubscribers would need to change to handle multiple arguments and this might be a breaking change.

@mbest
Copy link
Member

mbest commented Jun 12, 2012

James,

You're right that we shouldn't need to have two subscriptions for each call. Here's a new version that combines the best of both of ours:

ko.subscribable.fn.subscribeChanged = function(callback) {
    if (!this.previousValueSubscription) {
        this.previousValueSubscription = this.subscribe(function(_previousValue) {
            this.previousValue = _previousValue;
        }, this, 'beforeChange');
    }
    return this.subscribe(function(latestValue) {
        callback(latestValue, this.previousValue);
    }, this);
};

You're also right that this doesn't work for arrays. But that could be solved by creating a specific function for observableArray:

ko.observableArray.fn.subscribeChanged = function(callback) {
    if (!this.previousValueSubscription) {
        this.previousValueSubscription = this.subscribe(function(_previousValue) {
            this.previousValue = _previousValue.slice(0);
        }, this, 'beforeChange');
    }
    return this.subscribe(function(latestValue) {
        callback(latestValue, this.previousValue);
    }, this);
};

@jamesfoster
Copy link
Contributor

:) do you see what I see?

ko.subscribable.fn.trackPreviousValue = function() {
  this.subscribe(function(previousValue) {
    this.previousValue = previousValue;
  }, this, 'beforeChange');

  return this;
};

since you're storing the value in the observable then you can just do...

var x = ko.observable().trackPreviousValue();

x.subscribe(function(value) {
  console.log(x.previousValue);
});

you could even write it as an extender.

var x = ko.observable().extend( track: true );

@odedbd
Copy link

odedbd commented Jun 21, 2012

I would love to have either of the options in this thread. Would simplify some flows in my app. Thanks to all the good folks working on this.

@jamesfoster
Copy link
Contributor

@mbest Do you think we've got something worth baking into knockout? The extender seems very clean to me.

If so I'll re-write this and send a pull request.

@mbest
Copy link
Member

mbest commented Jun 22, 2012

I think this is something we can look after we complete version 2.2, especially since it isn't hard to have this as an add-on for now.

@paglias
Copy link
Contributor

paglias commented Apr 10, 2013

But I think having all change notifications include the previous value is overkill.

@mbest it would only pass a reference to the old value stored in observable.oldValue or something similar, I don't think it's going to be very expensive in terms of performance

@rniemeyer
Copy link
Member

Closing this one and allowing discussion to continue in #914 Thanks!

@rniemeyer rniemeyer closed this Mar 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants