fix: incorrect scope when using assign with for, fixes #115

* remove AssignScope, CaptureScope, IncrementScope, DecrementScope concepts
* introduce Scope.environments for decrement/increment
* assign to scopes[0] for assign/capture

src/builtin/tags/assign.ts

@@ -1,6 +1,5 @@
 import assert from '../../util/assert'
 import { identifier } from '../../parser/lexical'
-import { AssignScope } from '../../scope/scopes'
 import TagToken from '../../parser/tag-token'
 import Scope from '../../scope/scope'
 import ITagImplOptions from '../../template/tag/itag-impl-options'
@@ -15,8 +14,6 @@ export default {
     this.value = match[2]
   },
   render: async function (scope: Scope) {
-    const ctx = new AssignScope()
-    ctx[this.key] = await this.liquid.evalValue(this.value, scope)
-    scope.push(ctx)
+    scope.contexts[0][this.key] = await this.liquid.evalValue(this.value, scope)
   }
 } as ITagImplOptions

src/builtin/tags/capture.ts

@@ -1,6 +1,5 @@
 import assert from '../../util/assert'
 import { identifier } from '../../parser/lexical'
-import { CaptureScope } from '../../scope/scopes'
 import TagToken from '../../parser/tag-token'
 import Token from '../../parser/token'
 import Scope from '../../scope/scope'
@@ -26,8 +25,6 @@ export default {
   },
   render: async function (scope: Scope) {
     const html = await this.liquid.renderer.renderTemplates(this.templates, scope)
-    const ctx = new CaptureScope()
-    ctx[this.variable] = html
-    scope.push(ctx)
+    scope.contexts[0][this.variable] = html
   }
 } as ITagImplOptions

src/builtin/tags/decrement.ts

@@ -1,6 +1,5 @@
 import assert from '../../util/assert'
 import { identifier } from '../../parser/lexical'
-import { CaptureScope, AssignScope, DecrementScope } from '../../scope/scopes'
 import TagToken from '../../parser/tag-token'
 import Scope from '../../scope/scope'
 import ITagImplOptions from '../../template/tag/itag-impl-options'
@@ -11,20 +10,11 @@ export default {
     assert(match, `illegal identifier ${token.args}`)
     this.variable = match[0]
   },
-  render: function (scope: Scope) {
-    let context = scope.findContextFor(
-      this.variable,
-      ctx => {
-        return !(ctx instanceof CaptureScope) && !(ctx instanceof AssignScope)
-      }
-    )
-    if (!context) {
-      context = new DecrementScope()
-      scope.unshift(context)
+  render: function (context: Scope) {
+    const scope = context.environments
+    if (typeof scope[this.variable] !== 'number') {
+      scope[this.variable] = 0
     }
-    if (typeof context[this.variable] !== 'number') {
-      context[this.variable] = 0
-    }
-    return --context[this.variable]
+    return --scope[this.variable]
   }
 } as ITagImplOptions

src/builtin/tags/increment.ts

@@ -1,6 +1,5 @@
 import assert from '../../util/assert'
 import { identifier } from '../../parser/lexical'
-import { CaptureScope, AssignScope, IncrementScope } from '../../scope/scopes'
 import ITagImplOptions from '../../template/tag/itag-impl-options'
 
 export default {
@@ -9,22 +8,13 @@ export default {
     assert(match, `illegal identifier ${token.args}`)
     this.variable = match![0]
   },
-  render: function (scope) {
-    let context = scope.findContextFor(
-      this.variable,
-      ctx => {
-        return !(ctx instanceof CaptureScope) && !(ctx instanceof AssignScope)
-      }
-    )
-    if (!context) {
-      context = new IncrementScope()
-      scope.unshift(context)
+  render: function (context) {
+    const scope = context.environments
+    if (typeof scope[this.variable] !== 'number') {
+      scope[this.variable] = 0
     }
-    if (typeof context[this.variable] !== 'number') {
-      context[this.variable] = 0
-    }
-    const val = context[this.variable]
-    context[this.variable]++
+    const val = scope[this.variable]
+    scope[this.variable]++
     return val
   }
 } as ITagImplOptions

src/scope/scope.ts

@@ -8,20 +8,22 @@ import { Context } from './context'
 
 export default class Scope {
   opts: NormalizedFullOptions
-  contexts: Array<Context>
+  contexts: Array<Context> = [{}]
+  environments: Context
   blocks: object = {}
   groups: {[key: string]: number} = {}
   blockMode: BlockMode = BlockMode.OUTPUT
   constructor (ctx: object = {}, opts?: NormalizedFullOptions) {
     this.opts = applyDefault(opts)
-    this.contexts = [ctx || {}]
+    this.environments = ctx
   }
   getAll () {
-    return this.contexts.reduce((ctx, val) => __assign(ctx, val), {})
+    return [this.environments, ...this.contexts]
+      .reduce((ctx, val) => __assign(ctx, val), {})
   }
   async get (path: string) {
     const paths = await this.propertyAccessSeq(path)
-    let ctx = this.findContextFor(paths[0]) || _.last(this.contexts)
+    let ctx = this.findContextFor(paths[0]) || this.environments
     for (const path of paths) {
       ctx = this.readProperty(ctx, path)
       if (_.isNil(ctx) && this.opts.strictVariables) {
@@ -30,27 +32,6 @@ export default class Scope {
     }
     return ctx
   }
-  async set (path: string, v: any) {
-    const paths = await this.propertyAccessSeq(path)
-    let scope = this.findContextFor(paths[0]) || _.last(this.contexts)
-    paths.some((key, i) => {
-      if (!_.isObject(scope)) {
-        return true
-      }
-      if (i === paths.length - 1) {
-        scope[key] = v
-        return true
-      }
-      if (undefined === scope[key]) {
-        scope[key] = {}
-      }
-      scope = scope[key]
-      return false
-    })
-  }
-  unshift (ctx: object) {
-    return this.contexts.unshift(ctx)
-  }
   push (ctx: object) {
     return this.contexts.push(ctx)
   }
@@ -64,10 +45,9 @@ export default class Scope {
     }
     return this.contexts.splice(i, 1)[0]
   }
-  findContextFor (key: string, filter: ((conttext: object) => boolean) = () => true) {
+  findContextFor (key: string) {
     for (let i = this.contexts.length - 1; i >= 0; i--) {
       const candidate = this.contexts[i]
-      if (!filter(candidate)) continue
       if (key in candidate) {
         return candidate
       }

src/types.ts

@@ -1,3 +1,2 @@
-export { AssignScope, CaptureScope, IncrementScope, DecrementScope } from './scope/scopes'
 export { ParseError, TokenizationError, RenderBreakError, AssertionError } from './util/error'
 export { Drop } from './drop/drop'

test/integration/builtin/tags/assign.ts

@@ -21,13 +21,6 @@ describe('tags/assign', function () {
     const html = await liquid.parseAndRender(src)
     return expect(html).to.equal('10086')
   })
-  it('should shading rather than overwriting', async function () {
-    const ctx = { foo: 'foo' }
-    const src = '{% assign foo="FOO" %}{{foo}}'
-    const html = await liquid.parseAndRender(src, ctx)
-    expect(html).to.equal('FOO')
-    expect(ctx.foo).to.equal('foo')
-  })
   it('should assign as array', async function () {
     const src = '{% assign foo=(1..3) %}{{foo}}'
     const html = await liquid.parseAndRender(src)
@@ -76,4 +69,16 @@ describe('tags/assign', function () {
     const html = await liquid.parseAndRender(src)
     return expect(html).to.equal('-6')
   })
+  describe('scope', function () {
+    it('should read from parent scope', async function () {
+      const src = '{%for a in (1..2)%}{{num}}{%endfor%}'
+      const html = await liquid.parseAndRender(src, { num: 1 })
+      return expect(html).to.equal('11')
+    })
+    it('should write to the belonging scope', async function () {
+      const src = '{%for a in (1..2)%}{%assign num = a%}{{a}}{%endfor%} {{num}}'
+      const html = await liquid.parseAndRender(src, { num: 1 })
+      return expect(html).to.equal('12 2')
+    })
+  })
 })

test/integration/builtin/tags/for.ts

@@ -39,19 +39,6 @@ describe('tags/for', function () {
     const html = await liquid.parseAndRender(src, ctx)
     return expect(html).to.equal('{"i":0,"length":1}')
   })
-  describe('scope', function () {
-    it('should read super scope', async function () {
-      const src = '{%for a in (1..2)%}{{num}}{%endfor%}'
-      const html = await liquid.parseAndRender(src, { num: 1 })
-      return expect(html).to.equal('11')
-    })
-    it('should write super scope', async function () {
-      const src = '{%for a in (1..2)%}{{num}}{%assign num = 2%}{%endfor%}'
-      const html = await liquid.parseAndRender(src, { num: 1 })
-      return expect(html).to.equal('12')
-    })
-  })
-
   describe('illegal', function () {
     it('should reject when for not closed', function () {
       const src = '{%for c in alpha%}{{c}}'

test/unit/scope/scope.ts

@@ -126,41 +126,6 @@ describe('scope', function () {
       expect(await scope.get('one.size')).to.equal(undefined)
     })
   })
-
-  describe('#set', function () {
-    it('should set nested value', async function () {
-      await scope.set('posts', {
-        'first': {
-          'name': 'A Nice Day'
-        }
-      })
-      await scope.set('category', {
-        'diary': ['first']
-      })
-      expect(await scope.get('posts[category.diary[0]].name'), 'A Nice Day')
-    })
-
-    it('should create parent if needed', async function () {
-      await scope.set('a.b.c.d', 'COO')
-      expect(await scope.get('a.b.c.d')).to.equal('COO')
-    })
-    it('should keep other properties of parent', async function () {
-      scope.push({ obj: { foo: 'FOO' } })
-      await scope.set('obj.bar', 'BAR')
-      expect(await scope.get('obj.foo')).to.equal('FOO')
-    })
-    it('should abort if property cannot be set', async function () {
-      scope.push({ obj: { foo: 'FOO' } })
-      await scope.set('obj.foo.bar', 'BAR')
-      expect(await scope.get('obj.foo')).to.equal('FOO')
-    })
-    it("should set parents' corresponding value", async function () {
-      scope.push({})
-      await scope.set('foo', 'bar')
-      scope.pop()
-      expect(await scope.get('foo')).to.equal('bar')
-    })
-  })
   describe('strictVariables', async function () {
     let scope: Scope
     beforeEach(function () {
@@ -172,15 +137,15 @@ describe('scope', function () {
       return expect(scope.get('notdefined')).to.be.rejectedWith(/undefined variable: notdefined/)
     })
     it('should throw when deep variable not exist', async function () {
-      await scope.set('foo', 'FOO')
+      scope.contexts.push({ 'foo': 'FOO' })
       return expect(scope.get('foo.bar.not.defined')).to.be.rejectedWith(/undefined variable: bar/)
     })
     it('should throw when itself not defined', async function () {
-      await scope.set('foo', 'bar')
+      scope.contexts.push({ 'foo': 'FOO' })
       return expect(scope.get('foo.BAR')).to.be.rejectedWith(/undefined variable: BAR/)
     })
     it('should find variable in parent scope', async function () {
-      await scope.set('foo', 'foo')
+      scope.contexts.push({ 'foo': 'foo' })
       scope.push({
         'bar': 'bar'
       })
@@ -196,15 +161,15 @@ describe('scope', function () {
 
   describe('.push()', function () {
     it('should push scope', async function () {
-      await scope.set('bar', 'bar')
+      scope.contexts.push({ 'bar': 'bar' })
       scope.push({
         foo: 'foo'
       })
       expect(await scope.get('foo')).to.equal('foo')
       expect(await scope.get('bar')).to.equal('bar')
     })
     it('should hide deep properties by push', async function () {
-      await scope.set('bar', { bar: 'bar' })
+      scope.contexts.push({ 'bar': { bar: 'bar' } })
       scope.push({ bar: { foo: 'foo' } })
       expect(await scope.get('bar.foo')).to.equal('foo')
       expect(await scope.get('bar.bar')).to.equal(undefined)