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

Keep original list order #16771

Open
wants to merge 6 commits into
base: gh-pages
Choose a base branch
from

Conversation

pazams
Copy link

@pazams pazams commented Oct 21, 2015

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
Copy link

nzucker commented Oct 21, 2015

+1

@LeaVerou
Copy link
Owner

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
Copy link
Author

pazams commented Oct 28, 2015

@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
Copy link
Author

pazams commented Oct 30, 2015

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
Copy link
Contributor

lydell commented Nov 18, 2015

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
Copy link
Author

pazams commented Nov 20, 2015

@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
Copy link
Contributor

lydell commented Nov 21, 2015

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
Copy link
Author

pazams commented Nov 21, 2015

@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
Copy link
Contributor

lydell commented Nov 22, 2015

@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
Copy link
Collaborator

vlazar commented Nov 24, 2015

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
Copy link
Author

pazams commented Nov 24, 2015

@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
Copy link
Collaborator

vlazar commented Nov 25, 2015

@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
Copy link
Collaborator

vlazar commented Nov 25, 2015

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

@pazams
Copy link
Author

pazams commented Nov 25, 2015

@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
Copy link
Contributor

lydell commented Nov 25, 2015

@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
Copy link
Author

pazams commented Nov 25, 2015

@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
Copy link
Contributor

lydell commented Nov 26, 2015

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
Owner

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

@lydell
Copy link
Contributor

lydell commented Nov 26, 2015

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
Copy link
Author

pazams commented Nov 26, 2015

@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

@@ -41,5 +41,12 @@ var shared = {
expect(li.getAttribute('aria-selected')).toBe('true');
expect(shared.awesompleter.status.textContent).toBe(li.textContent);
}
},
getListFromResults: function (awesompleter){
Copy link
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
Author

Choose a reason for hiding this comment

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

+1

Copy link
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
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