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

Integrate Eleventy Universal Filters, Shortcodes, and Paired Shortcodes #9

Closed
noelforte opened this issue Jul 2, 2024 · 6 comments
Closed
Assignees
Labels
enhancement New feature or request released Available to the masses! upstream Related to upstream dependencies

Comments

@noelforte
Copy link
Owner

With the addition of 11ty/eleventy#3310, it should now be possible to fully integrate Eleventy's template features (filters, shortcodes, paired shortcodes) into this plugin.

Only outstanding thing that still needs to be worked out is the appropriate way to add page context (this.page/this.eleventy) correctly. The approach in 11ty/eleventy#3081 (comment) could work by extending the TemplateEngine class, or perhaps by doing some other sort of wheel-reinvention to achieve context availability in shortcodes, filters, etc.

@noelforte noelforte self-assigned this Jul 3, 2024
@zachleat
Copy link

zachleat commented Jul 5, 2024

Sorry, I’m a little confused—isn’t this already implemented with

for (const helper in helperFunctions) {
helperFunctions[helper] = helperFunctions[helper].bind({
page,
eleventy,
});
}
?

@zachleat
Copy link

zachleat commented Jul 5, 2024

Ah, I think I see what you’re asking—you just want a less brittle way of doing it—right? I think we can accommodate that

@noelforte
Copy link
Owner Author

Yessir, if possible! Or at the very least an "official" recommendation on approach if you have one; even if that's "DIY, you're on your own" 😅

Looks like you do some call()s on the internal Liquid/Nunjucks engines to populate this correctly. If bind()/call() is the preferred recommendation to plugin authors for wiring up custom engines for adding page and eleventy context, that's totally fine for me to support on my end, just say the word and I'll stick with that.

@zachleat
Copy link

zachleat commented Jul 8, 2024

Related 11ty/eleventy#3355 let me know if that could be improved, I’m not super happy with it though it is less brittle than DIY

@noelforte
Copy link
Owner Author

noelforte commented Jul 10, 2024

@zachleat first off thank you for offering your time to address this upstream. Here's some observations I've made after several hours of trying, failing, and finally succeeding in using augmentFunctionContext but I'm not sure if my solution was how you intended for the augmenter method to be used:

As you saw, I loop through all filters per template compiled and add context to each, I'm not sure whether there's a more efficient way to do it, but there it is. My first attempt was calling augmentFunctionContext with each filter, and then assigning that to Vento's filters object before compiling the template.

for (const name in eleventyConfig.getFilters()) {
const filter = eleventyConfig.getFilter(name);
const filterWithContext = eleventyConfig.augmentFunctionContext(filter);
ventoEnv.filters[name] = filterWithContext;
}

Regardless, that didn't work because this.page and this.eleventy were still left unpopulated; this containing the data cascade and the Vento engine env.

What did end up working for me was supplying data as the source option for the augmentor within the filter loop.

for (const name in eleventyConfig.getFilters()) {
const filter = eleventyConfig.getFilter(name);
const filterWithContext = function (...args) {
const fn = eleventyConfig.augmentFunctionContext(filter, { source: data });

Again, I have no idea if this was how you imagined this method being used, and it's more than likely I'm missing something critical here. I reviewed the example you provided for Nunjucks and had trouble seeing how I would apply it to my use case. Only if you have the time, I'm curious to hear if where I arrived was what you envisioned.

Given the API differences from engine to engine in core, and that the implementation for shortcodes/filters differs with core vs plugins maybe it is simpler to just have custom engines handle this props DIY. The only downside I see being a scenerio where Eleventy introduces, changes, or deletes props on this for use in filters/shortcodes in the future and then it's up to plugin authors to re-establish feature parity. Not a huge problem since there's only 2 data keys on this for now. Parity and maintenence are why I brought this up to begin with.

Sorry for the lengthy response, hope it helps identify any improvements that could be made!

@noelforte noelforte added enhancement New feature or request upstream Related to upstream dependencies labels Jul 11, 2024
@noelforte
Copy link
Owner Author

@zachleat I took another look at what I wrote and realized that in the context of wrapping the filters with a function, this.data, this.env are populated by the template engine. I was getting confused because the verbage is very similar, when I see data as a prop, I inherently associate it with something that Eleventy is doing. Also, this is weird.

I took another pass at my implementation with this.data so I'm not passing the entire data cascade to the augmenter, although I guess it really doesn't matter whether I pass this.data as the source or the entire cascade as the data argument, but for parity's sake using only what the template engine has access to is probably best.

// Wrap filter with a function that augments the filter function
// with the data from Vento to extract `page` and `eleventy`
// values, then returns a call to that filter with the original arguments
ventoEnv.filters[name] = function (...filterArguments) {
const filterWithContext = eleventyConfig.augmentFunctionContext(filter, {
source: this.data,
});
return filterWithContext(...filterArguments);
};

My only other piece of feedback is that wrapping functions with functions, etc. feels a bit clunky. Would it be possible to expose props in some way on custom template engines so that plugin authors could pick those up and bind to this themselves, or is that an oversimplification on my part? I'm thinking then if some internal functionality changes down the line the list of props exposed to this is still maintained at the Eleventy level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released Available to the masses! upstream Related to upstream dependencies
Projects
None yet
Development

No branches or pull requests

2 participants