Skip to content

Keep original list order - #16771

Open
pazams wants to merge 6 commits into
nudeui:gh-pagesfrom
pazams:keep-original-list-order
Open

Keep original list order#16771
pazams wants to merge 6 commits into
nudeui:gh-pagesfrom
pazams:keep-original-list-order

Conversation

@pazams

@pazams pazams commented Oct 21, 2015

Copy link
Copy Markdown

Consider the list's data originated from data fetched by an AJAX call.
The data is sorted by the server with a unique strategic algorithm (e.g a locations auto complete service sorted by distance from user).

There should be a way to maintain the server's list order.
Using the compare function: function (a,b){return 0;} won't work.
That's because Array.prototype.sort is not a stable sort.
from http://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.11
"sort is not necessarily stable (that is, elements that compare equal do not necessarily remain in their original order)"

This unstable sort behaviour can be demonstrated in arrays above 10 items in a chrome browser:
http://plnkr.co/edit/8Chk6VB2eR7WPfSNYVRl?p=preview

To fix this issue in Awesomplete, we can either:

  • split the evaluation chain inside evaluate(), and conditionally apply .sort.
  • extend Array.prototype with a function to wrap Array.prototype.sort, and conditionally apply .sort.

This pull request implements the 1st solution, to avoid extending Array.prototype with it's potential conflicts.

@nzucker

nzucker commented Oct 21, 2015

Copy link
Copy Markdown

+1

@LeaVerou

Copy link
Copy Markdown
Collaborator

Thanks for the PR, and for spotting the issue! Also, great to see a PR that actually adds tests for the behavior too!
However, this solution is rather inelegant and hacky. Not only the code is awkward, but it doesn't solve the problem in cases where this.sort is set. I’d rather we implemented a stable sort for all cases. It doesn’t have to extend Array.prototype, it could just be a helper on Awesomplete.$.

@pazams
pazams force-pushed the keep-original-list-order branch from 456a969 to 8a0c766 Compare October 28, 2015 01:24
@pazams

pazams commented Oct 28, 2015

Copy link
Copy Markdown
Author

@LeaVerou , I've updated the PR to reflect your suggestions.
mergeSort algorithm was added (adapted from here).
There was a way to "make" Array.sort be stable by passing in a wrapper object with original index, but this tested out to be too slow(http://jsperf.com/stable-sort-comparison/5).

My personal view:

  • The code now is less hacky.
  • But we traded off diverging from the ECMAS spec. There is a reason why ECMAS didn't require Array.prototype.sort to be a stable sort, and I'm not sure why Awesomplete needs to be different (excluding the case where the array shouldn't be sorted at all).

@pazams

pazams commented Oct 30, 2015

Copy link
Copy Markdown
Author

now also improved testing - getListFromResults() will produce an array from the list elements output, to aid in comparing with original input array. this helper function should be moved to the shared file created by PR #16776 .

@lydell

lydell commented Nov 18, 2015

Copy link
Copy Markdown
Contributor

I had the same use case recently: I want to preserve the order from the server. I made the following modification in evalute():

diff --git a/awesomplete.js b/awesomplete.js
index a422d1b..872b7e2 100644
--- a/awesomplete.js
+++ b/awesomplete.js
@@ -219,11 +219,16 @@ _.prototype = {
            // Populate list with options that match
            this.ul.innerHTML = "";

-           this._list
+           var filtered = this._list
                .filter(function(item) {
                    return me.filter(item, value);
                })
-               .sort(this.sort)
+
+           var sorted = this.sort.length === 1
+               ? this.sort(filtered)
+               : filtered.sort(this.sort);
+
+           sorted
                .every(function(text, i) {
                    me.ul.appendChild(me.item(text, value));

This allowed me to use the following sorting function (while still being able to use the default sorting functions if I wanted to):

function sort(list) {
    return list
        .map((item, index) => {
            return {
                item: rank(item),
                index,
                original: item,
            }
        })
        .sort(stableSort((a, b) => b - a))
        .map(({original}) => original)
}

function stableSort(fn) {
    return (a, b) => {
        let value = fn(a.item, b.item)
        return value === 0 ? a.index - b.index : value
    }
}

function rank(item) {
    // expensive function
}

In my opinion, it is up to the user to make sure the sorting is stable, just like I did above. Also note how I could move my expensive rank() function out of .sort, which noticeably increased performance for me.

Thoughts?

@pazams

pazams commented Nov 20, 2015

Copy link
Copy Markdown
Author

@lydell thanks for sharing your solution.
Here's a summary of suggested approaches mentioned above:

  • allow control whether awesomeplete should perform a sort or not (original PR)
  • always use a stable sort allowing effectively to maintain the original list order (current PR as requested by @LeaVerou)
  • allow control whether awesomeplete should perform a stable sort or unstable sort.

From a technical perspective, I'd prefer the first or third approach.
But these suggestions should also be viewed from the perspective of API consumers- web devs who just want an auto complete widget with no hassle around it.
When taking the API consumer into account, the 2nd approach might be the most API friendly, while the third might require some developers to ask themselves, what's all this "stable" talk about and re read their old algorithms textbooks.

Just my thoughts :)

@lydell

lydell commented Nov 21, 2015

Copy link
Copy Markdown
Contributor

Nice summary, @pazams. I have one thing I’d like to add to it, though. Only the third approach allows me to move my expensive rank() function out of .sort(), right?

@pazams

pazams commented Nov 21, 2015

Copy link
Copy Markdown
Author

@lydell - it might be so. I've edited my earlier response to reflect that technically the 1st and 3rd are some what equivalent. The 3rd option is a generalized 1st option, and might be better for when someone would need a stable sort applied on a compare function other than just keeping the original list order. Though I can't think of a use case for a stable sort with a compare function other than function (a,b){return 0;} in the context of an auto complete widget.

@lydell

lydell commented Nov 22, 2015

Copy link
Copy Markdown
Contributor

@pazams My compare function mostly returns 0 (to keep server order, which requires a stable sort), but in a few special cases does not.

@vlazar

vlazar commented Nov 24, 2015

Copy link
Copy Markdown
Collaborator

I'm kind of late to the party, but was this already considered?

// to keep the original list order use:
new Awesomplete(input, { sort: false });

// otherwise API user should be aware of browser's sort behavior and deal with it
new Awesomplete(input, { sort: function (a, b) { return 0; } });

There are for sure lots of existing stable sort libs, so why not only introduce one more, but also add it to Awesomplete codebase?

The code change required to be able to keep list order can be as simple as:

diff --git a/awesomplete.js b/awesomplete.js
index e2a8341..32effcd 100644
--- a/awesomplete.js
+++ b/awesomplete.js
@@ -220,12 +220,12 @@ _.prototype = {
                        // Populate list with options that match
                        this.ul.innerHTML = "";

-                       this._list
+                       var items = this._list
                                .filter(function(item) {
                                        return me.filter(item, value);
-                               })
-                               .sort(this.sort)
-                               .every(function(text, i) {
+                               });
+                               if (this.sort) items = items.sort(this.sort);
+                               items.every(function(text, i) {
                                        me.ul.appendChild(me.item(text, value));

                                        return i < me.maxItems - 1;

@pazams

pazams commented Nov 24, 2015

Copy link
Copy Markdown
Author

@vlazar, yes, your code suggestion was how I coded the original PR:

split the evaluation chain inside evaluate(), and conditionally apply .sort.

It too applied the sorting if this.sort was truthy.
I should have used git revert over git reset to preserve the code of the original PR.

@vlazar

vlazar commented Nov 25, 2015

Copy link
Copy Markdown
Collaborator

@pazams I'm sad this goes in "more helpers" direction. More code means more place for bugs.

Having tests for original PR intention (Keep original list order) is nice, but sort algorithm probably needs extensive coverage. And good tests for sort implementation is kind of project of it's own.

@LeaVerou If you are willing to add sort helper anyway, why not use external module and maybe bundle it at build step? This way it can be developed and supported in it's own repo. Also, such module probably already exists.

@vlazar

vlazar commented Nov 25, 2015

Copy link
Copy Markdown
Collaborator

As an afterthought, maybe we need to think about introducing awesomplete plugins?

@pazams

pazams commented Nov 25, 2015

Copy link
Copy Markdown
Author

@vlazar, I too would've just preferred to just conditionally apply the sort, and stay away from stable sorting.
We probably should wait on Lea's remarks before anything else.
@LeaVerou, please reconsider keeping the original list order by simply applying the sort conditionally? either condition on this.sort or on a new exposed property?

@lydell

lydell commented Nov 25, 2015

Copy link
Copy Markdown
Contributor

@pazams Do you propose being able to choose between not sorting at all and sorting using a possibly unstable sort (depending on the browser)? So everyone needing a stable sort are left to using a fork?

@pazams

pazams commented Nov 25, 2015

Copy link
Copy Markdown
Author

@lydell, I'm suggesting that the code consuming Awesomplete can stable sort the list on his part. Then, Awesomplete only needs to allow to keep the original list order, and everyone is happy.

I looked at AngularJS as a reference for a library that exposes a sorting functionality ("orderBy")

In the past:
they too seem not to care about a stable sort, but rather allow to short circuit it if the predicate is falsey value (as I and @vlazar suggested).
from http://stackoverflow.com/a/21343621:

Just took a peek at the angular source. The orderByFilter short circuits if the sort predicate (the thing after the colon) is a falsy value and returns the array unsorted...

In the present:
a recent commit (angular/angular.js@ed3a33a) added stable sort by default.

however:
comparing a lightweight library with a monolithic framework is problematic. Being lightweight is in Awesomplete's title, and therefore I think this library should let the consumer handle his special sorting needs by his own, and maintain his sort by allowing to keep the original list order.

Thoughts?

@lydell

lydell commented Nov 26, 2015

Copy link
Copy Markdown
Contributor

Being lightweight is in Awesomplete's title, and therefore I think this library should let the consumer handle his special sorting needs by his own, and maintain his sort by allowing to keep the original list order.

I agree 100% on this. That's why I suggested just that in my first comment.

However, the proposal in your second to last post does not seem to allow for a stable sort. If it does, could you please explain how?

@LeaVerou

Copy link
Copy Markdown
Collaborator

Yes, we do want Awesomplete to be small, but not buggy, and this is a bug and there are small stable sorts around.

@lydell

lydell commented Nov 26, 2015

Copy link
Copy Markdown
Contributor

But I’d still like to be able to do this.sort(list), rather than list.sort(this.sort). While the latter is more elegant, the former is more flexible (allowing you to do anything to the list).

By the way, this is a pretty minimal stable sort:

// Possible unstable:
array.sort(sortFn)

// Stable:
array
  .map((item, index) => {item, index})
  .sort((a, b) => sortFn(a.item, b.item) || a.index - b.index)
  .map(({item}) => item)

@pazams

pazams commented Nov 26, 2015

Copy link
Copy Markdown
Author

@lydell,

However, the proposal in your second to last post does not seem to allow for a stable sort. If it does, could you please explain how?

yes, consider this:

var input = document.getElementById("myinput");
var data = getData();
var sortedData = mySortingAlgorithm(data);

awesompleter = new Awesomplete(input,{sort: false});
awesompleter.list = sortedData;

as for the stable sort snippet on your last comment- it is very elegant. However, the technique of wrapping the elements and sorting on wrapped objects is very slow

Comment thread test/awesompleteShared.js

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like an awesomplete API needs some tweaks (not only for tests). See my comment here #16725 (comment)

Thoughts?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea about splitting list setter/getter API #16790 (comment)

@evenfrost

Copy link
Copy Markdown
Contributor

Any updates on this?

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.

6 participants