eta icon indicating copy to clipboard operation
eta copied to clipboard

compile without disabling include/includeFiles causes failure when using compiled template fn

Open filoxo opened this issue 3 years ago • 2 comments

Describe the bug

I'm new to ETA, but I discovered that I cannot use compile without specifying options because that later causes an error when invoking. The Compile documentation shows an example of using compile without options, but does show using options later when invoking the compiled template fn.

To Reproduce

Steps to reproduce the behavior:

  1. Go to interactive examples page https://eta.js.org/docs/examples/interactive

  2. Add a call to compile that does not have options. For completeness, I'll include the whole code block here:

    var Eta = require('eta')
    
    var compiled = Eta.compile(`
      <ul>
        <% for (var i = 0, l = it.list.length; i < l; i ++) { %>
            <li>User: <%~ it.list[i].user %> / Web Site: <%~ it.list[i].site %></li>
        <% } %>
      </ul>
    `)
    
    compiled({list: [{user: 'Me', site: 'https://my.cool.site'}]})
    
  3. Click on the green "Run" button

  4. Observe the error that reports TypeError: Cannot read property 'include' of undefined (see below for screenshot of this error)

  5. Now add a configuration option in the compile call. In this example, I set include and includeFiles to false which resolves my problem for now.

    var Eta = require('eta')
    
    var compiled = Eta.compile(`
      <ul>
        <% for (var i = 0, l = it.list.length; i < l; i ++) { %>
            <li>User: <%~ it.list[i].user %> / Web Site: <%~ it.list[i].site %></li>
        <% } %>
      </ul>
    `, { include: false, includeFiles: false } )
    
    compiled({list: [{user: 'Me', site: 'https://my.cool.site'}]})
    

The reason I set these to false is because inside of the compileToString fn (line 389 on my machine) within dist/eta.cjs file, its checking if these config options are truthy and rebinds some methods' context.

Screen Shot 2021-07-13 at 11 54 42 AM

But the error here stems from when that code is eventually eval'd, since E or E.include is never defined in the compileToString method. And I'm in unfamiliar territory so I'm not sure if E should already be defined, or if its a global. Would love to learn!

Expected behavior

I would expect that not providing any options should work with Eta's defaults, either when calling compile or when using the compiled template fn. I would also expect that compileToString should not throw an error and somehow ensure that E and E.include are defined before binding.

Screenshots

Screen Shot 2021-07-13 at 11 33 34 AM

Package & Environment Details

  • Environment: Node
  • Version: 14.15.1

Additional context

I originally stumbled upon this when wanting to add Eta to lukeed/tempura's benchmark tests. My fork might be helpful for executing tests locally. I'd also be happy to use this as a place to validate any fixes should there be any.

I would not be averse to helping contribute a fix. I think I just need some guidance before delving in to try and figure out how 😅

filoxo avatar Jul 13 '21 18:07 filoxo

Hey @filox

I was able to reproduce this, I can't tell for certain atm where the problem is coming from, but it seems to be problem with either getConfig in config.ts, or with copyProps from utils.ts, as it appears that its messing up and not passing any config to the compileToString. As for your question, E is the name of the parameter that is used within compiled functions to get the configuration.

shadowtime2000 avatar Aug 24 '21 22:08 shadowtime2000

filoxo's workaround with passing "include: false, includeFiles: false" didn't work for me

Now I get an even more cryptic error:

Cannot read property 'e' of undefined

v4dkou avatar Jan 21 '22 15:01 v4dkou

Indeed something as simple as this appears to be blowing up:

const Eta = require("eta");
const compiled = Eta.compile("Hi <%= it.name %>");

compiled({name: "Foo"});

and turning off include & includeFile doesn't resolve this (not to mention, what if your template has nested includes and you need those functions? xD) - I get the same error as in the post above. What is that E parameter supposed to be for? The generated function from the compile call looks something like

function anonymous(it, E, cb) {
   var tR = '';
   ...
   tR += E.e(it.name);
   ...
}

Any tricks we're missing here? I didn't see anything relevant in the docs. This looks like a pretty vanilla use case.

gkentr avatar Jan 05 '23 17:01 gkentr

@gkentr the second argument, E, should hold an Eta config object -- try passing in eta.config. Does that work for you?

nebrelbug avatar Jan 05 '23 17:01 nebrelbug

@nebrelbug thanks for replying, that seems to be working for the example I posted above and it's probably fine if you stay in a Node context. In my use case, I'm sending out rendered templates from Express that contain functions generated by Eta.compile, which can then be run in the browser later (via inline <script> tags). That would mean I have to send the config out with the rendered template, it looks like. It's a bit problematic because the config contains functions (at the very least XMLEscape - that's what 'e' is, after all - include, includeFile) that would have to be stringified to be sent out. In my experience, when you send out compiled template functions in EJS, everything needed by their scope seems to be already present, except the data and included templates.

Using Eta.config from the CDN-loaded version of Eta didn't work because the functions that were compiled server-side require includeFile, although not having that one in a browser context makes sense. I then tried sending out the functions required (thankfully they look fairly simple) and it looks like it worked, at least for templates that don't include others:

<script>
// this is sent by Express (res.render()):
...
        myTemplateFn: <%~ it.myTemplateFn ~%>, // generated by Eta.compile
        etaConfig: {
            e: <%~ it.etaConfig.e %>, // etaConfig = Eta.config, fed into res.render()
            include: <%~ it.etaConfig.include %>,
            includeFile: <%~ it.etaConfig.includeFile %>
        }
...
<script>

Then later on, in the browser:

myTemplateFn({foo: bar}, etaConfig);

I can open a separate issue if it's more appropriate. Thanks again!

gkentr avatar Jan 06 '23 13:01 gkentr

@gkentr I understand what you're saying. I agree that it is a bit of a pain having to pass in a config object, but I think it's best to keep Eta's behavior as it is right now since this is a pretty rare use case. The next version of Eta will handle config differently.

In order for include to work, you'll probably need to pass in templates to the config object. Try this:

        etaConfig: {
            e: <%~ it.etaConfig.e %>, // etaConfig = Eta.config, fed into res.render()
            include: <%~ it.etaConfig.include %>,
            includeFile: <%~ it.etaConfig.includeFile %>,
            templates: <%~ it.etaConfig.templates %>
        }

nebrelbug avatar Jan 06 '23 17:01 nebrelbug

If you want, you should be able to remove the need to pass in includeFile like this:

myTemplateFn: <%~ it.myTemplateFn.replace(",includeFile=E.includeFile.bind(E)", "") ~%>,

nebrelbug avatar Jan 06 '23 17:01 nebrelbug

Should be fixed in Eta v3

nebrelbug avatar Jun 13 '23 20:06 nebrelbug