Template variables can now execute functions and methods #182

Merged
merged 4 commits into from Jun 9, 2013

Projects

None yet
@fzaninotto
Contributor

Before:

<p>Hi, this is {{ user.fullName() }}</p>

Would return nothing, even though fullName() is a real function. This PR fixes it.

Function call executes in the scope of the variable, which means that in this example, if fullName() usesthis, it refers to user.

Refs #174, #151, #145, #140, #100, #90, #89

fzaninotto added some commits Jan 18, 2013
@tanepiper

+1 this would be highly useful!

@TimNZ
TimNZ commented Jan 22, 2013

👍 Definitely a wanted enhancement - please extend this to the tags e.g. {% for i in var.func('args') %}, they call setVar directly.

The argument that is passed to setVar already has an args property, I can see this is set with 'args', but ignored by setVar, you might want to revisit your implementation to factor that in.

@paularmstrong paularmstrong commented on the diff Jan 22, 2013
lib/helpers.js
};
-exports.wrapMethod = function (variable, filter, context) {
- var output = '(function () {\n',
- args;
-
- variable = variable || '""';
-
- if (!filter) {
- return variable;
+exports.wrapArgs = function (arguments) {
@paularmstrong
paularmstrong Jan 22, 2013 Owner

arguments is a reserved word. You shouldn't ever use it unless referencing a functions arguments object: more info

@paularmstrong
Owner

This pull request breaks the for tag. It will need to pass all tests before being considered for merging.

@paularmstrong
Owner

@fzaninotto: are you available to get things working?

@fzaninotto
Contributor

@paularmstrong: Well, I already spent a couple hours trying to get all tests passing and failed. I don't know if my approach fits your architecture well. I think I need your help to finish this.

@mjacksonw

This would be pretty huge. Rooting for this one.

@suin
suin commented Feb 22, 2013

This would be very useful feature!

@rypan
rypan commented Mar 1, 2013

Would love to see this happen. I'd use it on a project I'm currently working on.

@rickocbt

+1 this would be an immensely useful feature!

@paularmstrong
Owner

For all of those people that want this, someone will need to pick up where this was left off and fix all of the tests and inline notes before anything can happen moving forward with it.

@iamjochem

I had a look at the failing tests and found line 30 of lib\tags\for.js to be causing the test failures mentioned previously, the line looks like this:

operand1 = helpers.escapeVarName(operand1);

removing that line stops the failing "for" tag tests. I am not sure whether it is correct to remove that line but I cannot see any reason to escape the varname for operand1 of a "for" tag either (operand1 should only ever be a simple varname, no?)

given a template like so:

{% for foo in bar %}{% endfor %}

the call to helpers.escapeVarName() causes "foo" to become the string (typeof foo === \'function\') ? foo(undefined) : foo, that string is then used in the generated JS as the key in the 'context' object, e.g.:

__ctx_operand = _context["(typeof key === \'function\') ? key(undefined) : key"]

... which is quite obviously not going to work!

maybe this info helps @fzaninotto :)
maybe @paularmstrong could comment on whether the escaping of operand1 in the "for" tag is actually wanted/required?

@dgbeck
dgbeck commented Apr 23, 2013

+1 for this guy, will be super helpful

@iamjochem

ping @fzaninotto, @paularmstrong - please check my previous comment regarding a possible fix for the failing for tag tests.

@paularmstrong
Owner

@iamjochem Sounds fine. I'm just waiting for a pull request with those changes.

@fzaninotto
Contributor

I don't have a Node.js at hand to test that fix. @iamjochem, can you add commits to the PR?

@iamjochem

@fzaninotto no I cannot - it's your fork not mine (and I don't have "write permissions" on your fork). I would suggest you wait until you do have Node.js at hand, I'm sure we can all wait a few more days - alternatively just patch the file as per my suggestion (delete line 30 of lib\tags\for.js), commit into your branch and push it to github.com, travis will automatically be run as a result and you will see whether my 'fix' solves the failing test issues (underneath the original PR comment at the top of this page)

personally I an not really invested in this PR anymore, I would have liked to be able to use [data] object methods in my templates (in the way I was accustomed to doing inside twig templates, for instance) but I really could not wait a month to get this issue moving ... I ended up using object property getters instead

@fzaninotto
Contributor

Good to merge!

@dgbeck
dgbeck commented May 19, 2013

Nice!!

@hrajchert

So, whats the status on this, is it going to be merged? its the only thing keeping me of using swig. Also, does this PR accept parameters?

@iamjochem

@hrajchert with regard to your first question - I don't know, with regard to the second question: yes method parameters are supported.

@gustavohenke
Contributor

I'm also interested to know if this is ever going to be merged.

@paularmstrong paularmstrong merged commit af004b3 into paularmstrong:master Jun 9, 2013

1 check passed

default The Travis build passed
Details
@paularmstrong
Owner

Sorry everyone, I haven't had the time to get to this due to much else going on. It's merged now. Will put up a new version tonight.

@fzaninotto fzaninotto deleted the fzaninotto:executable_variable branch Jun 9, 2013
@gustavohenke
Contributor

Thank you ;)

@podviaznikov podviaznikov referenced this pull request in andrewrk/swig-email-templates Jul 20, 2013
Merged

update version of swig library #7

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