tj / ejs

Embedded JavaScript templates for node
4.47k stars 514 forks source link

Unexpected behavior when trying to initialize local variables #92

Open ChrisMuc opened 11 years ago

ChrisMuc commented 11 years ago

I'm using multiple ejs templates which all include a common header tamplate, whereas the header template should be customizeable by the parent templates. Therefore, I'm using the following (simplified) structure:

index.ejs:

<% var showMenu = true; %>
<% include header%>
CONTENT

header.ejs:

<% if (typeof(showMenu) === 'undefined') { var showMenu = false; } %>
HEADER
showMenu:<%= showMenu %>

Unfortunately, the first line in header.ejs ends up in an unexpected behavior / bug! The idea behind this line is to check whether the parent template uses customization, and if not, initialize customization-variables with defaults. But in this small example showMenu is always set to false! Just like showMenu has never been declared in the parent template!

More interestingly: When you swap the first line in header.ejs to:

  typeof(showMenu):<%= typeof(showMenu) %>

to see if typeof() really returns 'undefined', you wil get surprised again. Instead of 'undefined' it returns the previously expected value ('boolean'):

  typeof(showMenu):boolean
  showMenu:true

So technically in our first example, showMenu should not be set to false.

If you remove the first line in index.ejs, you will also see an 'undefined'. (To actually see this value, you also have to remove the last line in header.ejs, as otherwise you will get an undefined exception)

If you are using a header.ejs like this:

typeof(showMenu):<%= typeof(showMenu) %>
HEADER
<% if (typeof(showMenu) === 'undefined') { %>
showMenu:not-set-by-parent
<% } else { %>
showMenu:<%= showMenu %>
<% }%>

everything works fine. Anyway, I would prefer the first version to work for mainly three reasons:

And just in case: I don't wanna start a discussion about MVC principles. I know that logic belongs to the controller and not to templates! But in my opinion this defnitely is a bug; in particular as the codes does not behave as expected.

ChrisMuc commented 11 years ago

As a quick fix, users can prevent the issue by using a local object:

header.ejs:

typeof(showMenu):<%= typeof(showMenu) %>
<%
    var local = {};
    if (typeof(showMenu) === 'undefined') {
        local.showMenu = false;
    } else {
        local.showMenu = showMenu;
    }
%>
HEADER
showMenu:<%= local.showMenu %>

Anyway, the bug should be fixed!

Cheers

ForbesLindesay commented 11 years ago

Could you put your code in code blocks to make it easier to read in GitHub issues. Just put 'html' or 'javascript' on its own line before your code and '```' on its own line after your code.

robdodson commented 11 years ago

+1 for this. it's totally weird

netpoetica commented 11 years ago

Possibly, this answer may be too simple, or I may not see the problem. But if you have index.ejs and you declare a variable and set it to true, it's type is now "boolean". For example, in JS console type

typeof(true)
> "boolean"

So if you call header afterwards, your variable showMenu is not "undefined", it's "true". This is why your code

 typeof(showMenu):<%= typeof(showMenu) %>

has a typeof "boolean".

If you want to manually override the showMenu variable, you will have to test if its value is true instead of typeof undefined - because it is defined.

<% if (showMenu) { var showMenu = false; } %>
HEADER
showMenu:<%= showMenu %>

Again, it's possible I am misunderstanding the issue here.

robdodson commented 11 years ago

No that's not the issue. The problem is that a variable declared in a parent template is available in a child template only in a really bizarre fashion.

For instance, if you do this:

foo.ejs

<% var hello = 'world' %>
<% include partials/bar.ejs %> 

bar.ejs

<%= hello %>

Then 'world' will show up in your bar.html file when it's rendered. So far so good. But if you do this:

foo.ejs

<% var hello = 'world' %>
<% include partials/bar.ejs %> 

bar.ejs

<% var hello = hello || 'busted' %>
<%= hello %>

Then the value of hello will be 'busted'. Which doesn't seem right. If you do this in the same template:

<%
var hello = 'world';
hello = hello || 'world';
%>

then the value of hello will still be 'world'. It's just that whenever you do a partial this seems to get screwed up. As a result it's really hard to provide a safe default value for a variable.

robdodson commented 11 years ago

Also it seems like the only way to get around it is to use typeof.

Neither of these work:

var stylesheets = stylesheets || [];
var stylesheets = stylesheets ? stylesheets : [];

This is the only solution that works for me:

var local = {};
if (typeof(stylesheets) === 'undefined') {
  local.stylesheets = [];
} else {
  local.stylesheets = stylesheets;
}
netpoetica commented 11 years ago

@robdodson - for the typeof thing, see https://github.com/visionmedia/ejs/issues/90

For why all this strangeness is happening, I look forward to someone way smarter than me telling us why :) my speculation is that it has something to do with the fact that you're trying to declare a local variable with the same name as a global variable, and somehow the EJS engine managing var declarations in a way that results in var hello = hello || 'world', hello being world.

There is this super-genius, magical parse function https://github.com/visionmedia/ejs/blob/master/lib/ejs.js#L116 which uses an array of strings to turn your template into a JS function (I think). I wonder if somehow scope gets lost in there

Mart-Bogdan commented 10 years ago

I don't think you should use sutch code. All point of EJS is to separate logic from View, you should move that variable to model and pass it as parameter when you are applying ViewEngine.