feat(orderBy): Add `null` and `undefined` at the end of the sorted list #16376

frederikprijck · 2017-12-17T21:07:39Z

What is the current behavior? (You can also link to an open issue here)
null values are sorted using type string resulting in a string comparison.

What is the new behavior (if this is a feature change)?
null and undefined values are, explicitly, being put at the end of the sorted list.`

Does this PR introduce a breaking change?
Yes

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

This fixes: #15294

frederikprijck · 2017-12-18T07:03:11Z

It looks like travis is failing coz of another commit message:

Need a colon after the closing parenthesis: ):
Commit message was:
style(css) separate selectors and declarations by new lines

But it looks like this is a very old commit. I'm somewhat confused 😕

Narretz · 2017-12-18T11:51:55Z

Did you fork this branch a while back, possibly in July? Because the commit that is throwing the error was merged back in July this year.

Although the commit message checker should only check commits that belong to the PR.

frederikprijck · 2017-12-18T11:53:19Z

@Narretz I did fork it awhile back idd. Would rebasing solve it (I doubt) ?

gkalpak · 2017-12-18T12:01:56Z

Rebasing should solve it (it is a bug if it doesn't).

frederikprijck · 2017-12-18T12:26:18Z

@gkalpak @Narretz Okay, I've rebased it!

gkalpak

Much needed change 👍
A couple of minor coments and should be good to go 🎉

gkalpak · 2017-12-19T09:00:09Z

src/ng/filter/orderBy.js

@@ -74,11 +75,13 @@
 *
 * The default, built-in comparator should be sufficient for most usecases. In short, it compares
 * numbers numerically, strings alphabetically (and case-insensitively), for objects falls back to
- * using their index in the original collection, and sorts values of different types by type.
+ * using their index in the original collection, sorts values of different types by type and puts `undefined` and `null` values at the end of the sorted list.


Wrap lines at 100 chars (everywhere) 🙏

gkalpak · 2017-12-19T09:03:25Z

test/ng/filter/orderBySpec.js

@@ -309,6 +309,14 @@ describe('Filter: orderBy', function() {

        expect(orderBy(items, expr)).toEqual(sorted);
      });
+
+      it('should treat `null` values bigger then all other values except `undefined`', function() {


Can you change to something like:

should consider null and undefined greater than any other value

gkalpak · 2017-12-19T09:20:50Z

src/ng/filter/orderBy.js

- * **Note:** For the purpose of sorting, `null` values are treated as the string `'null'` (i.e.
- *           `type: 'string'`, `value: 'null'`). This may cause unexpected sort order relative to
- *           other values.
+ * **Note:** `null` and `undefined` values are always treated bigger than any other value, where `undefined` is treated bigger than `null`.


I would change it to something like:

For the purpose of sorting, null and undefined are considered "greater than" any other value (with undefined "greater than" null). This effectively means that null and undefined values end up at the end of a list sorted in ascending order.

I would also add another note about null values using 'null' as their type (to more easily distinguish them from objects).

@gkalpak What about: https://github.com/angular/angular.js/pull/16376/files#diff-131b4811903f9fb834e9994a4386dc05R43 ?

I think that's fine. I would just add another note here.

gkalpak · 2017-12-19T09:23:03Z

src/ng/filter/orderBy.js

- * 1. If the compared values are of different types, compare the types themselves alphabetically.
+ * 1. If the compared values are of different types:
+ *    - Explicitly check for `null` and `undefined` values and treat them bigger as any other value.
+ *    - If both values are different from `null` and `undefined`, compare the types themselves alphabetically.


I would change the bullets to make the relative order of null vs undefined more explicit:

If one of the values is undefined, consider it "greater than" the other.

Else if one of the values is null, consider it "greater than" the other.

Else compare the types themselves aplhabetically.

gkalpak · 2017-12-19T09:26:50Z

test/ng/filter/orderBySpec.js

+        var expr = null;
+        var sorted = [false, 999, {}, 'z', null, undefined];
+
+        expect(orderBy(items, expr)).toEqual(sorted);


Just to be sure, could you also add an expectation for sorting in descending order?

gkalpak · 2017-12-19T09:33:54Z

BTW, we need a BREAKING CHANGE notice in the commit message.

frederikprijck · 2017-12-19T10:06:33Z

@gkalpak I'll add the BC notice in the commit message and squash the other commits.

frederikprijck · 2017-12-19T10:14:21Z

I've added the BC notice, unsure if it's enough or needs an example.

gkalpak

Getting there 😉

gkalpak · 2017-12-20T09:38:42Z

src/ng/filter/orderBy.js

- *           other values.
+ * **Note:** For the purpose of sorting, null and undefined are considered "greater than"
+ * any other value (with undefined "greater than" null). This effectively means that null
+ * and undefined values end up at the end of a list sorted in ascending order.


Indent subsequent lines as above.

gkalpak · 2017-12-20T09:39:43Z

src/ng/filter/orderBy.js

- * **Note:** For the purpose of sorting, `null` values are treated as the string `'null'` (i.e.
- *           `type: 'string'`, `value: 'null'`). This may cause unexpected sort order relative to
- *           other values.
+ * **Note:** For the purpose of sorting, null and undefined are considered "greater than"


Wrap "null" and "undefined" in backticks (as in other places).

gkalpak · 2017-12-20T09:41:22Z

src/ng/filter/orderBy.js

@@ -690,7 +695,7 @@ function orderByFilter($parse) {
        result = value1 < value2 ? -1 : 1;
      }
    } else {
-      result = type1 < type2 ? -1 : 1;
+      result = (type1 === 'undefined') ? 1 : (type2 === 'undefined') ? -1 : (type1 === 'null') ? 1 : (type2 === 'null') ? -1 : (type1 < type2) ? -1 : 1;


This line is still >100 chars long 🤔

What about using the following instead of wrapping it at 100 :

result = (type1 === 'undefined') ? 1 : (type2 === 'undefined') ? -1 : (type1 === 'null') ? 1 : (type2 === 'null') ? -1 : (type1 < type2) ? -1 : 1;

I think it improves readability ...

Previously, `null` values where sorted using type `string` resulting in a string comparison. `undefined` values where compared to other values by type and were usually considered greater than other values (since their type happens to start with a `u`), but this was coincidental. This commit ensures that `null` and `undefined ` values are explicitly considered greater than other values (with `undefined` > `null`) and will effectively be put at the end of the sorted list (for ascending order sorting). Closes #15294 BREAKING CHANGE: When using `orderBy` to sort arrays containing `null` values, the `null` values will be considered "greater than" all other values, except for `undefined`. Previously, they were sorted as strings. This will result in different (but more intuitive) sorting order. Before: ```js orderByFilter(['a', undefined, 'o', null, 'z']); //--> 'a', null, 'o', 'z', undefined ``` After: ```js orderByFilter(['a', undefined, 'o', null, 'z']); //--> 'a', 'o', 'z', null, undefined ```

Narretz · 2018-01-10T12:08:19Z

@gkalpak If you're happy with this, I will merge it

googlebot added cla: yes labels Dec 17, 2017

Narretz added component: filters needs: breaking change labels Dec 18, 2017

Narretz added this to the 1.7.0 milestone Dec 18, 2017

gkalpak suggested changes Dec 19, 2017

View reviewed changes

gkalpak suggested changes Dec 20, 2017

View reviewed changes

gkalpak approved these changes Jan 25, 2018

View reviewed changes

gkalpak closed this in 1d80464 Jan 25, 2018

dependencies bot mentioned this pull request May 11, 2018

Update 3 dependencies from npm JetBrains/ring-ui#257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(orderBy): Add `null` and `undefined` at the end of the sorted list #16376

feat(orderBy): Add `null` and `undefined` at the end of the sorted list #16376

frederikprijck commented Dec 17, 2017 •

edited

Loading

frederikprijck commented Dec 18, 2017 •

edited

Loading

Narretz commented Dec 18, 2017

frederikprijck commented Dec 18, 2017

gkalpak commented Dec 18, 2017

frederikprijck commented Dec 18, 2017

gkalpak left a comment

gkalpak Dec 19, 2017

gkalpak Dec 19, 2017

gkalpak Dec 19, 2017

gkalpak Dec 19, 2017

frederikprijck Dec 19, 2017 •

edited

Loading

gkalpak Dec 19, 2017

frederikprijck Dec 19, 2017

gkalpak Dec 19, 2017

gkalpak Dec 19, 2017

gkalpak commented Dec 19, 2017

frederikprijck commented Dec 19, 2017

frederikprijck commented Dec 19, 2017

gkalpak left a comment

gkalpak Dec 20, 2017

gkalpak Dec 20, 2017

gkalpak Dec 20, 2017

frederikprijck Dec 20, 2017 •

edited

Loading

Narretz commented Jan 10, 2018

feat(orderBy): Add null and undefined at the end of the sorted list #16376

feat(orderBy): Add null and undefined at the end of the sorted list #16376

Conversation

frederikprijck commented Dec 17, 2017 • edited Loading

frederikprijck commented Dec 18, 2017 • edited Loading

Narretz commented Dec 18, 2017

frederikprijck commented Dec 18, 2017

gkalpak commented Dec 18, 2017

frederikprijck commented Dec 18, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikprijck Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Dec 19, 2017

frederikprijck commented Dec 19, 2017

frederikprijck commented Dec 19, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederikprijck Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Narretz commented Jan 10, 2018

feat(orderBy): Add `null` and `undefined` at the end of the sorted list #16376

feat(orderBy): Add `null` and `undefined` at the end of the sorted list #16376

frederikprijck commented Dec 17, 2017 •

edited

Loading

frederikprijck commented Dec 18, 2017 •

edited

Loading

frederikprijck Dec 19, 2017 •

edited

Loading

frederikprijck Dec 20, 2017 •

edited

Loading