Can’t find layout template in v9.26.0 (using Eleventy v1.0.0-canary.44)

[paulrobertlloyd]

Updating the dependencies for my Eleventy-based website (currently using v1.0.0-canary.44), and updating LiquidJS from v9.25.1 to v9.26.0 generates the following error:

[11ty] Problem writing Eleventy templates: (more in DEBUG output)
[11ty] > Having trouble writing template: www/bookmarks/2010/06/star_wars_ipad/index.html

`TemplateWriterWriteError` was thrown
[11ty] > ENOENT: Failed to lookup "default" in "src/includes", file:./src/layouts/bookmark.liquid, line:1, col:1

`RenderError` was thrown
[11ty] > ENOENT: Failed to lookup "default" in "src/includes"

`Error` was thrown:
[11ty]     Error: ENOENT: Failed to lookup "default" in "src/includes"
        at Liquid.lookupError (/Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/node_modules/liquidjs/dist/liquid.node.cjs.js:3121:21)
        at Liquid._parseFile (/Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/node_modules/liquidjs/dist/liquid.node.cjs.js:3063:20)
        at _parseFile.next (<anonymous>)
        at reduce (/Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/node_modules/liquidjs/dist/liquid.node.cjs.js:1737:25)
        at async TemplateLayout.render (/Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/node_modules/@11ty/eleventy/src/TemplateLayout.js:160:25)
        at async Template.renderPageEntry (/Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/node_modules/@11ty/eleventy/src/Template.js:781:17)
        at async /Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/node_modules/@11ty/eleventy/src/Template.js:803:21
        at async Promise.all (index 0)
        at async Promise.all (index 9)
        at async Eleventy.executeBuild (/Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/node_modules/@11ty/eleventy/src/Eleventy.js:958:13)

As to some background, I have Eleventy set up to look for includes and layouts in separate specified folders. At the top of my templates, I extend a common default.liquid template, i.e.:

{%- layout 'default' -%}
{%- block 'main' -%}
  …
{%- endblock -%}

Eleventy looks for this default template in ./src/layouts/, but since updating to the latest version of LiquidJS, it appears to now be looking for it in /src/includes/. Reverting to v9.25.1 of LiquidJS prevents this error from occurring. Looks like there was a bit of refactoring in the most recent release, which may be the culprit!

Happy to provide more details if required. My site’s code can be found at https://github.com/paulrobertlloyd/paulrobertlloyd-v4

[harttle]

Seems 11ty is relying on .render(ctx, scope, **opts**). I removed it (only set when constructing) to be more static and performanc. I'll see what impact it has on perf if I get it back.

[harttle]

Supporting customize LiquidOptions on .render() have 10%-50% performance impact (with cache enabled, since we care about production perf only). After some investigation, the bottleneck is customizing LiquidOptions will postpone cache validation. For example:

• options being static (root, cache, etc.): by {% render "foo" %}, we can directly check cache with key "foo".
• options being determined when render: we need resolve "foo" from filesystem against currrent root, then check cache with key like /www/includes/foo.liquid.
  • Resolving a file from filesystem is very costly, we even support npm style resolving that makes it even worse. Contributs 80% of regression.
  • Passing options around in parse*(), render*(), parseAndRender(), tag implementations is also contributing 10%-20% of regression.

I think we can do this differently to allow @paulrobertlloyd 's scenario and achieve good perf at the same time. Maybe:

• let 11ty have different liquid engine instances for different root configs.
• support root for includes and layout natively in LiquidJS

The second approach maybe better, but still need LiquidJS make changes and 11ty then make corresponding changes (move these root configs from render to new Liquid). And I want to remove opts from .render() for good, not sure whether there's elsewhere 11ty depends on it. @zachleat how do you think?

[harttle]

Maybe we take the second approach and @paulrobertlloyd can manually specify these roots. Is providing 2 more constructor parameters partials and layouts sufficient for you? @paulrobertlloyd

[paulrobertlloyd]

Sounds good to me! I assume this is something 11ty would do, taking the values for dir.layouts and dir.includes and passing them on to Liquid?

[harttle]

Yes, I believe so. I mean before 11ty can manage to change accordingly, can you use this LiquidJS feature directly so this regression unblocks your work?

[paulrobertlloyd]

Oooh, I understand. I can use my own Liquid instance with 11ty again and use these new options, certainly. Might me doing so be a good way to test this is a good approach, as well?

[harttle]

Try 9.27, added partials and layouts, same type as root.

[paulrobertlloyd]

Hmm, not sure if this is working. With the following options:

new Liquid({
  extname: '.liquid',
  layouts: './src/layouts',
  partials: './src/includes',
});

I get the following error:

[11ty] Problem writing Eleventy templates: (more in DEBUG output)
[11ty] > Having trouble rendering liquid template ./src/content/articles/1999-03-01-early_graphic_design_in_television.md

`TemplateContentRenderError` was thrown
[11ty] > ENOENT: Failed to lookup "photos" in ".", file:./src/content/articles/1999-03-01-early_graphic_design_in_television.md, line:44, col:1

`RenderError` was thrown
[11ty] > ENOENT: Failed to lookup "photos" in "."

`Error` was thrown:
[11ty]     Error: ENOENT: Failed to lookup "photos" in "."
        at Loader.lookupError (/Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/node_modules/liquidjs/dist/liquid.node.cjs.js:847:21)
        at Loader.lookup (/Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/node_modules/liquidjs/dist/liquid.node.cjs.js:832:20)
        at lookup.next (<anonymous>)
        at reduce (/Users/paulrobertlloyd/Sites/paulrobertlloyd-v4/node_modules/liquidjs/dist/liquid.node.cjs.js:913:25)

However, I get no errors if I set an array of directories for root instead:

new Liquid({
  extname: '.liquid',
  root: ['./src/layouts', './src/includes'],
});

[zachleat]

Canary 45 will use liquid 9.27.x—I’ll push this out today

[zachleat]

Oh, hmm—maybe it will, looks like we had some liquidjs regressions in our test suite

[zachleat]

Eleventy specifically only uses the compile time options to pass in the input file path to support relative includes e.g. include ../up-one-directory.liquid.

Is there a better way to do this?

[zachleat]

Just as info, it looks like 9.26 introduced regressions specifically with the issues documented above which are being picked up by all of the 1.0 canaries (we tied to ^9.25.1 in canary 44, which resolves to 9.27.0). I think I’ll need to lock to 9.25.x until we iron those out.

Here are the failures I’m seeing:

TemplateRenderLiquidTest › Liquid Render Include Subfolder Single quotes (relative include current dir) dynamicPartials true
  test/TemplateRenderLiquidTest.js:583

   582:   );                                           
   583:   t.is(await fn(), "<p>TIME IS RELATIVE.</p>");
   584: });                                            

  Rejected promise returned by test. Reason:

  RenderError {
    context: '>> 1| <p>{% include \'./relative-liquid/dir/included\' %}</p>',
    originalError: Error {
      code: 'ENOENT',
      message: 'ENOENT: Failed to lookup "./relative-liquid/dir/included" in "test/stubs/_includes"',
    },
    token: TagToken {
      args: '\'./relative-liquid/dir/included\'',
      begin: 3,
      content: 'include \'./relative-liquid/dir/included\'',
      end: 49,
      file: './test/stubs/does_not_exist_and_thats_ok.liquid',
      input: '<p>{% include \'./relative-liquid/dir/included\' %}</p>',
      kind: 4,
      name: 'include',
      trimLeft: false,
      trimRight: false,
    },
    message: 'ENOENT: Failed to lookup "./relative-liquid/dir/included" in "test/stubs/_includes", file:./test/stubs/does_not_exist_and_thats_ok.liquid, line:1, col:4',
  }

  › RenderError: ENOENT: Failed to lookup "./relative-liquid/dir/included" in "test/stubs/_includes", file:./test/stubs/does_not_exist_and_thats_ok.liquid, line:1, col:4
  › Render.renderTemplates (node_modules/liquidjs/dist/liquid.node.cjs.js:991:53)
  › renderTemplates.throw (<anonymous>)
  › node_modules/liquidjs/dist/liquid.node.cjs.js:923:34
  › test/TemplateRenderLiquidTest.js:583:8
  › From Error: ENOENT: Failed to lookup "./relative-liquid/dir/included" in "test/stubs/_includes"
  › Loader.lookupError (node_modules/liquidjs/dist/liquid.node.cjs.js:847:21)
  › Loader.lookup (node_modules/liquidjs/dist/liquid.node.cjs.js:832:20)
  › lookup.next (<anonymous>)
  › reduce (node_modules/liquidjs/dist/liquid.node.cjs.js:913:25)
  › test/TemplateRenderLiquidTest.js:583:8



  TemplateRenderLiquidTest › Liquid Render Relative (current dir) Include

  test/TemplateRenderLiquidTest.js:109

   108:   let fn = await tr.getCompiledTemplate("<p>{% include ./dir/included %}</p>");
   109:   t.is(await fn(), "<p>TIME IS RELATIVE.</p>");                                
   110: });                                                                            

  Rejected promise returned by test. Reason:

  RenderError {
    context: '>> 1| <p>{% include ./dir/included %}</p>',
    originalError: Error {
      code: 'ENOENT',
      message: 'ENOENT: Failed to lookup "./dir/included" in "test/stubs/_includes"',
    },
    token: TagToken {
      args: './dir/included',
      begin: 3,
      content: 'include ./dir/included',
      end: 31,
      file: './test/stubs/relative-liquid/does_not_exist_and_thats_ok.liquid',
      input: '<p>{% include ./dir/included %}</p>',
      kind: 4,
      name: 'include',
      trimLeft: false,
      trimRight: false,
    },
    message: 'ENOENT: Failed to lookup "./dir/included" in "test/stubs/_includes", file:./test/stubs/relative-liquid/does_not_exist_and_thats_ok.liquid, line:1, col:4',
  }

  › RenderError: ENOENT: Failed to lookup "./dir/included" in "test/stubs/_includes", file:./test/stubs/relative-liquid/does_not_exist_and_thats_ok.liquid, line:1, col:4
  › Render.renderTemplates (node_modules/liquidjs/dist/liquid.node.cjs.js:991:53)
  › renderTemplates.throw (<anonymous>)
  › node_modules/liquidjs/dist/liquid.node.cjs.js:923:34
  › test/TemplateRenderLiquidTest.js:109:8
  › From Error: ENOENT: Failed to lookup "./dir/included" in "test/stubs/_includes"
  › Loader.lookupError (node_modules/liquidjs/dist/liquid.node.cjs.js:847:21)
  › Loader.lookup (node_modules/liquidjs/dist/liquid.node.cjs.js:832:20)
  › lookup.next (<anonymous>)
  › reduce (node_modules/liquidjs/dist/liquid.node.cjs.js:913:25)
  › test/TemplateRenderLiquidTest.js:109:8



  TemplateRenderLiquidTest › Liquid Render Relative (parent dir) Include

  test/TemplateRenderLiquidTest.js:124

   123:   let fn = await tr.getCompiledTemplate("<p>{% include ../dir/included %}</p>");
   124:   t.is(await fn(), "<p>TIME IS RELATIVE.</p>");                                 
   125: });                                                                             

  Rejected promise returned by test. Reason:

  RenderError {
    context: '>> 1| <p>{% include ../dir/included %}</p>',
    originalError: Error {
      code: 'ENOENT',
      message: 'ENOENT: Failed to lookup "../dir/included" in "test/stubs/_includes"',
    },
    token: TagToken {
      args: '../dir/included',
      begin: 3,
      content: 'include ../dir/included',
      end: 32,
      file: './test/stubs/relative-liquid/dir/does_not_exist_and_thats_ok.liquid',
      input: '<p>{% include ../dir/included %}</p>',
      kind: 4,
      name: 'include',
      trimLeft: false,
      trimRight: false,
    },
    message: 'ENOENT: Failed to lookup "../dir/included" in "test/stubs/_includes", file:./test/stubs/relative-liquid/dir/does_not_exist_and_thats_ok.liquid, line:1, col:4',
  }

  › RenderError: ENOENT: Failed to lookup "../dir/included" in "test/stubs/_includes", file:./test/stubs/relative-liquid/dir/does_not_exist_and_thats_ok.liquid, line:1, col:4
  › Render.renderTemplates (node_modules/liquidjs/dist/liquid.node.cjs.js:991:53)
  › renderTemplates.throw (<anonymous>)
  › node_modules/liquidjs/dist/liquid.node.cjs.js:923:34
  › test/TemplateRenderLiquidTest.js:124:8
  › From Error: ENOENT: Failed to lookup "../dir/included" in "test/stubs/_includes"
  › Loader.lookupError (node_modules/liquidjs/dist/liquid.node.cjs.js:847:21)
  › Loader.lookup (node_modules/liquidjs/dist/liquid.node.cjs.js:832:20)
  › lookup.next (<anonymous>)
  › reduce (node_modules/liquidjs/dist/liquid.node.cjs.js:913:25)
  › test/TemplateRenderLiquidTest.js:124:8

  ─

  3 tests failed

[harttle]

It works on integration tests, not sure where's the difference. Just added a demo for partials and layouts here:

liquidjs/demo/nodejs/index.js

Lines 7 to 11 in 92bfc65

	root: __dirname,
	// layout files for `{% layout %}`
	layouts: './layouts',
	// partial files for `{% include %}` and `{% render %}`
	partials: './partials'

@paulrobertlloyd BTW, the above error Failed to lookup "photos" in "." is incorrect in the "." part, that's still using root. It's fixed on 9.27.1

uses the compile time options to pass in the input file path to support relative includes e.g. include ../up-one-directory.liquid.

@zachleat can be done by setting to .partials and .layouts instead of .root here: https://github.com/11ty/eleventy/blob/2824db33bab6ef5e8790f2f15a651d331c146f30/src/Engines/Liquid.js#L232-L235

Maybe it's cleaner to be implemented in LiquidJS, what do you think?

[paulrobertlloyd]

This is still not working with 9.27.1… I get the same error. Do I need to set root as well as partials and layout? (I tried that, and using a variety of combinations, but only setting an array of roots appears to work).

[harttle]

The demo here seems working https://github.com/harttle/liquidjs/tree/92bfc65e0b1d937c00a8368b272223c702132d23/demo/nodejs

Could you provide a minimal codebase to reproduce? I guess there's something with 11ty, maybe it's setting root again in .compile(). Will dig into that latter, resolving issues @zachleat posted above should make it work for everyone :)

[paulrobertlloyd]

I guess there's something with 11ty, maybe it's setting root again in .compile().

Ah, maybe changing settings even in a custom instance don’t affect how 11ty then handles LiquidJS internally. I’ll wait for the downstream update 🙂

[harttle]

Add support on v9.28.0 for relative reference. Things like following are now natively supported:

{% render ../foo/bar %}
{% layout ./bar %}

Note that relatively referenced files are also need to be within corresponding root.

[paulrobertlloyd]

Can confirm that using layouts and partials options in place of root now works as of v9.28.0 🎉

[zachleat]

I appreciate the movement here but I do think this was a semver breaking change, right?

Are we not allowed to use options or options.root in the third argument to render any more?

Should this have been bundled with a major version API change?

[zachleat]

Disregarding that, I think moving forward we will need a way to do relative includes that re-use the filepath passed in via parse. https://liquidjs.com/api/classes/liquid_.liquid.html#parse

e.g.

engine.parse(`{% include "../included.html" %}`, "./src/test.liquid");

should look in:

./included.html

(and any additional directories you specified in partials or layouts or root?

[harttle]

Should this have been bundled with a major version API change?

I made a mistake thinking that argument is only used by myself in .express(). I'll stick to semver next time :)

should look in: ./included.html

I agree, let me check that.

And..The second argument should be a full path. By passing in a template string, we'll not look up the filepath specified by second argument. That means "./src/test.liquid" cannot be relative to some root. I guess the actual behaviour ls looking in pwd/include.html.

[harttle]

Added 2 test cases for demo:

liquidjs/test/integration/liquid/liquid.ts

Lines 117 to 138 in 1de6adf

	it('should resolve relative partials', function () {
	const engine = new Liquid({
	root: ['/'],
	extname: '.html'
	})
	mock({
	'/root/partial.html': 'foo'
	})
	const tpls = engine.parse('{% render "./partial.html" %}', '/root/index.html')
	return expect(engine.renderSync(tpls)).to.equal('foo')
	})
	it('should resolve against pwd for relative filepath', function () {
	const engine = new Liquid({
	root: ['/'],
	extname: '.html'
	})
	mock({
	[`${process.cwd()}/partial.html`]: 'foo'
	})
	const tpls = engine.parse('{% render "./partial.html" %}', './index.html')
	return expect(engine.renderSync(tpls)).to.equal('foo')
	})

[zachleat]

I made a mistake thinking that argument is only used by myself in .express(). I'll stick to semver next time :)

Just looking at the docs, I think I was a bit harsh! I don’t think you put the third argument there into your API docs—so I’m not sure how I found it. Thank you for the help resolving this though!

[zachleat]

Just retested in 9.28.2 and still have a bunch of very basic regressions with lookup directories on the include tag 👀

[jaredwray]

I am also seeing issues with render and include still. Any status on this?

[harttle]

@jaredwray There is a breaking change here, opts for render() is no longer available. There may be other bugs, so please post the exact issue and let's see what we can do to fix that. I'm also willing to help :)

If it's not viable for you to incorporate this change and fix the issues, I suggest lock LiquidJS to its minor version.

[jaredwray]

@harttle - if opts for render is no longer available I will just remove that from the system. Is include the path moving forward?

[harttle]

In the latest version, there's a bug in include/render/layout that, if root/partials/layouts is specified as relative path, LiquidJS will fail to lookup the referenced file. I'm still figuring out how to fix this. Welcom to discusson #422

[harttle]

there's a bug in include/render/layout that,

@jaredwray FYI: this bug is fixed on 9.28.3 if there's still any issue, feel free to post it back.

[jaredwray]

@harttle - thanks will check this out soon.

[zachleat]

The fix for this has made it into Eleventy master and will ship with 1.0.0-beta.5.

Tracking at 11ty/eleventy#1995

Thank you @harttle for the PR!