Paginated layout removes arguments from liquid shortcode

[mahnunchik]

Describe the bug

Paginated layout removes arguments from liquid shortcode.

To Reproduce
Steps to reproduce the behavior:

Shortcode

  eleventyConfig.addShortcode('test', function (arg1, arg2, arg3) {
    console.log('test:', arg1, arg2, arg3);
    return `${arg1} - ${arg2} - ${arg3}`;
  });

Template test.liquid:

---
layout: main
pagination:
  data: 'testdata'
  size: 1
  addAllPagesToCollections: true
testdata:
  - item1
  - item2
  - item3
  - item4
---

{% test "file1" "file2" "file3" %}

Layout main.liquid:

{% test "main1" "main2" "main3" %}
{{ content }}

Expected behavior

All arguments passed to test shortcode in both cases: template and layout.

Actual behavior

Arguments are randomly removed from shortcode:

test: file1 file2 file3
test: file1 file2 file3
test: file1 file2 file3
test: file1 file2 file3
test: main1 undefined undefined
test: main1 undefined undefined
test: main1 main2 undefined
test: main1 main3 undefined

Environment:

• Eleventy Version 1.0.0-beta.X and 1.0.0-canary-X

Additional context

Maybe other versions of Eleventy and other template engines are affected.

[mahnunchik]

First argument is always present. Arguments from second are removed randomly.

[pdehaan]

Interesting. I can reproduce w/ @beta and @canary channel builds, but not 0.12.1/@latest (or beta/canary w/ Nunjucks). Possibly a LiquidJS regression between liquidjs@6 and liquidjs@9 (which I think is the version jump between eleventy@0.12 and eleventy@1).
/cc @harttle

> 11ty-2154@1.0.0 build /private/tmp/11ty-2154
> eleventy

www/test/index.html: file1 file2 file3
www/test/1/index.html: file1 file2 file3
www/test/2/index.html: file1 file2 file3
www/test/3/index.html: file1 file2 file3
www/test/2/index.html: main1 undefined undefined
www/test/3/index.html: main1 undefined undefined
www/test/index.html: main1 main2 undefined
www/test/1/index.html: main1 main3 undefined
[11ty] Writing www/test/2/index.html from ./src/test.liquid
[11ty] Writing www/test/3/index.html from ./src/test.liquid
[11ty] Writing www/test/index.html from ./src/test.liquid
[11ty] Writing www/test/1/index.html from ./src/test.liquid
[11ty] Wrote 4 files in 0.06 seconds (v1.0.0-canary.49)

Pushed sample repo to https://github.com/pdehaan/11ty-2154

  console.log(`@11ty/eleventy@${require("@11ty/eleventy/package.json").version}`);
  console.log(`liquidjs@${require("liquidjs/package.json").version}`);

  // @11ty/eleventy@1.0.0-canary.49
  // liquidjs@9.31.0

[harttle]

Hey @pdehaan, I don't think it's related to liquidjs but tonight I had time to look into it anyway and find it quite interesting.

I added some log to this file:

  static async parseArguments(lexer, str, scope, engine) {
    let argArray = [];
    let label = `[${id++}]`;

    if (!lexer) {
      lexer = moo.compile(Liquid.argumentLexerOptions);
    }

    console.log(label, "parsing arguments for string", str)
    if (typeof str === "string") {
      // TODO key=value key2=value
      // TODO JSON?
      lexer.reset(str);
      let arg = lexer.next();
      while (arg) {
        /*{
          type: 'doubleQuoteString',
          value: '"test 2"',
          text: '"test 2"',
          toString: [Function: tokenToString],
          offset: 0,
          lineBreaks: 0,
          line: 1,
          col: 1 }*/
        if (arg.type.indexOf("ignore:") === -1) {
          const item = await engine.evalValue(arg.value, scope);
          console.log(label, "got item:", arg.value, "eval:", item)
          argArray.push(item);
        }
        arg = lexer.next();
      }
    }
    console.log(label, "return array", argArray)
    return argArray;
  }

It prints something like:

[4] got item: "main1" eval: main1
[5] got item: "main1" eval: main1
[6] got item: "main1" eval: main1
[6] return array [ 'main1' ]
[7] got item: "main1" eval: main1
[7] return array [ 'main1' ]
[4] got item: "main2" eval: main2
[4] return array [ 'main1', 'main2' ]
[5] got item: "main3" eval: main3
[5] return array [ 'main1', 'main3' ]

This shows the array's value is not expected even before passed to liquidjs. I guess it's related to this line reseting the lexer based on string value:

eleventy/src/Engines/Liquid.js

Line 105 in 1ea3a90

lexer.reset(str);

But in JavaScript strings are immutable, which means the three passes of parseArguments() are reseting each other regardless they're different references to that string. Can be confirmed if I comment out if (!lexer) to create a new lexer each pass. After this change, it prints the following result (I guess it's the expected result):

www/test/index.html: file1 file2 file3
www/test/1/index.html: file1 file2 file3
www/test/2/index.html: file1 file2 file3
www/test/3/index.html: file1 file2 file3
www/test/index.html: main1 main2 main3
www/test/1/index.html: main1 main2 main3
www/test/2/index.html: main1 main2 main3
www/test/3/index.html: main1 main2 main3
[11ty] Writing www/test/index.html from ./src/test.liquid
[11ty] Writing www/test/1/index.html from ./src/test.liquid
[11ty] Writing www/test/2/index.html from ./src/test.liquid
[11ty] Writing www/test/3/index.html from ./src/test.liquid
[11ty] Wrote 4 files in 0.05 seconds (v1.0.0-canary.49)

The reason why it's may be related to LiquidJS version could be something changed to async and now concurrency actually happens. Nevertheless, I guess it can be a bug to reset the lexer based on string values. First time dig into this so maybe I'm wrong. @zachleat please let me know if there's something need my help.

[pdehaan]

Awesome, thanks for the 👀 @harttle! ❤️

[thisanimus]

I'm also noticing this behavior in 1.x eleventy builds when using a template that extends another template. eg:

If I extend the base layout in _layouts/posts.liquid:

---
layout: base.liquid
---

I get the same random missing args issue.
__
edit: turns out this can be circumvented by creating an include and putting the shortcode in the include

[epelc]

I just figured out a proper fix for this and #2348 without having to create a new lexer on each call.

Instead of using await on each call to evalValue you have to let it consume the current scope and use a await Promise.all() instead. This makes each call actually consume it's current scope at the time of call to evalValue otherwise it is randomly using the wrong scope because scope's fields are updated between calls to parseArguments since it's a object and objects are by reference in javascript.

I will submit a PR tomorrow but this is the rough code.
Without fix:

With fix:

Thanks @harttle for pointing in the right direction.

[zachleat]

Looks like we have two different conversations going so I’ll post comments on both 😅

Fixed by #2367. Thank you @epelc!

Release versions here: #2367 (comment)

[zachleat]

And a thank you to @harttle too!!

This reminded me also to set up an 11ty sponsorship of Liquid.js cc 11ty/11ty-community#78