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

Inject ctx into addFilter #72

Closed
boehs opened this issue Nov 20, 2024 · 8 comments · Fixed by #77
Closed

Inject ctx into addFilter #72

boehs opened this issue Nov 20, 2024 · 8 comments · Fixed by #77

Comments

@boehs
Copy link

boehs commented Nov 20, 2024

For nunjucks, this works:

eleventyConfig.addNunjucksFilter("interpolate", function (str) {
    return nunjucks.renderString(str, this.ctx);
});

but, ctx is undefined for us:

const env = vento({
    autoDataVarname: true,
    autoescape: true,
});

eleventyConfig.addFilter("interpolate", function (str) {
    console.log(this.ctx); // undefined
    return env.runStringSync(str, this.ctx);
});

according to the Eleventy docs:

this.ctx (Nunjucks-specific) Added in v3.0.0

is there some way for this library to inject ctx here? I believe liquid also does it somehow, but with a different variable name

@boehs
Copy link
Author

boehs commented Nov 20, 2024

Monkeypatching CONTEXT_DATA_KEYS to include the keys I want is promising

@boehs
Copy link
Author

boehs commented Nov 20, 2024

- for (const K of CONTEXT_DATA_KEYS) {
-  env.utils._11tyCtx[K] = newContext[K];
- }
+ env.utils._11tyCtx = newContext;

simply doing this seems to work

@noelforte
Copy link
Owner

noelforte commented Nov 20, 2024

Hi— feature requests should use the issue template when creating them. No need to open a new issue but please use the template going forward. It helps me get the right information related to your issue so I can triage appropriately.


I'm curious to get more information about your use case. If you're using this plugin, importing ventojs and initializing a new environment isn't necessary since this plugin handles all .vto files Eleventy knows about.

Second, I'm confused why you would need an interpolate filter that re-runs Vento since Vento provides support for includes out of the box, which this plugin hooks into Eleventy's dir.includes directory by default, but can also be changed to something else via the plugin options.

If your use case is to run a template passed as a string, like so:

{{ '{{ 2 + 4 }}' | interpolate }}

why not just do

{{ 2 + 4 }}

If you're trying to render Vento from another template, you can use Eleventy's render plugin to render other template languages from inside a template.

The documentation you referenced states that this.ctx is a Nunjucks-specific feature, hence that wouldn't apply to a custom language like Vento. Also, you referenced Liquid as well— this.ctx has a special meaning in liquid that grants access to Liquids internals.

Lastly for a bit of background, setting this.page and this.eleventy for filters and shortcodes from this plugin is meant to maintain feature-parity with what's hard-coded in Eleventy. Since I can't import those keys from Eleventy itself and importing the augmentFunction utility seems like too much overhead (calling a function, that calls a function, ...) I maintain a very terse clone of that logic in this plugin instead.

In other words, since this.page and this.eleventy are what Eleventy provides by default, this plugin doesn't include it. Let me know if there's something specific you're after and I'll be happy to look into it.

EDIT: I just noticed the linked issue in the Vento repository. I'll take a look at that and add additional comments if I have any to this issue. Stand by!

@noelforte
Copy link
Owner

noelforte commented Nov 20, 2024

@boehs Ah, ok, I see what you're trying to do. It looks like you got there with a custom vento plugin loaded into the plugins option and using this.data to grab the function execution context established by the Vento environment.

To improve on that further, what I could do on my end is assign this.ctx to Vento's FilterThis context instead of discarding it as I do currently. Implementation would be as follows, using your example of this data:

{
    "viewport": "width=device-width,initial-scale=1",
    "theme-color": "{{color}}",
    "author": "{{config.author.name}}",
    "generator": "{{ eleventy.generator }}" // <-- you can use `eleventy.generator` here
}

Instead of relying on a custom plugin your declaration could be something like this:

eleventyConfig.addFilter('vento', function (code) {
  const file = `memory:${await hash(code)}.vto`;
  let res = await this.ctx.env.runString(code, this.ctx.data, file);
  return res.content;
});

and then your template would be:

{{ for name,content of metaTags }}
    <meta name="{{name}}" content="{{ content |> vento |> escape}}">
{{ /for }}

Then you wouldn't have to import ventojs or initialize a separate environment. Thoughts?

@noelforte
Copy link
Owner

As an addendum, is there a reason you don't want to have a meta.vto file in your _includes folder? Storing a list of meta tags that have dynamic values in a JSON file, seems like a bit of an over-engineered solution in my opinion, but again, I don't know what your use case is—maybe you have hundreds of meta tags to manage or you're managing them programatically.

In this particular case it doesn't seem unreasonable to maintain a file that consists of:

<meta name="viewport" content="width=device-width,initial-scale=1">
<meta name="theme-color" content="{{ color }}">
<meta name="author" content="{{ config.author.name }}">
<meta name="generator" content="{{ eleventy.generator }}">

and then in _includes/head.vto:

...
<!-- page title -->
<title>...</title>

<!-- meta tags -->
{{ include 'meta.vto' }}
...

That way you have proper syntax highlighting, can support multiple attributes on meta tags (future-proofing), and eliminate extra load and compile steps in memory.

@noelforte
Copy link
Owner

noelforte commented Nov 21, 2024

Instead of relying on a custom plugin your declaration could be something like this:

eleventyConfig.addFilter('vento', function (code) {
 const file = `memory:${await hash(code)}.vto`;
 let res = await this.ctx.env.runString(code, this.ctx.data, file);
 return res.content;
});

I took a deeper look into this idea and its definitely something I could implement. However, there's a big caveat to note, which is this.ctx is an unstable key to use within .addFilter() declarations since if a universal filter gets called by another template engine this.ctx could be undefined or contain a completely different object. (For instance, this.ctx doesn't exist in liquid templates, becuase the key is this.context.)

Do note also 11ty/eleventy#2844, which is tracking adding the entire data cascade to filters and shortcodes. If implemented this plugin will follow suit with whatever Eleventy does to maintain feature parity.

@github-actions github-actions bot mentioned this issue Nov 24, 2024
@boehs
Copy link
Author

boehs commented Nov 24, 2024

Hi, thanks for the detailed response

To improve on that further, what I could do on my end is assign this.ctx to Vento's FilterThis context instead of discarding it as I do currently. Implementation would be as follows, using your example of this data:

Yes, that would be ideal!

"you can use eleventy.generator here"

Good call!

As an addendum, is there a reason you don't want to have a meta.vto file in your _includes folder? Storing a list of meta tags that have dynamic values in a JSON file, seems like a bit of an over-engineered solution in my opinion, but again, I don't know what your use case is—maybe you have hundreds of meta tags to manage or you're managing them programatically.

At boehs.org enterprises everything is absurdly overengineered :-) https://github.com/boehs/site/blob/df888b1d877615f5313153022841cdf7af85e90f/.eleventy.js#L228-L269

I took a deeper look into this idea and its definitely something I could implement. However, there's a big caveat to note, which is this.ctx is an unstable key to use within .addFilter() declarations since if a universal filter gets called by another template engine this.ctx could be undefined or contain a completely different object. (For instance, this.ctx doesn't exist in liquid templates, becuase the key is this.context.)

Good point on the instability, I do think it would be nice to have with that note nevertheless! I'm sure there's other reasons why you'd want to access the cascade in a filter

Do note also 11ty/eleventy#2844, which is tracking adding the entire data cascade to filters and shortcodes. If implemented this plugin will follow suit with whatever Eleventy does to maintain feature parity.

Eagerly await!

Thank you for the amazing response and this great library

noelforte added a commit that referenced this issue Nov 24, 2024
@noelforte
Copy link
Owner

Shipped in 4.1.0!

At boehs.org enterprises everything is absurdly overengineered :-)

Disclosure: I wouldn't consider shipping this feature if it wasn't something Vento included as part of its own design. Since FilterThis is already a thing, it made sense to me to leverage it in an appropriate way and not throw it out.

Biggest reason against is that I'd highly reccomend avoiding excess compilation via template engines in Eleventy. The docs makes a point of recomending against template strings (see end of that section) when computing data, as using them incurs a performance overhead.

You can achieve a much more elegant result that's template engine independent by converting your JSON data files to JS modules that export objects. Then import the objects from other data file modules to create data dependence, since not all template engines will expose this.ctx (it's not a thing in Vento as I explained in #72 (comment)). For things like eleventy.generator where you need the Data cascade, use eleventyComputed.js and assign the data key to a function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants