Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Jekyll-like includes: {% include {{ page.my_variable }} %} #433

Closed
Nowaker opened this issue Dec 16, 2021 · 9 comments
Closed

Support Jekyll-like includes: {% include {{ page.my_variable }} %} #433

Nowaker opened this issue Dec 16, 2021 · 9 comments

Comments

@Nowaker
Copy link
Contributor

Nowaker commented Dec 16, 2021

{% include prefix/{{ page.my_variable }}/suffix %}

Just like it's included in Jekyll: https://jekyllrb.com/docs/includes/#using-variables-names-for-the-include-file

It's basically an improved and more flexible version of dynamic partials.

I tried working this limitation around in fs.readFile(path) interface, but unfortunately, the spec is too limited. The only argument passed is prefix/{{ page.my_variable }}/suffix. Liquid context is lost, so I cannot run this string through the Liquid engine again for processing to read the correct file.

If the spec of fs is modified to fs.readFile(path, context) or there's this.liquidContext, it would make my use case trivial to implement in fs, unless you want to make Jekyll-like dynamic includes an official part of Liquidjs.

@pdehaan
Copy link
Contributor

pdehaan commented Dec 17, 2021

I think you can do this now (at least in v9.29.0) w/ dynamic includes and prepend & append filters, or using the capture tag:

const { Liquid } = require('liquidjs');

const engine = new Liquid();
const tpl = engine.parse(`
  {%- assign myVariable = "home" -%}

  Option 1:
  {%- assign inclFile = myVariable | prepend: "_includes/header-" | append: ".liquid" -%}
  <!-- "{{ inclFile }}" -->
  {% include inclFile %}

  Option 2:
  {%- capture inclFile2 -%}
    _includes/header-{{ myVariable }}.liquid
  {%- endcapture -%}
  <!-- "{{ inclFile2 }}" -->
  {% include inclFile2 %}
`);
engine.render(tpl, {v: "Liquid"}).then(console.log);

Where my ./_includes/header-home.liquid file looks like this:

Welcome to {{ v }}!

And the output looks like:

> node example1

Option 1:<!-- "_includes/header-home.liquid" -->
Welcome to Liquid!


Option 2:<!-- "_includes/header-home.liquid" -->
Welcome to Liquid!

@Nowaker
Copy link
Contributor Author

Nowaker commented Dec 17, 2021

@pdehaan Modifying the source files is out of question. We have around 5,000 .html pages. And we're building a Gatsby-based toolkit that can source Jekyll sources as-is, and output the same thing as Jekyll does, at a 1000x speed increase. And all the goodness of Node.js and React.js.

I need a solution on liquidjs level, not on the Liquid source files level, that's why I'm asking to either introduce Jekyll-style includes into liquidjs core, or extend the fs interface to allow such things having Liquid context passed to it.

@harttle
Copy link
Owner

harttle commented Dec 17, 2021

I can try make this happen. If there's no perf regression I think we can ship an updated include. Otherwise, extending include would be better compared to rendering templates in FS.

@Nowaker
Copy link
Contributor Author

Nowaker commented Dec 17, 2021

@harttle Yeah, rendering in FS is definitely not the best solution! It's just a solution that I, a liquidjs user, can easily go for to get the result I need. :)

This is the code I have for now:

    fs: {
      resolve(root, file, ext) {
        return root + '/' + file
      },

      async readFile(file) {
        let urlToFetch = file
        if (file.indexOf('{{') !== -1) {
          const ctx = JSON.parse(pageContext.page)
          urlToFetch = await engine.parseAndRender(file, { page: ctx })
        }
        const data = await fetch(urlToFetch)
        if (data.ok) {
          return data.text()
        } else {
          throw new Error(`readFile(file): ${file} not found`)
        }
      },

It's not ideal, though. It doesn't preserve ALL context. pageContext.page is the initial page context, which is available to me. However, it's not a complete page context. As I recursively traverse through the Jekyll's layouts (page can extend a layout, which extends another layout, etc.) - a concept different to liquidjs-native layouts - the final context is not preserved.

So my solution works partially, and either a support for {{ vars }} inside {% include %} is needed, or adding a second parameter to fs.readFile so Liquid context is preserved (and any user, who shall need to access it for whatever reason, can access it).

Thanks for offering the help!

@harttle
Copy link
Owner

harttle commented Dec 18, 2021

A few more questions @Nowaker :

  1. do you need dynamicPartials and liquid syntax in include at the same time? I suppose these two features are exclusive.
  2. do you need different behaviors for include, layout, render? If we provide one single liquid option, they'll all have same behavior.

Maybe through a dynamicPartials: "liquid" option. How do you think?

@Nowaker
Copy link
Contributor Author

Nowaker commented Dec 18, 2021

@harttle

  1. They're mutually exclusive for me - since I'm looking for compatibility with Jekyll-flavored Liquid.
  2. Yeah, it makes sense to maintain the same behavior for them, and all other tags.

This sounds good. Maybe "jekyll" would be an even better name for this mode.

@harttle harttle changed the title Support for dynamic includes: {% include {{ page.my_variable }} %} Support Jekyll-like includes: {% include {{ page.my_variable }} %} Dec 18, 2021
github-actions bot pushed a commit that referenced this issue Dec 18, 2021
# [9.30.0](v9.29.0...v9.30.0) (2021-12-18)

### Features

* support jekyll-like include, see [#433](#433) ([23279a8](23279a8))
@harttle
Copy link
Owner

harttle commented Dec 18, 2021

I find it's already supported if dynamicPartials is true, but that requires quoting filenames into a pair of "".

On v9.30.0, I ported that feature also to dynamicPartials == false case, so it's enabled by default. See this e2e test for details:

liquidjs/test/e2e/issues.ts

Lines 164 to 179 in 23279a8

it('#433 Support Jekyll-like includes', async () => {
const engine = new Liquid({
dynamicPartials: false,
root: '/tmp',
fs: {
readFileSync: (file: string) => file,
async readFile (file: string) { return `CONTENT for ${file}` },
existsSync (file: string) { return true },
async exists (file: string) { return true },
resolve: (dir: string, file: string) => dir + '/' + file
}
})
const tpl = engine.parse('{% include prefix/{{ my_variable | append: "-bar" }}/suffix %}')
const html = await engine.render(tpl, { my_variable: 'foo' })
expect(html).to.equal('CONTENT for /tmp/prefix/foo-bar/suffix')
})

Also update docs for include, layout and render.

@Nowaker
Copy link
Contributor Author

Nowaker commented Dec 18, 2021

@harttle Implementation & docs are perfect, thank you! Thanks a lot! I'll promptly update and remove our readFile hackery :)

In the meantime, my project - rendering Jekyll sites in Gatsby - is moving forward! We have almost everything implemented already... Very exciting.

@harttle
Copy link
Owner

harttle commented Dec 19, 2021

Glad to hear! Also feel free to add an entry to Related Packages if you want to. Adding corresponding icons is also OK.

@harttle harttle closed this as completed Dec 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants