One-com / knockout-projections

Knockout.js observable arrays get smarter
5 stars 2 forks source link

sortKey undefined when sort descending after not sorting at all. #4

Closed AdamWillden closed 9 years ago

AdamWillden commented 9 years ago

I had originally written this in #1 but it was not directly related so I've deleted the comment and made this issue.

This issue was found due to the fact I change which properties are used to sort a data set. Starting with the data sorted by one or more properties, stop sorting upon any properties (such that array.sortBy(function(entity, descending) { /*...*/ return []; })) then sort a property descending. This will result in an issue where one of the sort keys are undefined (cannot read property 'value' of undefined).

Here is a simplified version of my script if it helps to make a unit test:

var sortedData = filteredData.sortBy(function(entity, descending) {
            var entityResourceKey = entity.getResourceKey(), sortedColumns = self.sortedColumns();
            var sort = [];
            for (var sortedColumnIndex = 0, len = sortedColumns.length; sortedColumnIndex < len; ++sortedColumnIndex) {
                var column = sortedColumns[sortedColumnIndex];
                var value = column.getSortValue(entity);
                sort.push(column.sortAscending() ? value : descending(value));
            }
            return sort;
        });

I have the 'fix' below which is based on code within the feature/change-sort-order branch. I don't know how to create unit tests to show the issue I'm facing. I'm not sure if this is the correct fix but it seems to fix it for me without showing any irregularities so far.

function compareSortingKeys(aSortKeys, bSortKeys) {
        var Descending = SortByProjection.Descending, isDecending;
        if (!Array.isArray(aSortKeys)) {
            aSortKeys = [aSortKeys];
            bSortKeys = [bSortKeys];
        }

        var aSortKey, bSortKey;

        for (var i = 0; i < aSortKeys.length; i += 1) {
            aSortKey = aSortKeys[i];
            bSortKey = bSortKeys[i];
//MODIFIED CODE STARTS HERE
            isDecending = aSortKey instanceof Descending; 
            aSortKey = aSortKey instanceof Descending ? aSortKey.value : aSortKey;
            bSortKey = bSortKey instanceof Descending ? bSortKey.value : bSortKey;
            if (isDecending) {
                if (aSortKey > bSortKey) {
                    return -1;
                } else if (aSortKey < bSortKey) {
                    return 1;
                }
//MODIFIED CODE ENDS HERE
            } else {
                if (aSortKey < bSortKey) {
                    return -1;
                } else if (aSortKey > bSortKey) {
                    return 1;
                }
            }
        }
        return 0;
    }
AdamWillden commented 9 years ago

I will try to see if this can happen under your supported usage. If it can then I will reopen with instructions. Again as in #3, I'll make the change locally myself and build it for my own release.