kmalakoff / knockback

Knockback.js provides Knockout.js magic for Backbone.js Models and Collections.
http://kmalakoff.github.com/knockback/
MIT License
1.05k stars 70 forks source link

Best practices for creating Knockout Extenders #124

Closed ncortines closed 10 years ago

ncortines commented 10 years ago

Hi,

Very often I find myself creating custom Knockout extenders to augment my viewmodel's observables. Custom extenders are very convenient way to reuse pieces of logic across the whole application.

In some cases I find myself applying several extenders to an observable:

function MyViewModel (model) {
    this.observable = kb.observable(model, {key: field}).extend(extenderA: { ... } ).extend( extenderB: { ... } );
    ...
}

The problem for me starts when I want one of those extenders to return a new observable, not exposing anymore the original observable. For example, that is the case when using Knockout's "throttle" extender:

ko.extenders = {
    'throttle': function(target, timeout) {
        // Throttling means two things:

        // (1) For dependent observables, we throttle *evaluations* so that, no matter how fast its dependencies
        //     notify updates, the target doesn't re-evaluate (and hence doesn't notify) faster than a certain rate
        target['throttleEvaluation'] = timeout;

        // (2) For writable targets (observables, or writable dependent observables), we throttle *writes*
        //     so the target cannot change value synchronously or faster than a certain rate
        var writeTimeoutInstance = null;
        return ko.dependentObservable({
            'read': target,
            'write': function(value) {
                clearTimeout(writeTimeoutInstance);
                writeTimeoutInstance = setTimeout(function() {
                    target(value);
                }, timeout);
            }
        });
    },
...

Once you apply such extender, the original observable is no longer directly exposed and that causes a problem when releasing the ViewModel.

"kb.release" will dispose the computed observable returned by the extender, but it will not release the original kb.observable.

To workaround this, I found that such a construction seems to work reasonably well. This is my custom version of Knockout's "throttle" extender:

ko.extenders.throttle = function (target, timeout) {

    // This custom extender replaces knockout's throttle extender in order to propagate Knockback's release

    var result,
        writeTimeoutInstance = null,
        release = target.release || target.destroy || target.dispose;

    // Throttling means two things:

    // (1) For dependent observables, we throttle *evaluations* so that, no matter how fast its dependencies
    //     notify updates, the target doesn't re-evaluate (and hence doesn't notify) faster than a certain rate
    target.throttleEvaluation = timeout;

    // (2) For writable targets (observables, or writable dependent observables), we throttle *writes*
    //     so the target cannot change value synchronously or faster than a certain rate

    result =  ko.computed({
        read: target,
        write: function (value) {
            window.clearTimeout(writeTimeoutInstance);
            writeTimeoutInstance = window.setTimeout(function () {
                writeTimeoutInstance = null;
                target(value);
            }, timeout);
        }
    });

    result.release = function () {
        result.dispose();
        if (writeTimeoutInstance) {
            window.clearTimeout(writeTimeoutInstance);
        }
        if (release) {
            release();
        }
    };

    return result;
};

The trick for me was to expose a new custom "release" function in the new computed, which will both dispose the new computed and moreover invoke the dispose callback of the observable which is not longer exposed. The tricky, maybe "hacky", part of this was to find in knockback's source code that "release" is invoked before "destroy" or "dispose" disposal callbacks:

  kb.release = function(obj) {
    var array, index, value;
    if (!kb.isReleaseable(obj)) {
      return;
    }
    if (_.isArray(obj)) {
      for (index in obj) {
        value = obj[index];
        if (kb.isReleaseable(value)) {
          obj[index] = null;
          kb.release(value);
        }
      }
      return;
    }
    obj.__kb_released = true;
    if (ko.isObservable(obj) && _.isArray(array = _peekObservable(obj))) {
      if (obj.__kb_is_co || (obj.__kb_is_o && (obj.valueType() === KB_TYPE_COLLECTION))) {
        if (obj.destroy) {
          obj.destroy();
        } else if (obj.dispose) {
          obj.dispose();
        }
      } else if (array.length) {
        for (index in array) {
          value = array[index];
          if (kb.isReleaseable(value)) {
            array[index] = null;
            kb.release(value);
          }
        }
      }
    } else if (typeof obj.release === 'function') {
      obj.release();
    } else if (typeof obj.destroy === 'function') {
      obj.destroy();
    } else if (typeof obj.dispose === 'function') {
      obj.dispose();
    } else if (!ko.isObservable(obj)) {
      this.releaseKeys(obj);
    }
  };

I couldn't find anything in the documentation for the usage of this "release" function, so I'm afraid my logic to "chain-dispose" extended observables will become invalid when one day I'll upgrade knockback to a newer version.

It would be nice to obtain some kind official support from the library in order to "chain-dispose" extended observables.

The thing becomes much more "hacky" when dealing with "kb.collectionObservable":

function MyViewModel (collection) {
    this.observable = kb.collectionObservable(collection, { ... }).extend(extenderA: { ... } ).extend( extenderB: { ... } );
    ...
}

In order to "chain-dispose" "kb.collectionObservable", I had to cheat "kb.release" into believing that the new computed is also a "kb.collectionObservable"

This is one of my extenders for "kb.collectionObservable" (it kind of emulates lazy loading over a collection which is already in memory):

ko.extenders.lazyArray = function (target, timeout) {

        var result,
            addTimeout = null,
            release = target.release || target.destroy || target.dispose;

        if (!target.__kb_is_co) {
            throw 'Only kb.collectionObservable supported at this point';
        }

        target.elementsToDisplay = ko.observable(0);

        result = ko.computed(function () {
            var all = target();
            if (target.elementsToDisplay() > all.length) {
                target.elementsToDisplay(all.length);
            } else if (addTimeout === null && target.elementsToDisplay() < all.length) {
                addTimeout = window.setTimeout(function () {
                    addTimeout = null;
                    target.elementsToDisplay(target.elementsToDisplay() + 1);
                }, timeout);
            }
            return all.slice(0, target.elementsToDisplay());
        });

        result.__kb_is_co = true;

        result.destroy = function () {
            result.dispose();
            if (addTimeout) {
                window.clearTimeout(addTimeout);
                addTimeout = null;
            }
            if (release) {
                release();
            }
        };

        return result;
    };

Really "hackish", isn't it? :)

So, to summarize I'd like to know if there are some better practices in order to "chain-dispose" extended observables and, if there aren't, maybe find out if there is anything that could be officially introduced into Knockback in order to support these use cases and make this great library even greater.

Thanks, Juan Cortines

kmalakoff commented 10 years ago

I'm sorry that you are having to find these work arounds. I'm not planning on deprecating the conventions.

I've discussed with some colleagues and we need to investigate a little more how the default memory management in knockout works with extenders. I'll get back to you after learning more.

kmalakoff commented 10 years ago

OK. I've patched Knockout's extender.

  _extend = ko.subscribable.fn.extend
  ko.subscribable.fn.extend = ->
    target = _extend.apply(@, arguments)

    # release the extended observable
    if target isnt @ and kb.isReleaseable(@)
      _dispose = target.dispose
      target.dispose = => _dispose?.apply(target, arguments); kb.release(@)

    return target

This isn't a full solution as chains of extenders could leak, but that is a general memory management responsibility question for knockout.

Please try the version in master and let me know if it fits your needs. If so, I will release it. If you need a more complete solution, we should wait for the offiical Knockout response.

ncortines commented 10 years ago

Hi Kevin, thanks for patch!

It seems to be working pretty well for extended instances of kb.observable.

However it does not seem to be working for extended instances of kb.collectionObservable.

In my application I have a simple kb.collectionObservable extended with the "lazyArray" extender mentioned above. Once I remove my custom disposal logic it would like this:

ko.extenders.lazyArray = function (target, timeout) {

    var result,
        addTimeout = null;

    target.elementsToDisplay = ko.observable(0);

    result = ko.computed(function () {
        var all = target();
        if (target.elementsToDisplay() > all.length) {
            target.elementsToDisplay(all.length);
        } else if (addTimeout === null && target.elementsToDisplay() < all.length) {
            addTimeout = window.setTimeout(function () {
                addTimeout = null;
                target.elementsToDisplay(target.elementsToDisplay() + 1);
            }, timeout);
        }
        return all.slice(0, target.elementsToDisplay());
    });

    return result;
};
kmalakoff commented 10 years ago

OK. It should be fixed now

ncortines commented 10 years ago

Thanks! I have just upgraded to 0.19.1. All my tests are a pass.