Fix sort-by with null values

[wongpeiyi]

e7a8f01 breaks sort-by when a value is null since null.toLowerCase throws the following:

Uncaught TypeError: Cannot read property 'toLowerCase' of null

I suppose the truthy checks were required after all to prevent that

---

I'm not sure what the ideal handling of null values should be, so this PR is just my take:

I believe null values should be sorted last as well since doing comparisons to null can lead to inconsistent sorting:

// Ignored (left in place) when compared to strings
"a" < null // false
"a" > null // false

// Sorted when compared to numbers
1 > null // true
1 < null // false

However, this doesn't sort between undefined and null -- not sure if that's a problem.

Also made a change to the 'ignores undefined values sorting' test case since undefined values should be expected to be sorted last now, not ignored. The way the test was written, it would never fail regardless of the position the undefined value was in

[snewcomer]

One behaviour of sort was to remove undefined values. We would probably want to keep this sort of test around.

[wongpeiyi]

Sorry, not sure I understand -- what do you mean by "remove undefined values"?

This merely changes the test to assert that the undefined value is at the end of the resulting array. At present the test will assert "abc" regardless of what position undefined is in the resulting array since undefined renders nothing, so textContent will be abc no matter what

You could change sort-by to sort undefined first, sort undefined last, remove undefineds or even randomly add a bunch of undefineds, and this test will always pass before this change, so it doesn't really test anything at all

[snewcomer]

See original assertion...

assert.equal(find('*').textContent.trim(), 'abc');

[wongpeiyi]

Sorry I've seen that and I still don't understand. I've been relooking the test over and over again, could you please elaborate in more detail?

If sort-by returns any of these results, textContent is still 'abc':

{{#each (array "a" "b" "c") as |str|}}{{str}}{{/each}}
textContent === 'abc'

{{#each (array "a" "b" undefined "c") as |str|}}{{str}}{{/each}}
textContent === 'abc'

{{#each (array undefined "a" undefined undefined "b" undefined "c" undefined) as |str|}}{{str}}{{/each}}
textContent === 'abc'

I'm not sure what the original assertion is testing for?

Put another way: what result of sort-by would cause this assertion to fail?

[snewcomer]

We simply want the original test around. The test assertions you added should be in a new test

[lukemelia]

Is there anything I can do to help get this merged? This is a production issue for us requiring us to stay locked to 4.2.0.

[snewcomer]

👍