Unable to reassign include arguments locally within the include file itself

[AleksandrHovhannisyan]

Related 11ty issue: 11ty/eleventy#2000

Steps to reproduce:

1. Create an include that accepts one argument.
2. Reassign the argument locally within the include.
3. Render the argument.
4. Invoke the include and specify a value for the argument.
5. Observe that the value does not get overridden.

Example:

parent.html

{% include child.html color: "red" %}

child.html

{% assign color = "green" %}{{ color }}

Expected behavior: the output is green.

Observed behavior: the output is red.

If it's okay, I'd like to take a crack at fixing this (if you think it's realistic for a first-time contributor). I have a failing test locally to confirm, but I'll need to figure out where in the code it's breaking.

[harttle]

I see the current implementation is

  render: function * (ctx: Context) {
    ctx.bottom()[this.key] = yield this.liquid._evalValue(this.value, ctx)
  }

It's assigning to the first scope in context stack. The idea is to skip transient scopes like the one created by for. Maybe we need mark which scope is a page-level one, and find the topmost page-level scope instead of scopes[0]. And I think the scope created by include, render, layout should be considered page-level.

Feel free to post back if you're stuck. I can help or continue your work!

[AleksandrHovhannisyan]

@harttle Per your suggestion, ee830f9 seems to fix the previously failing test I wrote here: AleksandrHovhannisyan@ee830f9#diff-144b633c9a17150f1e3fc00aebe1d21d22c09b622b0f8f81b457a718282842b9R197

However, it also causes 5 other tests to fail. Specifically, these ones:

 tags/render
    10) should support for...as
    11) should support for <non-array> as
    12) should support for without as
    13) should support for...as with other parameters
    14) should support for...as with other parameters (comma separated)

With these results:

10) tags/render
       should support for...as:

      AssertionError: expected ': \n: \n' to equal '1: red\n2: green\n'
      + expected - actual

      -:
      -:
      +1: red
      +2: green

      at Context.<anonymous> (test/integration/builtin/tags/render.ts:146:21)
      at Generator.next (<anonymous>)
      at fulfilled (test/integration/builtin/tags/render.ts:4:58)

  11) tags/render
       should support for <non-array> as:

      AssertionError: expected ': \n' to equal '1: green\n'
      + expected - actual

      -:
      +1: green

      at Context.<anonymous> (test/integration/builtin/tags/render.ts:154:21)
      at Generator.next (<anonymous>)
      at fulfilled (test/integration/builtin/tags/render.ts:4:58)

  12) tags/render
       should support for without as:

      AssertionError: expected ': \n: \n' to equal '1: \n2: \n'
      + expected - actual

      -:
      -:
      +1:
      +2:

      at Context.<anonymous> (test/integration/builtin/tags/render.ts:162:21)
      at Generator.next (<anonymous>)
      at fulfilled (test/integration/builtin/tags/render.ts:4:58)

  13) tags/render
       should support for...as with other parameters:

      AssertionError: expected '. .\n. .\n' to equal '1. red.\n2. green.\n'
      + expected - actual

      -. .
      -. .
      +1. red.
      +2. green.

      at Context.<anonymous> (test/integration/builtin/tags/render.ts:170:21)
      at Generator.next (<anonymous>)
      at fulfilled (test/integration/builtin/tags/render.ts:4:58)

  14) tags/render
       should support for...as with other parameters (comma separated):

      AssertionError: expected '. .\n. .\n' to equal '1. red.\n2. green.\n'
      + expected - actual

      -. .
      -. .
      +1. red.
      +2. green.

Any idea what I may have done wrong? The tests pass if I revert this line: https://github.com/AleksandrHovhannisyan/liquidjs/blob/reassignment/src/builtin/tags/render.ts#L59

[AleksandrHovhannisyan]

Hi @harttle! Just following up to see if you had a chance to take a look at my branch. Let me know if you want me to clarify any changes I made or if I misunderstood the requirements. Thanks!

[harttle]

Ah, I guess you'll need to call ctx.pop() when finished rendering. push() and pop() need to be used in pairs, otherwise it'll get into a messy state.

[AleksandrHovhannisyan]

Hi @harttle, thanks for getting back and clarifying! Unfortunately, I don't think I'll be able to tackle this ticket. Please feel free to assign it to someone else or continue where I left off.

(As an aside: I think it may be worth documenting some of the code, with inline comments or jsDoc, to make it easier for others to contribute. This was my biggest pain point.)

[harttle]

@AleksandrHovhannisyan feel free to create a pull request (even incomplete), I can try to continue your work. At least I also think it's a bug worth fixing.

[harttle]

After digging into Liquid and LiquidJS (I wrote this part long time ago and it takes me quite a while to figure it out, sorry for this), I find that:

include and layout are legacy tags which does not provide Context separation. In both Shopify/Liquid and LiquidJS, {% assgin %} in {%include%} files assigns to its parent template, thus will not hide include arguments. So it's by design and that's why {%include%} tag is deprecated.

render enforces Context separation by creating a new Context when render partials, but it has a bug resulting in {% assign %} inside {%render%} also not hiding render arguments. It's definitely a bug and not consistent with Shopify/Liquid, I updated @AleksandrHovhannisyan 's PR and fixed this bug on v9.28.4, check it out!

layout is implemented just like include and still not works for now, I'll consider provide a substitution for layout too if someone actually needs that. Feel free to open a feature request issue for it!

[AleksandrHovhannisyan]

@harttle Sounds good, thank you!

Since 11ty uses include and not render, does this mean that the bug won't be fixed for us unless we upgrade to the latest version? I'm specifically wondering about this test:

124f4c4#diff-d31563acb42db2c893c52aa0555c10ce781e05097b0b59cd5a403472972e7300R91

[pdehaan]

@AleksandrHovhannisyan I'm pretty sure 11ty v1 (beta) supports the render tag as well (although it's been a while since I've played with that).

Although I do see that https://github.com/11ty/eleventy/blob/19b4884e83e420a3b42f334aa922140f9cb18353/package.json#L107 is pinned to a very specific version of liquidjs, so we might need to tweak that line from the 11ty side (or else you can try manually installing the latest liquidjs locally and then try setting your own custom library instance.

---

UPDATE: Filed 11ty/eleventy#2059 from the 11ty side to bump liquidjs version back to latest.

[AleksandrHovhannisyan]

@pdehaan Gotcha—yeah, Zach mentioned that even if this fix got in, we wouldn't be able to take advantage of it yet due to this regression, which is pinning liquid to 9.25.x: 11ty/eleventy#1995.