fix: some filters on undefined variable throws, #140

Author: harttle

README.md

@@ -31,7 +31,7 @@ though there are still some differences:
 
 * Dynamic file locating (enabled by default), that means layout/partial names are treated as variables in liquidjs. See [#51](https://github.com/harttle/liquidjs/issues/51).
 * Truthy and Falsy. All values except `undefined`, `null`, `false` are truthy, whereas in Ruby Liquid all except `nil` and `false` are truthy. See [#26](https://github.com/harttle/liquidjs/pull/26).
-* Number Rendering. Since JavaScript do not distinguish `float` and `integer`, we cannot either convert between them nor render regarding to their type. See [#59](https://github.com/harttle/liquidjs/issues/59).
+* 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()`
 

src/builtin/filters/array.ts

@@ -1,28 +1,42 @@
-import { last } from '../../util/underscore'
+import { isArray, last } from '../../util/underscore'
 import { isTruthy } from '../../render/syntax'
 
 export default {
   'join': (v: any[], arg: string) => v.join(arg === undefined ? ' ' : arg),
-  'last': <T>(v: T[]): T => last(v),
-  'first': <T>(v: T[]): T => v[0],
-  'map': <T1, T2>(arr: {[key: string]: T1}[], arg: string): T1[] => arr.map(v => v[arg]),
+  'last': (v: any) => isArray(v) ? last(v) : '',
+  'first': (v: any) => isArray(v) ? v[0] : '',
+  'map': map,
   'reverse': (v: any[]) => [...v].reverse(),
   'sort': <T>(v: T[], arg: (lhs: T, rhs: T) => number) => v.sort(arg),
-  'size': (v: string | any[]) => v.length,
-  'concat': <T1, T2>(v: T1[], arg: T2[] | T2): (T1 | T2)[] => Array.prototype.concat.call(v, arg),
-  'slice': <T>(v: T[], begin: number, length: number = 1): T[] => {
-    begin = begin < 0 ? v.length + begin : begin
-    return v.slice(begin, begin + length)
-  },
-  'uniq': function<T> (arr: T[]): T[] {
-    const u = {}
-    return (arr || []).filter(val => {
-      if (u.hasOwnProperty(String(val))) return false
-      u[String(val)] = true
-      return true
-    })
-  },
-  'where': function<T> (arr: T[], property: string, value?: any): T[] {
-    return arr.filter(obj => value === undefined ? isTruthy(obj[property]) : obj[property] === value)
-  }
+  'size': (v: string | any[]) => (v && v.length) || 0,
+  'concat': concat,
+  'slice': slice,
+  'uniq': uniq,
+  'where': where
+}
+
+function map<T1, T2> (arr: {[key: string]: T1}[], arg: string): T1[] {
+  return arr.map(v => v[arg])
+}
+
+function concat<T1, T2> (v: T1[], arg: T2[] | T2): (T1 | T2)[] {
+  return Array.prototype.concat.call(v, arg)
+}
+
+function slice<T> (v: T[], begin: number, length: number = 1): T[] {
+  begin = begin < 0 ? v.length + begin : begin
+  return v.slice(begin, begin + length)
+}
+
+function where<T> (arr: T[], property: string, value?: any): T[] {
+  return arr.filter(obj => value === undefined ? isTruthy(obj[property]) : obj[property] === value)
+}
+
+function uniq<T> (arr: T[]): T[] {
+  const u = {}
+  return (arr || []).filter(val => {
+    if (u.hasOwnProperty(String(val))) return false
+    u[String(val)] = true
+    return true
+  })
 }

src/builtin/filters/string.ts

@@ -1,28 +1,51 @@
+/**
+ * String related filters
+ *
+ * * prefer stringify() to String() since `undefined`, `null` should eval ''
+ */
+import { stringify } from '../../util/underscore'
+
 export default {
-  'append': (v: string, arg: string) => v + arg,
-  'prepend': (v: string, arg: string) => arg + v,
-  'capitalize': (str: string) => String(str).charAt(0).toUpperCase() + str.slice(1),
-  'lstrip': (v: string) => String(v).replace(/^\s+/, ''),
-  'downcase': (v: string) => v.toLowerCase(),
-  'upcase': (str: string) => String(str).toUpperCase(),
-  'remove': (v: string, arg: string) => v.split(arg).join(''),
-  'remove_first': (v: string, l: string) => v.replace(l, ''),
-  'replace': (v: string, pattern: string, replacement: string) =>
-    String(v).split(pattern).join(replacement),
-  'replace_first': (v: string, arg1: string, arg2: string) => String(v).replace(arg1, arg2),
-  'rstrip': (str: string) => String(str).replace(/\s+$/, ''),
-  'split': (v: string, arg: string) => String(v).split(arg),
-  'strip': (v: string) => String(v).trim(),
-  'strip_newlines': (v: string) => String(v).replace(/\n/g, ''),
-  'truncate': (v: string, l: number = 50, o: string = '...') => {
-    v = String(v)
-    if (v.length <= l) return v
-    return v.substr(0, l - o.length) + o
-  },
-  'truncatewords': (v: string, l: number = 15, o: string = '...') => {
-    const arr = v.split(/\s+/)
-    let ret = arr.slice(0, l).join(' ')
-    if (arr.length >= l) ret += o
-    return ret
-  }
+  'append': (v: string, arg: string) => stringify(v) + arg,
+  'prepend': (v: string, arg: string) => arg + stringify(v),
+  'capitalize': capitalize,
+  'lstrip': (v: string) => stringify(v).replace(/^\s+/, ''),
+  'downcase': (v: string) => stringify(v).toLowerCase(),
+  'upcase': (str: string) => stringify(str).toUpperCase(),
+  'remove': (v: string, arg: string) => stringify(v).split(arg).join(''),
+  'remove_first': (v: string, l: string) => stringify(v).replace(l, ''),
+  'replace': replace,
+  'replace_first': replaceFirst,
+  'rstrip': (str: string) => stringify(str).replace(/\s+$/, ''),
+  'split': (v: string, arg: string) => stringify(v).split(arg),
+  'strip': (v: string) => stringify(v).trim(),
+  'strip_newlines': (v: string) => stringify(v).replace(/\n/g, ''),
+  'truncate': truncate,
+  'truncatewords': truncateWords
+}
+
+function capitalize (str: string) {
+  str = stringify(str)
+  return str.charAt(0).toUpperCase() + str.slice(1)
+}
+
+function replace (v: string, pattern: string, replacement: string) {
+  return stringify(v).split(pattern).join(replacement)
+}
+
+function replaceFirst (v: string, arg1: string, arg2: string) {
+  return stringify(v).replace(arg1, arg2)
+}
+
+function truncate (v: string, l: number = 50, o: string = '...') {
+  v = stringify(v)
+  if (v.length <= l) return v
+  return v.substr(0, l - o.length) + o
+}
+
+function truncateWords (v: string, l: number = 15, o: string = '...') {
+  const arr = v.split(/\s+/)
+  let ret = arr.slice(0, l).join(' ')
+  if (arr.length >= l) ret += o
+  return ret
 }

src/util/underscore.ts

@@ -28,9 +28,8 @@ export function promisify (fn: any) {
 }
 
 export function stringify (value: any): string {
-  if (isNil(value)) return ''
-  value = toLiquid(value)
-  return String(value)
+  value = toValue(value)
+  return isNil(value) ? '' : String(value)
 }
 
 export function toValue (value: any): any {
@@ -42,7 +41,7 @@ export function isNumber (value: any): value is number {
 }
 
 export function toLiquid (value: any): any {
-  if (isFunction(value.toLiquid)) return toLiquid(value.toLiquid())
+  if (value && isFunction(value.toLiquid)) return toLiquid(value.toLiquid())
   return value
 }
 

test/integration/builtin/filters/array.ts

@@ -40,17 +40,85 @@ describe('filters/array', function () {
     })
   })
   describe('size', function () {
-    it('should return string length',
-      () => test('{{ "Ground control to Major Tom." | size }}', '28'))
-    it('should return array size', function () {
-      return test('{% assign my_array = "apples, oranges, peaches, plums"' +
-                ' | split: ", " %}{{ my_array | size }}',
-      '4')
-    })
-    it('should be respected with <string>.size notation',
-      () => test('{% assign my_string = "Ground control to Major Tom." %}{{ my_string.size }}', '28'))
-    it('should be respected with <array>.size notation',
-      () => test('{% assign my_array = "apples, oranges, peaches, plums" | split: ", " %}{{ my_array.size }}', '4'))
+    it('should return string length', async () => {
+      const html = await liquid.parseAndRender('{{ "Ground control to Major Tom." | size }}')
+      expect(html).to.equal('28')
+    })
+    it('should return array size', async () => {
+      const html = await liquid.parseAndRender(
+        '{% assign my_array = "apples, oranges, peaches, plums" | split: ", " %}{{ my_array | size }}')
+      expect(html).to.equal('4')
+    })
+    it('should be respected with <string>.size notation', async () => {
+      const html = await liquid.parseAndRender('{% assign my_string = "Ground control to Major Tom." %}{{ my_string.size }}')
+      expect(html).to.equal('28')
+    })
+    it('should be respected with <array>.size notation', async () => {
+      const html = await liquid.parseAndRender('{% assign my_array = "apples, oranges, peaches, plums" | split: ", " %}{{ my_array.size }}')
+      expect(html).to.equal('4')
+    })
+    it('should return 0 for false', async () => {
+      const html = await liquid.parseAndRender('{{ false | size }}')
+      expect(html).to.equal('0')
+    })
+    it('should return 0 for nil', async () => {
+      const html = await liquid.parseAndRender('{{ nil | size }}')
+      expect(html).to.equal('0')
+    })
+    it('should return 0 for undefined', async () => {
+      const html = await liquid.parseAndRender('{{ foo | size }}')
+      expect(html).to.equal('0')
+    })
+  })
+  describe('first', function () {
+    it('should support first', async () => {
+      const html = await liquid.parseAndRender(
+        '{{arr | first}}',
+        { arr: [ 'zebra', 'tiger' ] }
+      )
+      expect(html).to.equal('zebra')
+    })
+    it('should return empty for nil', async () => {
+      const html = await liquid.parseAndRender('{{nil | first}}')
+      expect(html).to.equal('')
+    })
+    it('should return empty for undefined', async () => {
+      const html = await liquid.parseAndRender('{{foo | first}}')
+      expect(html).to.equal('')
+    })
+    it('should return empty for false', async () => {
+      const html = await liquid.parseAndRender('{{false | first}}')
+      expect(html).to.equal('')
+    })
+    it('should return empty for string', async () => {
+      const html = await liquid.parseAndRender('{{"zebra" | first}}')
+      expect(html).to.equal('')
+    })
+  })
+  describe('last', function () {
+    it('should support last', async () => {
+      const html = await liquid.parseAndRender(
+        '{{arr | last}}',
+        { arr: [ 'zebra', 'tiger' ] }
+      )
+      expect(html).to.equal('tiger')
+    })
+    it('should return empty for nil', async () => {
+      const html = await liquid.parseAndRender('{{nil | last}}')
+      expect(html).to.equal('')
+    })
+    it('should return empty for undefined', async () => {
+      const html = await liquid.parseAndRender('{{foo | last}}')
+      expect(html).to.equal('')
+    })
+    it('should return empty for false', async () => {
+      const html = await liquid.parseAndRender('{{false | last}}')
+      expect(html).to.equal('')
+    })
+    it('should return empty for string', async () => {
+      const html = await liquid.parseAndRender('{{"zebra" | last}}')
+      expect(html).to.equal('')
+    })
   })
   describe('slice', function () {
     it('should slice first char by 0', () => test('{{ "Liquid" | slice: 0 }}', 'L'))

test/integration/builtin/filters/math.ts

@@ -60,6 +60,10 @@ describe('filters/math', function () {
       () => test('{{ 183.357 | plus: 12 }}', '195.357'))
     it('should convert first arg as number', () => test('{{ "4" | plus: 2 }}', '6'))
     it('should convert both args as number', () => test('{{ "4" | plus: "2" }}', '6'))
+    it('should support variable', async () => {
+      const html = await l.parseAndRender('{{ 4 | plus: b }}', { b: 2 })
+      expect(html).to.equal('6')
+    })
   })
 
   describe('sort_natural', function () {

test/integration/builtin/filters/string.ts

@@ -1,13 +1,30 @@
 import { test } from '../../../stub/render'
+import Liquid from '../../../../src/liquid'
+import { expect } from 'chai'
 
 describe('filters/string', function () {
+  let liquid: Liquid
+  beforeEach(function () {
+    liquid = new Liquid()
+  })
   describe('append', function () {
     it('should return "-3abc" for -3, "abc"',
       () => test('{{ -3 | append: "abc" }}', '-3abc'))
     it('should return "abar" for "a",foo', () => test('{{ "a" | append: foo }}', 'abar'))
   })
   describe('capitalize', function () {
-    it('should capitalize first', () => test('{{ "i am good" | capitalize }}', 'I am good'))
+    it('should capitalize first', async () => {
+      const html = await liquid.parseAndRender('{{ "i am good" | capitalize }}')
+      expect(html).to.equal('I am good')
+    })
+    it('should return empty for nil', async () => {
+      const html = await liquid.parseAndRender('{{ nil | capitalize }}')
+      expect(html).to.equal('')
+    })
+    it('should return empty for undefined', async () => {
+      const html = await liquid.parseAndRender('{{ foo | capitalize }}')
+      expect(html).to.equal('')
+    })
   })
   describe('concat', function () {
     it('should concat arrays', () => test(`
@@ -45,10 +62,18 @@ describe('filters/string', function () {
       `))
   })
   describe('downcase', function () {
-    it('should return "parker moore" for "Parker Moore"',
-      () => test('{{ "Parker Moore" | downcase }}', 'parker moore'))
-    it('should return "apple" for "apple"',
-      () => test('{{ "apple" | downcase }}', 'apple'))
+    it('should return "parker moore" for "Parker Moore"', async () => {
+      const html = await liquid.parseAndRender('{{ "Parker Moore" | downcase }}')
+      expect(html).to.equal('parker moore')
+    })
+    it('should return "apple" for "apple"', async () => {
+      const html = await liquid.parseAndRender('{{ "apple" | downcase }}')
+      expect(html).to.equal('apple')
+    })
+    it('should return empty for undefined', async () => {
+      const html = await liquid.parseAndRender('{{ foo | downcase }}')
+      expect(html).to.equal('')
+    })
   })
   describe('split', function () {
     it('should support split/first', function () {
@@ -57,7 +82,16 @@ describe('filters/string', function () {
       return test(src, 'apples')
     })
   })
-  it('should support upcase', () => test('{{ "Parker Moore" | upcase }}', 'PARKER MOORE'))
+  describe('upcase', function () {
+    it('should support upcase', async () => {
+      const html = await liquid.parseAndRender('{{ "Parker Moore" | upcase }}')
+      expect(html).to.equal('PARKER MOORE')
+    })
+    it('should return empty for undefined', async () => {
+      const html = await liquid.parseAndRender('{{ foo | upcase }}')
+      expect(html).to.equal('')
+    })
+  })
   it('should support lstrip', function () {
     const src = '{{ "          So much room for activities!          " | lstrip }}'
     return test(src, 'So much room for activities!          ')
@@ -78,13 +112,25 @@ describe('filters/string', function () {
             '{{ "/index.html" | prepend: url }}',
     'liquidmarkup.com/index.html')
   })
-  it('should support remove', function () {
-    return test('{{ "I strained to see the train through the rain" | remove: "rain" }}',
-      'I sted to see the t through the ')
+  describe('remove', function () {
+    it('should support remove', async () => {
+      const html = await liquid.parseAndRender('{{ "I strained to see the train through the rain" | remove: "rain" }}')
+      expect(html).to.equal('I sted to see the t through the ')
+    })
+    it('should return empty for undefined', async () => {
+      const html = await liquid.parseAndRender('{{ foo | remove: "rain" }}')
+      expect(html).to.equal('')
+    })
   })
-  it('should support remove_first', function () {
-    return test('{{ "I strained to see the train through the rain" | remove_first: "rain" }}',
-      'I sted to see the train through the rain')
+  describe('remove_first', function () {
+    it('should support remove_first', async () => {
+      const html = await liquid.parseAndRender('{{ "I strained to see the train through the rain" | remove_first: "rain" }}')
+      expect(html).to.equal('I sted to see the train through the rain')
+    })
+    it('should return empty for undefined', async () => {
+      const html = await liquid.parseAndRender('{{ foo | remove_first: "r" }}')
+      expect(html).to.equal('')
+    })
   })
   it('should support replace', function () {
     return test('{{ "Take my protein pills and put my helmet on" | replace: "my", "your" }}',

test/unit/util/underscore.ts

@@ -27,12 +27,6 @@ describe('util/underscore', function () {
     })
   })
   describe('.stringify()', function () {
-    it('should respect to toLiquid() method', function () {
-      expect(_.stringify({ toLiquid: () => 'foo' })).to.equal('foo')
-    })
-    it('should recursively call toLiquid()', function () {
-      expect(_.stringify({ toLiquid: () => ({ toLiquid: () => 'foo' }) })).to.equal('foo')
-    })
     it('should return "" for null', function () {
       expect(_.stringify(null)).to.equal('')
     })