issue in include() functionality

morandd commented 1 year ago

Describe the bug

Nested includes() do not pass data to children templates by default. If you use: <%~ include("./child.eta") %> then child.eta will not receive any data available at the parent. But, if you provide an empty object as the second parameter to include, like this: <%~ include("./child.eta", {}) %> then the child does inherit access to the parents data.

The documentation implies that the second data parameter is optional and provides extra variables that will be merged with "it". But the observed behaviour is that the second parameter is undefined and thus overwrites "it". I think the issue arises as line 23 of compile-string.ts.

I suggest that nested child includes()'s should have access to the parent's data by default as this makes sense intuitively and is the behaviour of EJS.

Thanks for your contribution to this. Deno needs a good templating engine.

nebrelbug commented 1 year ago

@morandd thanks for opening this issue!

But, if you provide an empty object as the second parameter to include, like this: <%~ include("./child.eta", {}) %> then the child does inherit access to the parents data.

Are you sure this is the case? Though this should happen (according to documentation :sweat_smile:), looking at the code, it doesn't seem like the data is merged with it at all.

morandd commented 1 year ago

Yes, I think I can confirm this is a bug. Here's a test case:

templates/mypage.eta template page:

--- start of mypage.eta ---
<%~ 
/* According to 'merge' principle in the documentation, header.eta should receive it.lang and it.name */
include('header.eta',{name:'Ben'}); %>
--- end of mypage.eta ---

which uses the header templates/header.eta partial:

--header.eta
<html lang="<%~ it?.lang%>">
hi I am a header
<tag lang="<%~ JSON.stringify(it) %>">
--end header.eta

And we execute it using a simple script test.js:


import { Eta } from "https://deno.land/x/eta@v3.1.0/src/index.ts";
const eta = new Eta({ views:  Deno.cwd() + '/templates' })
console.log( eta.render("mypage.eta", { lang: "en" }) );

The If the merge operation happened correctly then header.eta should have access to it.lang and it.name, but it only has it.lang. Exectuing test.js produces:

--- start of mypage.eta ---
--header.eta
<html lang="undefined">
hi I am a header
<tag lang="{"name":"Ben"}">
--end header.eta--- end of mypage.eta ---

morandd commented 1 year ago

a workaround is to add {it} to every child include() call. For example:

<%~ include('header.eta',{it}) %>

this way the child partial header.eta will receive it from the parent.

nebrelbug commented 1 year ago

@morandd thanks for the thorough details and tests. This begs the question: do we want to automatically have partials inherit it from their parents, or should we remove that behavior from the docs?

I'll open up the discussion on Discord.

paul-norman commented 5 months ago

I've just started porting an EJS project to ETA and this is causing quite a lot of extra code. The docs state that:

"we can also pass in data that will be merged with it and passed to the partial"

But, as noted above by Ben, this is not in fact implemented at all.

Oddly, the layout() function does seem to work like this by default (and would be quite weird if it didn't), but the include / includeAsync functions are missing the ability.

At the moment, I am basically writing something like:

<%~ include('@template', Object.assign({}, it, { extra: 123 })) %>

almost everywhere (out of laziness - because I need a handful of global variables in almost all partials). This is leaving me with very messy template files. It would be nice to either have a third parameter that tells the function to use the global data value, or a separate set of functions that work this way:

<%~ include('@template', { extra: 123 }, true) %>
// or
<%~ includeData('@template', { extra: 123 }) %>

I prefer the second approach because it allows seemless passing without an empty object if there is no extra data to pass:

<%~ include('@template', {}, true) %>
// vs
<%~ includeData('@template') %>

Of course the includeAsync functions would also need this ability too.

nebrelbug commented 5 months ago

@paul-norman I understand where you're coming from. However, there is a faster way to merge data. It seems like { ...it, extra: 123 } might not add that much code and might make it more obvious for users how data is flowing.

eta-dev / eta

issue in include() functionality #254