Add 'mappingWithDisposeCallback' option to sortBy projection

[AdamWillden]

Has the sortBy disposeItem option been used so far in any of your projects as far as you know? I don't see any usability in the parameter being the sort values array (as in [123, 'bob', 'fred']). I would have thought the parameter should be the state item's inputItem (as in 'mapping' : function(inputItem, descending) {}

The code I'm referring to is as follows:

    SortedStateItem.prototype.dispose = function() {
        var mappedItem = this.mappedValueComputed();
        this.mappedValueComputed.dispose();
        if (this.projection.options.disposeItem) {
            this.projection.options.disposeItem(mappedItem);
        }
    };

This is what would make sense to me:

    SortedStateItem.prototype.dispose = function() {
        var inputItem = this.inputItem;
        this.mappedValueComputed.dispose();
        if (this.projection.options.disposeItem) {
            this.projection.options.disposeItem(inputItem);
        }
    };

Any thoughts? I'm really sorry to keep picking up such things, I tend to push the limits of anything I use. Or should that be "I misuse everything".

[sunesimonsen]

Sorry - no we don't use disposeItem in any of our projects. I guess I added it to be API compatible with: https://github.com/One-com/knockout-projections/blob/master/src/knockout-projections.js#L58 you can see how it is used here: https://github.com/One-com/knockout-projections/blob/master/spec/map.spec.js#L371

But I'm struggling to find a use case for disposeItem in the sortBy context as the mapping is already evaluated in a computed context. So I guess we could just remove the code.

By the way, thanks for keeping a critical eye on the code. I was released into production a bit early for my taste, but overall it seems to work pretty flawless for us, but I guess if you push the boundaries you probably going to hit some ruff spots.

Kind regards Sune

[AdamWillden]

No worries for the critical eye, I've looked over the code rather intensely now as I've been working on a version that may well be usable for issue #5 (almost done). I've not seen anything else wrong, although I do think #4 may well be a live and current issue.

As for disposeItem, I've since looked again and realised that the disposeItem option is not actually what I'm after, what I actually want is the mappingWithDisposeCallback option which is not implemented (yet). I agree there's no usecase for disposeItem as only primitive objects should be passed to the mapping, it would not make sense to pass an object which might need disposing as you can't compare objects.

I was misusing disposeItem after all!

My need for mappingWithDisposeCallback: My use case is that I have 'pathing' between related entities (like full blown database relations). The source array 'entity' might have a jobId which I can then use to asynchronously fetch the referenced job. As such I can do asynchronous sorting using this plugin e.g. on the related job title. It fetches all the associated jobs and when each job is returned it caches the data and increments a sort counter which internally updates the status of the sort task. Only when the status reports the sort is complete does the job name get added to the mapped sort array thus triggering the sort. It works nicely!

When an item is removed permanently (or due to filtering) I need to decrement the counter so the sort status is correctly determined for the items in the array, to do this I need the mappingWithDisposeCallback option.

My code would be something like this:

var self = this;
this.data = fetchData();
this.filteredData = this.data.filter( /*...*/ );
this.sortedData = this.filteredData.sortBy({
    'mappingWithDisposeCallback': function (entity, descending) {
        var entityResourceKey = entity.getResourceKey();
        var pathConfigs = self.pathConfigs;
        var dataPaths = self.dataPaths;
        var dataPathStatuses = self.dataPathStatuses;
        //TODO path status need disposing if an item is removed from the root self.data
        //TODO path status needs disposing when table is disposed
        var paths = dataPaths[entityResourceKey] || (dataPaths[entityResourceKey] = {});
        var pathStatuses = dataPathStatuses[entityResourceKey] || (dataPathStatuses[entityResourceKey] = {});
        var sortedColumns = self.sortedColumns();

        var sort = [];
        for (var sortedColumnIndex = 0, numSortedColumns = sortedColumns.length; sortedColumnIndex < numSortedColumns; ++sortedColumnIndex) {
            var column = sortedColumns[sortedColumnIndex];
            var columnSortReady = column.sortReady();
            var columnValue;
            if (column.isEntityResolved(entity)) {
                //Don't start sorting this column until the sort is ready
                if (columnSortReady) {
                    columnValue = column.getSortValue(entity, paths);
                    sort.push(column.sortAscending() ? columnValue : descending(columnValue));
                }

                column.includeSortEntity(entity);
            } else if (column.resolveEntity(entity, pathConfigs, pathStatuses, paths)) {
               //entity has now been fetched from server (entity is resolved)
                column.includeSortEntity(entity);
            }
        }
        return {
            'mappedValue': sort,
             'dispose': function () {
                var sortedColumns = self.sortedColumns();
                for (var sortedColumnIndex = 0, numSortedColumns = sortedColumns.length; sortedColumnIndex < numSortedColumns; ++sortedColumnIndex) {
                    var column = sortedColumns[sortedColumnIndex];
                    if (column.isEntityResolved(entity)) {
                        column.excludeSortEntity(entity);
                    }
                }
            }
        };
    }
});

Hope this makes sense.

---

By the way I'll look at doing the code for this and I'll share it, whether you then include it is up to you!

[AdamWillden]

I have the code for this, its very simple. I'll wait in case my other PR is merged as it's based on that code. I haven't written any tests for it though as there's not really anything much to test.