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.

Sign up for GitHub

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

Jump to bottom

Added support for subexpressions #690

Merged
merged 1 commit into from
Dec 31, 2013
Merged

Added support for subexpressions #690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions lib/handlebars/compiler/ast.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ var AST = {
MustacheNode: function(rawParams, hash, open, strip, locInfo) {
LocationInfo.call(this, locInfo);
this.type = "mustache";
this.hash = hash;
this.strip = strip;

// Open may be a string parsed from the parser or a passed boolean flag
Expand All @@ -58,6 +57,25 @@ var AST = {
this.escaped = !!open;
}

if (rawParams instanceof AST.SexprNode) {
this.sexpr = rawParams;
} else {
// Support old AST API
this.sexpr = new AST.SexprNode(rawParams, hash);
}

// Support old AST API that stored this info in MustacheNode
this.id = this.sexpr.id;
this.params = this.sexpr.params;
this.hash = this.sexpr.hash;
this.eligibleHelper = this.sexpr.eligibleHelper;
this.isHelper = this.sexpr.isHelper;
},

SexprNode: function(rawParams, hash) {
this.type = "sexpr";
this.hash = hash;

var id = this.id = rawParams[0];
var params = this.params = rawParams.slice(1);

Expand All @@ -84,8 +102,8 @@ var AST = {
},

BlockNode: function(mustache, program, inverse, close, locInfo) {
if(mustache.id.original !== close.path.original) {
throw new Exception(mustache.id.original + " doesn't match " + close.path.original);
if(mustache.sexpr.id.original !== close.path.original) {
throw new Exception(mustache.sexpr.id.original + " doesn't match " + close.path.original);
}

LocationInfo.call(this, locInfo);
Expand Down
90 changes: 46 additions & 44 deletions lib/handlebars/compiler/compiler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,12 +156,13 @@ Compiler.prototype = {
inverse = this.compileProgram(inverse);
}

var type = this.classifyMustache(mustache);
var sexpr = mustache.sexpr;
var type = this.classifySexpr(sexpr);

if (type === "helper") {
this.helperMustache(mustache, program, inverse);
this.helperSexpr(sexpr, program, inverse);
} else if (type === "simple") {
this.simpleMustache(mustache);
this.simpleSexpr(sexpr);

// now that the simple mustache is resolved, we need to
// evaluate it by executing `blockHelperMissing`
Expand All @@ -170,7 +171,7 @@ Compiler.prototype = {
this.opcode('emptyHash');
this.opcode('blockValue');
} else {
this.ambiguousMustache(mustache, program, inverse);
this.ambiguousSexpr(sexpr, program, inverse);

// now that the simple mustache is resolved, we need to
// evaluate it by executing `blockHelperMissing`
Expand Down Expand Up @@ -198,6 +199,12 @@ Compiler.prototype = {
}
this.opcode('getContext', val.depth || 0);
this.opcode('pushStringParam', val.stringModeValue, val.type);

if (val.type === 'sexpr') {
// Subexpressions get evaluated and passed in
// in string params mode.
this.sexpr(val);
}
} else {
this.accept(val);
}
Expand Down Expand Up @@ -226,26 +233,17 @@ Compiler.prototype = {
},

mustache: function(mustache) {
var options = this.options;
var type = this.classifyMustache(mustache);

if (type === "simple") {
this.simpleMustache(mustache);
} else if (type === "helper") {
this.helperMustache(mustache);
} else {
this.ambiguousMustache(mustache);
}
this.sexpr(mustache.sexpr);

if(mustache.escaped && !options.noEscape) {
if(mustache.escaped && !this.options.noEscape) {
this.opcode('appendEscaped');
} else {
this.opcode('append');
}
},

ambiguousMustache: function(mustache, program, inverse) {
var id = mustache.id,
ambiguousSexpr: function(sexpr, program, inverse) {
var id = sexpr.id,
name = id.parts[0],
isBlock = program != null || inverse != null;

Expand All @@ -257,8 +255,8 @@ Compiler.prototype = {
this.opcode('invokeAmbiguous', name, isBlock);
},

simpleMustache: function(mustache) {
var id = mustache.id;
simpleSexpr: function(sexpr) {
var id = sexpr.id;

if (id.type === 'DATA') {
this.DATA(id);
Expand All @@ -274,9 +272,9 @@ Compiler.prototype = {
this.opcode('resolvePossibleLambda');
},

helperMustache: function(mustache, program, inverse) {
var params = this.setupFullMustacheParams(mustache, program, inverse),
name = mustache.id.parts[0];
helperSexpr: function(sexpr, program, inverse) {
var params = this.setupFullMustacheParams(sexpr, program, inverse),
name = sexpr.id.parts[0];

if (this.options.knownHelpers[name]) {
this.opcode('invokeKnownHelper', params.length, name);
Expand All @@ -287,6 +285,18 @@ Compiler.prototype = {
}
},

sexpr: function(sexpr) {
var type = this.classifySexpr(sexpr);

if (type === "simple") {
this.simpleSexpr(sexpr);
} else if (type === "helper") {
this.helperSexpr(sexpr);
} else {
this.ambiguousSexpr(sexpr);
}
},

ID: function(id) {
this.addDepth(id.depth);
this.opcode('getContext', id.depth);
Expand Down Expand Up @@ -349,14 +359,14 @@ Compiler.prototype = {
}
},

classifyMustache: function(mustache) {
var isHelper = mustache.isHelper;
var isEligible = mustache.eligibleHelper;
classifySexpr: function(sexpr) {
var isHelper = sexpr.isHelper;
var isEligible = sexpr.eligibleHelper;
var options = this.options;

// if ambiguous, we can possibly resolve the ambiguity now
if (isEligible && !isHelper) {
var name = mustache.id.parts[0];
var name = sexpr.id.parts[0];

if (options.knownHelpers[name]) {
isHelper = true;
Expand All @@ -383,35 +393,27 @@ Compiler.prototype = {

this.opcode('getContext', param.depth || 0);
this.opcode('pushStringParam', param.stringModeValue, param.type);

if (param.type === 'sexpr') {
// Subexpressions get evaluated and passed in
// in string params mode.
this.sexpr(param);
}
} else {
this[param.type](param);
}
}
},

setupMustacheParams: function(mustache) {
var params = mustache.params;
this.pushParams(params);

if(mustache.hash) {
this.hash(mustache.hash);
} else {
this.opcode('emptyHash');
}

return params;
},

// this will replace setupMustacheParams when we're done
setupFullMustacheParams: function(mustache, program, inverse) {
var params = mustache.params;
setupFullMustacheParams: function(sexpr, program, inverse) {
var params = sexpr.params;
this.pushParams(params);

this.opcode('pushProgram', program);
this.opcode('pushProgram', inverse);

if(mustache.hash) {
this.hash(mustache.hash);
if (sexpr.hash) {
this.hash(sexpr.hash);
} else {
this.opcode('emptyHash');
}
Expand Down
42 changes: 20 additions & 22 deletions lib/handlebars/compiler/javascript-compiler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,6 @@ JavaScriptCompiler.prototype = {
var current = this.topStack();
params.splice(1, 0, current);

// Use the options value generated from the invocation
params[params.length-1] = 'options';

this.pushSource("if (!" + this.lastHelper + ") { " + current + " = blockHelperMissing.call(" + params.join(", ") + "); }");
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer passing options object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another thing that was inlined


Expand Down Expand Up @@ -398,19 +395,23 @@ JavaScriptCompiler.prototype = {

this.pushString(type);

if (typeof string === 'string') {
this.pushString(string);
} else {
this.pushStackLiteral(string);
// If it's a subexpression, the string result
// will be pushed after this opcode.
if (type !== 'sexpr') {
if (typeof string === 'string') {
this.pushString(string);
} else {
this.pushStackLiteral(string);
}
}
},

emptyHash: function() {
this.pushStackLiteral('{}');

if (this.options.stringParams) {
this.register('hashTypes', '{}');
this.register('hashContexts', '{}');
this.push('{}'); // hashContexts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are registers still used after removing this and the other references?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for hashTypes/hashContexts

this.push('{}'); // hashTypes
}
},
pushHash: function() {
Expand All @@ -421,9 +422,10 @@ JavaScriptCompiler.prototype = {
this.hash = undefined;

if (this.options.stringParams) {
this.register('hashContexts', '{' + hash.contexts.join(',') + '}');
this.register('hashTypes', '{' + hash.types.join(',') + '}');
this.push('{' + hash.contexts.join(',') + '}');
this.push('{' + hash.types.join(',') + '}');
}

this.push('{\n ' + hash.values.join(',\n ') + '\n }');
},

Expand Down Expand Up @@ -526,7 +528,7 @@ JavaScriptCompiler.prototype = {
invokeAmbiguous: function(name, helperCall) {
this.context.aliases.functionType = '"function"';

this.pushStackLiteral('{}'); // Hash value
this.emptyHash();
var helper = this.setupHelper(0, name, helperCall);

var helperName = this.lastHelper = this.nameLookup('helpers', name, 'helper');
Expand Down Expand Up @@ -805,6 +807,11 @@ JavaScriptCompiler.prototype = {

options.push("hash:" + this.popStack());

if (this.options.stringParams) {
options.push("hashTypes:" + this.popStack());
options.push("hashContexts:" + this.popStack());
}

inverse = this.popStack();
program = this.popStack();

Expand Down Expand Up @@ -838,22 +845,13 @@ JavaScriptCompiler.prototype = {
if (this.options.stringParams) {
options.push("contexts:[" + contexts.join(",") + "]");
options.push("types:[" + types.join(",") + "]");
options.push("hashContexts:hashContexts");
options.push("hashTypes:hashTypes");
}

if(this.options.data) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the useRegister true case no longer used/needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is one of the things that had to be inlined to support subexpressions. There shouldn't be any performance impac (the same number of objects are being evaluated/created), just that the templates will be just slightly larger.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not possible to use a stack value or other means to remove the duplicated values? I worry about anything that adds bytes to the wire but that might be unfounded as gzip would ideally remove a lot of the weight of purely duplicated code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explored some options but this was by far the simplest/most efficient. The stack var optimization that prevented us from needing multiple stack vars when invoking helpers is no longer available to us since invoking a helper means invoking its 0-N subexpressions that would also need stack vars. It's really hard to explain this bit of it but I'm fairly certain this is as simple/efficient as it's gonna get, and I think you're correct about gzip.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't think of this until now. What happens if you have subexpressions on the hash? Will that logic be duplicated or will it utilize a stack reference to lookup the value? That could be very costly if multiple expressions are nested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tests for subexpressions on the hash, and yes, nested subexpressions causes a lot of duplication.

Take a look at this gist: https://gist.github.com/machty/2091198e5cba9fe0163f On here I voiced some concerns about duplication, getting rid of stack vars, etc, and Yehuda seemed pretty ok with it given that gzip is a thing. (Though it still startled me at first)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with merging this for now, taking the gzip argument but I am going to take a crack at simplifying it post merge. Another set of eyes, etc :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be wonderful

options.push("data:data");
}

options = "{" + options.join(",") + "}";
if (useRegister) {
this.register('options', options);
params.push('options');
} else {
params.push(options);
}
return params.join(", ");
params.push("{" + options.join(",") + "}");
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer returning the params?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one used the return value. Should I just keep it as it was?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, didn't realize that it wasn't used until after I made the comment. It's not an API method and not used so we shouldn't waste cycles/bandwidth with it.


Expand Down
12 changes: 8 additions & 4 deletions lib/handlebars/compiler/printer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,18 +62,22 @@ PrintVisitor.prototype.block = function(block) {
return out;
};

PrintVisitor.prototype.mustache = function(mustache) {
var params = mustache.params, paramStrings = [], hash;
PrintVisitor.prototype.sexpr = function(sexpr) {
var params = sexpr.params, paramStrings = [], hash;

for(var i=0, l=params.length; i<l; i++) {
paramStrings.push(this.accept(params[i]));
}

params = "[" + paramStrings.join(", ") + "]";

hash = mustache.hash ? " " + this.accept(mustache.hash) : "";
hash = sexpr.hash ? " " + this.accept(sexpr.hash) : "";

return this.accept(sexpr.id) + " " + params + hash;
};

return this.pad("{{ " + this.accept(mustache.id) + " " + params + hash + " }}");
PrintVisitor.prototype.mustache = function(mustache) {
return this.pad("{{ " + this.accept(mustache.sexpr) + " }}");
};

PrintVisitor.prototype.partial = function(partial) {
Expand Down
14 changes: 12 additions & 2 deletions spec/ast.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,24 @@ describe('ast', function() {
});
});
describe('BlockNode', function() {
it('should throw on mustache mismatch (old sexpr-less version)', function() {
shouldThrow(function() {
var mustacheNode = new handlebarsEnv.AST.MustacheNode([{ original: 'foo'}], null, '{{', {});
new handlebarsEnv.AST.BlockNode(mustacheNode, {}, {}, {path: {original: 'bar'}});
}, Handlebars.Exception, "foo doesn't match bar");
});
it('should throw on mustache mismatch', function() {
shouldThrow(function() {
new handlebarsEnv.AST.BlockNode({id: {original: 'foo'}}, {}, {}, {path: {original: 'bar'}});
var sexprNode = new handlebarsEnv.AST.SexprNode([{ original: 'foo'}], null);
var mustacheNode = new handlebarsEnv.AST.MustacheNode(sexprNode, null, '{{', {});
new handlebarsEnv.AST.BlockNode(mustacheNode, {}, {}, {path: {original: 'bar'}});
}, Handlebars.Exception, "foo doesn't match bar");
});

it('stores location info', function(){
var block = new handlebarsEnv.AST.BlockNode({strip: {}, id: {original: 'foo'}},
var sexprNode = new handlebarsEnv.AST.SexprNode([{ original: 'foo'}], null);
var mustacheNode = new handlebarsEnv.AST.MustacheNode(sexprNode, null, '{{', {});
var block = new handlebarsEnv.AST.BlockNode(mustacheNode,
{strip: {}}, {strip: {}},
{
strip: {},
Expand Down
16 changes: 16 additions & 0 deletions spec/string-params.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,4 +142,20 @@ describe('string params mode', function() {

equals(result, "STOP ME FROM READING HACKER NEWS I need-a dad.joke wot", "Proper context variable output");
});

it("with nested block ambiguous", function() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this test doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It adds coverage to something that broke during my code reorg, specific to ambiguous block helper invocations only in string mode. Is there a better name or something more meaningful I could put in the description?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, I wasn't sure that it was related to string mode etc. For my own curiosity, how was the code failing when it broke?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Illegal JS code was generated in the template with stack-1 and stack-2 references showing up because I was missing a few pushStacks in the ambiguous block helper code path.

var template = CompilerContext.compile('{{#with content}}{{#view}}{{firstName}} {{lastName}}{{/view}}{{/with}}', {stringParams: true});

var helpers = {
with: function(options) {
return "WITH";
},
view: function() {
return "VIEW";
}
};

var result = template({}, {helpers: helpers});
equals(result, "WITH");
});
});
Loading