fix: stable sort for undefined keys, fixes #191

README.md

@@ -62,6 +62,8 @@ For detailed documents, see:
 * Number. In JavaScript we cannot distinguish or convert between `float` and `integer`, see [#59](https://github.com/harttle/liquidjs/issues/59). And when applied `size` filter, numbers always return 0, which is 8 for integer in ruby, cause they do not have a `length` property.
 * [.to_liquid()](https://github.com/Shopify/liquid/wiki/Introduction-to-Drops) is replaced by `.toLiquid()`
 * [.to_s()](https://www.rubydoc.info/gems/liquid/Liquid/Drop) is replaced by JavaScript `.toString()`
+* Iteration order for objects. The iteration order of JavaScript objects, and thus LiquidJS objects, is a combination of the insertion order for string keys, and ascending order for number-like keys, while the iteration order of Ruby Hash is simply the insertion order.
+* Sort stability. The [sort](https://shopify.github.io/liquid/filters/sort/) stability is also not defined in both shopify/liquid and LiquidJS, but it's [considered stable](https://v8.dev/features/stable-sort) for LiquidJS in Node.js 12+ and Google Chrome 70+.
 
 Features that available on shopify website but not on shopify/liquid repo will not be implemented in this repo,
 but there're some plugins available: <https://github.com/harttle/liquidjs/wiki/Plugins>

src/builtin/filters/math.ts

@@ -1,4 +1,4 @@
-const toLowerCase = String.prototype.toLowerCase
+import { caseInsensitiveCompare } from '../../util/underscore'
 
 export default {
   'abs': (v: number) => Math.abs(v),
@@ -18,22 +18,12 @@ export default {
   'times': (v: number, arg: number) => v * arg
 }
 
-function caseInsensitiveCmp (a: any, b: any) {
-  if (!b) return -1
-  if (!a) return 1
-  a = toLowerCase.call(a)
-  b = toLowerCase.call(b)
-  if (a < b) return -1
-  if (a > b) return 1
-  return 0
-}
-
 function sortNatural (input: any[], property?: string) {
   if (!input || !input.sort) return []
   if (property !== undefined) {
     return [...input].sort(
-      (lhs, rhs) => caseInsensitiveCmp(lhs[property], rhs[property])
+      (lhs, rhs) => caseInsensitiveCompare(lhs[property], rhs[property])
     )
   }
-  return [...input].sort(caseInsensitiveCmp)
+  return [...input].sort(caseInsensitiveCompare)
 }

src/util/underscore.ts

@@ -1,6 +1,7 @@
 import { Drop } from '../drop/drop'
 
 const toStr = Object.prototype.toString
+const toLowerCase = String.prototype.toLowerCase
 
 /*
  * Checks if value is classified as a String primitive or object.
@@ -127,3 +128,15 @@ export function changeCase (str: string): string {
 export function ellipsis (str: string, N: number): string {
   return str.length > N ? str.substr(0, N - 3) + '...' : str
 }
+
+// compare string in case-insensitive way, undefined values to the tail
+export function caseInsensitiveCompare (a: any, b: any) {
+  if (a == null && b == null) return 0
+  if (a == null) return 1
+  if (b == null) return -1
+  a = toLowerCase.call(a)
+  b = toLowerCase.call(b)
+  if (a < b) return -1
+  if (a > b) return 1
+  return 0
+}

test/integration/builtin/filters/math.ts

@@ -89,6 +89,19 @@ describe('filters/math', function () {
       const html = await l.parseAndRender(src, { students })
       expect(html).to.equal('bob alice carol')
     })
+    it('should be stable when it comes to undefined props', async () => {
+      const src = '{{ students | sort_natural: "age" | map: "name" | join }}'
+      const students = [
+        { name: 'bob' },
+        { name: 'alice', age: 2 },
+        { name: 'amber' },
+        { name: 'watson' },
+        { name: 'michael' },
+        { name: 'charlie' }
+      ]
+      const html = await l.parseAndRender(src, { students })
+      expect(html).to.equal('alice bob amber watson michael charlie')
+    })
     it('should tolerate undefined props', async () => {
       const src = '{{ students | sort_natural: "age" | map: "name" | join }}'
       const students = [

test/unit/util/underscore.ts

@@ -108,4 +108,21 @@ describe('util/underscore', function () {
       expect(_.changeCase('FOOA')).to.equal('fooa')
     })
   })
+  describe('.caseInsensitiveCompare()', function () {
+    it('should "foo" > "bar"', () => {
+      expect(_.caseInsensitiveCompare('foo', 'bar')).to.equal(1)
+    })
+    it('should "foo" < null', () => {
+      expect(_.caseInsensitiveCompare('foo', null)).to.equal(-1)
+    })
+    it('should null > "foo"', () => {
+      expect(_.caseInsensitiveCompare(null, 'foo')).to.equal(1)
+    })
+    it('should -1 < 0', () => {
+      expect(_.caseInsensitiveCompare(-1, 0)).to.equal(-1)
+    })
+    it('should 1 > 0', () => {
+      expect(_.caseInsensitiveCompare(1, 0)).to.equal(1)
+    })
+  })
 })