ejs files loose variables after $.each loop #242

Closed
markalanevans opened this Issue Jan 19, 2013 · 20 comments

Comments

Projects
None yet
6 participants
@markalanevans

Here is the fiddle: http://jsfiddle.net/Daff/2PNyJ/3/
I showed it to daffl and he trimmed down my fiddle.

View the console output the way it is.

You will get "m is not defined"

Then remove the .each loop. All is well.

@scorphus

This comment has been minimized.

Show comment
Hide comment
@scorphus

scorphus Jan 24, 2013

Contributor

Also, please compare that fiddle with this one: http://jsfiddle.net/scorphus/2PNyJ/5/

Maybe http://jsfiddle.net/Daff/2PNyJ/3/ shows the result of how the scanner works? Just thinking out loud...

Contributor

scorphus commented Jan 24, 2013

Also, please compare that fiddle with this one: http://jsfiddle.net/scorphus/2PNyJ/5/

Maybe http://jsfiddle.net/Daff/2PNyJ/3/ shows the result of how the scanner works? Just thinking out loud...

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Jan 28, 2013

Contributor

When the EJS scanner encounters a %> and the content in the EJS tags contains more { than }, It knows we are starting a new block. So EJS will wrap everything in the EJS tags in a can.view.txt call whose fifth parameter is a function to be executed that will output the correct HTML.

When you have a variable declaration in the same EJS tag as the beginning of a block statement, the declaration gets put in the function that is passed to can.view.txt, meaning the var declaration is now inside an anonymous function and is "lost".

To make this work, the EJS scanner would have to check for the var keyword and output it on the line before can.view.txt. This is more complicated than it sounds and I haven't worked on the scanner code, so I'm hoping @justinbmeyer can chime in here.

Contributor

ccummings commented Jan 28, 2013

When the EJS scanner encounters a %> and the content in the EJS tags contains more { than }, It knows we are starting a new block. So EJS will wrap everything in the EJS tags in a can.view.txt call whose fifth parameter is a function to be executed that will output the correct HTML.

When you have a variable declaration in the same EJS tag as the beginning of a block statement, the declaration gets put in the function that is passed to can.view.txt, meaning the var declaration is now inside an anonymous function and is "lost".

To make this work, the EJS scanner would have to check for the var keyword and output it on the line before can.view.txt. This is more complicated than it sounds and I haven't worked on the scanner code, so I'm hoping @justinbmeyer can chime in here.

@markalanevans

This comment has been minimized.

Show comment
Hide comment
@markalanevans

markalanevans Jan 29, 2013

For now, I have just been double declaring my variables in each block. As its the only way around it. @justinbmeyer any thoughts?

For now, I have just been double declaring my variables in each block. As its the only way around it. @justinbmeyer any thoughts?

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Jan 29, 2013

Contributor

double declaring...

All you have to do is declare the variable in a separate set of EJS tags. That way it will be outside of the function passed to can.view.txt

<% var m = "..."; %>
<% can.each(items, function(item){ %>
...
Contributor

ccummings commented Jan 29, 2013

double declaring...

All you have to do is declare the variable in a separate set of EJS tags. That way it will be outside of the function passed to can.view.txt

<% var m = "..."; %>
<% can.each(items, function(item){ %>
...
@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 29, 2013

Contributor

I'll bump this to 1.1.5. Looks as if it needs some more investigation.

Contributor

daffl commented Jan 29, 2013

I'll bump this to 1.1.5. Looks as if it needs some more investigation.

@markalanevans

This comment has been minimized.

Show comment
Hide comment
@markalanevans

markalanevans Jan 29, 2013

@ccummings thanks. I will give that a shot.

@ccummings thanks. I will give that a shot.

@ghost ghost assigned ccummings Feb 22, 2013

@scorphus

This comment has been minimized.

Show comment
Hide comment
@scorphus

scorphus Mar 11, 2013

Contributor

I've been dealing with this issue for a few days and, after trying to come up with a solution and thinking about it, I've come to the conclusion that it shouldn't be fixed.

Failed attempts

The following is what I've tried, in brief. Please excuse me if these changes are too subtle, they are just to back my point of view.

A) Split the block caller and what comes before it:

First, I tried to split the block caller -- a can.each call, a for loop or an if or etc. -- and all the lines that come before it. Considering a correct and simple template:

diff --git a/view/scanner.js b/view/scanner.js
index ccdffec..0bf3324 100644
--- a/view/scanner.js
+++ b/view/scanner.js
@@ -326,6 +326,14 @@ Scanner.prototype = {
                        // We are ending a block.
                        if (bracketCount == 1) {

+                           // Try and split the block caller and what comes before it (simple regex just for demo)
+                           var pos = content.search(/[^;\n\{]+[\s]*\{/);
+                           if (pos >= 0) {
+                               var before = content.substring(0, pos);
+                               content = content.substring(pos, content.length);
+                               buff.push(before);
+                           }
+
                            // We are starting on.
                            buff.push(insert_cmd, "can.view.txt(0,'"+getTag(tagName,tokens, i)+"'," + status() + ",this,function(){", startTxt, content);

Activate your console log and check http://jsfiddle.net/scorphus/6qpQp/ to see the result for a simple and kinda-fixable-by-this-attempt template. Note that it's still not what one would expect.

B) Extract variable declaration (not definition):

Then I tried to extract every variable declaration and place it before the block; but just the declaration. Extracting definition would be crazy given all the possible options. See:

diff --git a/view/scanner.js b/view/scanner.js
index ccdffec..51687e0 100644
--- a/view/scanner.js
+++ b/view/scanner.js
@@ -326,6 +326,16 @@ Scanner.prototype = {
                        // We are ending a block.
                        if (bracketCount == 1) {

+                           // Try and extract variable declaration (simple regex just for demo)
+                           var matches = content.match(/var[\s\n]+[^=]+=/g);
+                           if (matches && matches.length > 0) {
+                               for (var k = 0; (match = matches[k++]) !== undefined;) {
+                                   var vname = match.match(/var[\s\n]+([^=]+)=/)[1];
+                                   buff.push('var ' + vname + ';');
+                                   content.replace(match, vname + '=');
+                               }
+                           }
+
                            // We are starting on.
                            buff.push(insert_cmd, "can.view.txt(0,'"+getTag(tagName,tokens, i)+"'," + status() + ",this,function(){", startTxt, content);

Activate your console log and check http://jsfiddle.net/scorphus/CvnSe/ to see the result for a simple and kinda-fixable-by-this-attempt template. Note that it's still not what one would expect.

Conclusion (read "my humble opinion")

The scanner works the way it works in order to be quick. There is no need to parse the template, we can leave it up to the browser. So this leaves no space to do things such as A and B correctly for all possible syntaxes. There would be necessary to implement a proper parser to correctly extract variable definition or whatever comes before the block caller. And that would turn things unnecessarily slower and more complicated.

As suggested by Curtis, it's better and still fast to simply use %><% to separate such things, and to state this in the documentation.

Contributor

scorphus commented Mar 11, 2013

I've been dealing with this issue for a few days and, after trying to come up with a solution and thinking about it, I've come to the conclusion that it shouldn't be fixed.

Failed attempts

The following is what I've tried, in brief. Please excuse me if these changes are too subtle, they are just to back my point of view.

A) Split the block caller and what comes before it:

First, I tried to split the block caller -- a can.each call, a for loop or an if or etc. -- and all the lines that come before it. Considering a correct and simple template:

diff --git a/view/scanner.js b/view/scanner.js
index ccdffec..0bf3324 100644
--- a/view/scanner.js
+++ b/view/scanner.js
@@ -326,6 +326,14 @@ Scanner.prototype = {
                        // We are ending a block.
                        if (bracketCount == 1) {

+                           // Try and split the block caller and what comes before it (simple regex just for demo)
+                           var pos = content.search(/[^;\n\{]+[\s]*\{/);
+                           if (pos >= 0) {
+                               var before = content.substring(0, pos);
+                               content = content.substring(pos, content.length);
+                               buff.push(before);
+                           }
+
                            // We are starting on.
                            buff.push(insert_cmd, "can.view.txt(0,'"+getTag(tagName,tokens, i)+"'," + status() + ",this,function(){", startTxt, content);

Activate your console log and check http://jsfiddle.net/scorphus/6qpQp/ to see the result for a simple and kinda-fixable-by-this-attempt template. Note that it's still not what one would expect.

B) Extract variable declaration (not definition):

Then I tried to extract every variable declaration and place it before the block; but just the declaration. Extracting definition would be crazy given all the possible options. See:

diff --git a/view/scanner.js b/view/scanner.js
index ccdffec..51687e0 100644
--- a/view/scanner.js
+++ b/view/scanner.js
@@ -326,6 +326,16 @@ Scanner.prototype = {
                        // We are ending a block.
                        if (bracketCount == 1) {

+                           // Try and extract variable declaration (simple regex just for demo)
+                           var matches = content.match(/var[\s\n]+[^=]+=/g);
+                           if (matches && matches.length > 0) {
+                               for (var k = 0; (match = matches[k++]) !== undefined;) {
+                                   var vname = match.match(/var[\s\n]+([^=]+)=/)[1];
+                                   buff.push('var ' + vname + ';');
+                                   content.replace(match, vname + '=');
+                               }
+                           }
+
                            // We are starting on.
                            buff.push(insert_cmd, "can.view.txt(0,'"+getTag(tagName,tokens, i)+"'," + status() + ",this,function(){", startTxt, content);

Activate your console log and check http://jsfiddle.net/scorphus/CvnSe/ to see the result for a simple and kinda-fixable-by-this-attempt template. Note that it's still not what one would expect.

Conclusion (read "my humble opinion")

The scanner works the way it works in order to be quick. There is no need to parse the template, we can leave it up to the browser. So this leaves no space to do things such as A and B correctly for all possible syntaxes. There would be necessary to implement a proper parser to correctly extract variable definition or whatever comes before the block caller. And that would turn things unnecessarily slower and more complicated.

As suggested by Curtis, it's better and still fast to simply use %><% to separate such things, and to state this in the documentation.

@markalanevans

This comment has been minimized.

Show comment
Hide comment
@markalanevans

markalanevans Mar 13, 2013

Fair enough. I don't mind doing it at all, but definitely add it to the docs, this can easily cause a developer hours of confusion.

Thanks Scorphus for diving into this.

-Mark

Fair enough. I don't mind doing it at all, but definitely add it to the docs, this can easily cause a developer hours of confusion.

Thanks Scorphus for diving into this.

-Mark

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Mar 13, 2013

Contributor

Thanks @scorphus for investigating! I will bump this to 1.2 but as it looks right now we can only mention this in the documentation and leave as is right?

Contributor

daffl commented Mar 13, 2013

Thanks @scorphus for investigating! I will bump this to 1.2 but as it looks right now we can only mention this in the documentation and leave as is right?

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant May 10, 2013

Contributor

Recently ran into a similar issue while migrating a JMVC 3.1 codebase to latest. The workaround seems to be what Curtis mentioned (adding extra EJS tags to separate lines). It seems that EJS loses track of the number of brackets being opened and closed somewhere along the line.

This will generate a bad template:

<% if (true) { %>
        <% if (true) { %>
                hi
        <% }
 } %>

Whereas this is a good template:

<% if (true) { %>
        <% if (true) { %>
                hi
        <% } %>
<% } %>
Contributor

andykant commented May 10, 2013

Recently ran into a similar issue while migrating a JMVC 3.1 codebase to latest. The workaround seems to be what Curtis mentioned (adding extra EJS tags to separate lines). It seems that EJS loses track of the number of brackets being opened and closed somewhere along the line.

This will generate a bad template:

<% if (true) { %>
        <% if (true) { %>
                hi
        <% }
 } %>

Whereas this is a good template:

<% if (true) { %>
        <% if (true) { %>
                hi
        <% } %>
<% } %>
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 10, 2013

Contributor

Yeah, this is a very hard, if not impossible fix without parsing JS.

Sent from my iPhone

On May 10, 2013, at 11:17 AM, Andy Kant notifications@github.com wrote:

Recently ran into a similar issue while migrating a JMVC 3.1 codebase to latest. The workaround seems to be what Curtis mentioned (adding extra EJS tags to separate lines). It seems that EJS loses track of the number of brackets being opened and closed somewhere along the line.

This will generate a bad template:

<% if (awardCodeList && awardCodeList instanceof Array) { %>
<% if (true) { %>
hi
<% }
} %>
Whereas this is a good template:

<% if (awardCodeList && awardCodeList instanceof Array) { %>
<% if (true) { %>
hi
<% } %>
<% } %>

Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented May 10, 2013

Yeah, this is a very hard, if not impossible fix without parsing JS.

Sent from my iPhone

On May 10, 2013, at 11:17 AM, Andy Kant notifications@github.com wrote:

Recently ran into a similar issue while migrating a JMVC 3.1 codebase to latest. The workaround seems to be what Curtis mentioned (adding extra EJS tags to separate lines). It seems that EJS loses track of the number of brackets being opened and closed somewhere along the line.

This will generate a bad template:

<% if (awardCodeList && awardCodeList instanceof Array) { %>
<% if (true) { %>
hi
<% }
} %>
Whereas this is a good template:

<% if (awardCodeList && awardCodeList instanceof Array) { %>
<% if (true) { %>
hi
<% } %>
<% } %>

Reply to this email directly or view it on GitHub.

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant May 10, 2013

Contributor

Yeah, I think you're right.

Contributor

andykant commented May 10, 2013

Yeah, I think you're right.

andykant added a commit that referenced this issue May 13, 2013

Added support for EJS shared blocks #242
Transforms the EJS template to add support for shared blocks.
Essentially, this breaks up EJS tags into multiple EJS tags if they
contained unmatched brackets.

For example, this doesn't work:
	<% if (1) { %><% if (1) { %> hi <% } } %>
...without isolated EJS blocks:
	<% if (1) { %><% if (1) { %> hi <% } %><% } %>
The result of transforming:
	<% if (1) { %><% %><% if (1) { %><% %> hi <% } %><% } %>

@ghost ghost assigned andykant May 13, 2013

andykant added a commit that referenced this issue May 13, 2013

Eliminated the empty generated EJS tags, added more shared block tests
…#242

For some reason a template loaded by file generates a different result
than one loaded by string.
@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant May 13, 2013

Contributor

I've fixed a chunk of this issue (my semi-related portions), but the original issue still fails.

Contributor

andykant commented May 13, 2013

I've fixed a chunk of this issue (my semi-related portions), but the original issue still fails.

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant May 13, 2013

Contributor

I fixed the issue involving variables mentioned in the original issue description, except it only works if you're finishing your statements with semi-colons.

So these will work:

<%
  var m = "Mark";
  can.each(this, function( todo ) {
%>
  <span><%= todo.name %></span>
<% }); %>
<div><%== m %></div>

<%
  var m = "Mark"; can.each(this, function( todo ) {
%>
  <span><%= todo.name %></span>
<% }); %>
<div><%== m %></div>

but these won't:

<%
  var m = "Mark"
  can.each(this, function( todo ) {
%>
  <span><%= todo.name %></span>
<% }); %>
<div><%== m %></div>

<%
  var m = "Mark" can.each(this, function( todo ) {
%>
  <span><%= todo.name %></span>
<% }); %>
<div><%== m %></div>
Contributor

andykant commented May 13, 2013

I fixed the issue involving variables mentioned in the original issue description, except it only works if you're finishing your statements with semi-colons.

So these will work:

<%
  var m = "Mark";
  can.each(this, function( todo ) {
%>
  <span><%= todo.name %></span>
<% }); %>
<div><%== m %></div>

<%
  var m = "Mark"; can.each(this, function( todo ) {
%>
  <span><%= todo.name %></span>
<% }); %>
<div><%== m %></div>

but these won't:

<%
  var m = "Mark"
  can.each(this, function( todo ) {
%>
  <span><%= todo.name %></span>
<% }); %>
<div><%== m %></div>

<%
  var m = "Mark" can.each(this, function( todo ) {
%>
  <span><%= todo.name %></span>
<% }); %>
<div><%== m %></div>
@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant May 13, 2013

Contributor

I edited my last comment, but originally this solution also supported breaking up on new lines. The last two cases (statements broken by whitespace) are too difficult to fix in an automated way without adding a full JS parser.

Hopefully requiring a semi-colon is an acceptable tradeoff.

Contributor

andykant commented May 13, 2013

I edited my last comment, but originally this solution also supported breaking up on new lines. The last two cases (statements broken by whitespace) are too difficult to fix in an automated way without adding a full JS parser.

Hopefully requiring a semi-colon is an acceptable tradeoff.

andykant added a commit that referenced this issue May 13, 2013

Merge pull request #387 from bitovi/issue-242
Adds support for EJS shared blocks #242
@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant May 13, 2013

Contributor

I merged these fixes over. I'm going to leave this issue open for now for discussion.

I don't think there is a feasible way to fix this issue entirely without completely parsing the embedded JS code, due to how live binding wires up.

We should update the documentation to include some minor details regarding this quirk:

  • Avoid multi-purpose embedded JS in your EJS blocks
  • If you're defining variables, either define them in an isolated EJS block or use a semi-colon (which will have the same effect behind the scenes)
Contributor

andykant commented May 13, 2013

I merged these fixes over. I'm going to leave this issue open for now for discussion.

I don't think there is a feasible way to fix this issue entirely without completely parsing the embedded JS code, due to how live binding wires up.

We should update the documentation to include some minor details regarding this quirk:

  • Avoid multi-purpose embedded JS in your EJS blocks
  • If you're defining variables, either define them in an isolated EJS block or use a semi-colon (which will have the same effect behind the scenes)

andykant added a commit that referenced this issue May 13, 2013

andykant added a commit that referenced this issue May 13, 2013

Merge pull request #388 from bitovi/issue-242
EJS transforming wasn't processing semi-colons properly within parentheses #242

andykant added a commit that referenced this issue May 13, 2013

Merge pull request #389 from bitovi/issue-242
Removing the semi-colon splitter in EJS transform, too unreliable #242
@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant May 13, 2013

Contributor

Lots of activity... I reverted the changed related to variables as the solution just caused issues that weren't reasonably resolvable.

I would say we should just recommend that variables defined in templates are always defined in isolated blocks <% var m = "Mark" %> or are only used in the block they're defined in.

Contributor

andykant commented May 13, 2013

Lots of activity... I reverted the changed related to variables as the solution just caused issues that weren't reasonably resolvable.

I would say we should just recommend that variables defined in templates are always defined in isolated blocks <% var m = "Mark" %> or are only used in the block they're defined in.

andykant added a commit that referenced this issue May 26, 2013

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant May 26, 2013

Contributor

I've researched this a bit more and it seems borderline impossible to fix efficiently. In order for live binding to work, live binding uses the open EJS tag as a hint to inject a live-bound section.

The only possible solution seems to be to implement a JS parser to find the beginning of the final JS statement as a live-bound insertion point, but that requires a lot of code for limited benefit (although only at compile time).

The best solution seems to be to recommend that any long-lived variable, a variable that is accessed outside the active EJS block, should be defined in an isolated EJS tag.

<%
  var bestTeam = teams[0];
  can.each(teams, function(team) {
%>
  <div><%== team.name %></div>
<% }) %>
<div class='best'><%== bestTeam.name %>!</div>

becomes:

<% var bestTeam = teams[0]; %>
<%
  can.each(teams, function(team) {
%>
  <div><%== team.name %></div>
<% }) %>
<div class='best'><%== bestTeam.name %>!</div>
Contributor

andykant commented May 26, 2013

I've researched this a bit more and it seems borderline impossible to fix efficiently. In order for live binding to work, live binding uses the open EJS tag as a hint to inject a live-bound section.

The only possible solution seems to be to implement a JS parser to find the beginning of the final JS statement as a live-bound insertion point, but that requires a lot of code for limited benefit (although only at compile time).

The best solution seems to be to recommend that any long-lived variable, a variable that is accessed outside the active EJS block, should be defined in an isolated EJS tag.

<%
  var bestTeam = teams[0];
  can.each(teams, function(team) {
%>
  <div><%== team.name %></div>
<% }) %>
<div class='best'><%== bestTeam.name %>!</div>

becomes:

<% var bestTeam = teams[0]; %>
<%
  can.each(teams, function(team) {
%>
  <div><%== team.name %></div>
<% }) %>
<div class='best'><%== bestTeam.name %>!</div>
@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant May 26, 2013

Contributor

After discussing with fellow devs, we will be adding documentation for recommended usage/workaround but the bug itself won't be fixed since it would require fully parsing the JS.

Contributor

andykant commented May 26, 2013

After discussing with fellow devs, we will be adding documentation for recommended usage/workaround but the bug itself won't be fixed since it would require fully parsing the JS.

andykant added a commit that referenced this issue May 26, 2013

andykant added a commit that referenced this issue May 26, 2013

Documented that variable declarations and control blocks should alway…
…s be defined in dedicated EJS tags #242

Also commented out the test, since it won't be fixed

andykant added a commit that referenced this issue May 26, 2013

Merge pull request #402 from bitovi/ejs-lost-variables-242
Documented that EJS variable declarations and control blocks should be enclosed in dedicated tags #242

@andykant andykant closed this May 26, 2013

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant May 26, 2013

Contributor

Added documentation stating "declared variables and control blocks should always be enclosed in dedicated tags". Closed as won't fix.

Contributor

andykant commented May 26, 2013

Added documentation stating "declared variables and control blocks should always be enclosed in dedicated tags". Closed as won't fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment