tbranyen / combyne

A template engine that works the way you expect.
MIT License
144 stars 19 forks source link

Use variable as argument of a function #61

Closed chikamichi closed 9 years ago

chikamichi commented 9 years ago

Hi,

I wonder whether it is possible to use a variable as an argument of a function (Property).

My use-case is pluralization of i18n strings. Starting with the following:

i18nFn =
   i18n: (key, options) =>
      # options could either be an object, or a integer in which case it's a count.
      # Here, I'm interested by the use-case with a count argument.
      @langManager.engine.t(key, options)

then passing i18nFn to a rendering context allows me to write things like

{{i18n 'some.translation.key'}}

But, when it comes to pluralization, I would need to be able to write things like

{{i18n 'some.translation.key' count}}

where count is part of the rendering context (data) and should evaluate to an integer, say 3. Unfortunately, this raises a ReferenceError (count is not defined).

How could one work around this?

The compiled template would currently look like:

[…] encode(data['i18n']('some.translation.key',count)) […]

which is short a data[] accessor/context on the count variable to make it right! Would you think poking for the data object as a possible fallback value for otherwise undefined argument would do the trick? (count || data["count"]) that is, or maybe using .call to enforce the scope to data? It would not support any variable to fit in, just those in the scope of the current context, which seems to be ok.

Thank you.

tbranyen commented 9 years ago

Ya I think that could be a relatively easy fix. If you want to try and patch it yourself (might be faster than me getting to this), you could start in: https://github.com/tbranyen/combyne/blob/master/lib/compiler.js#L204 which is where the args get compiled. Also if you could write a test similar to: https://github.com/tbranyen/combyne/blob/master/test/tests/properties.js#L114-L124 that be awesome :+1:

tbranyen commented 9 years ago

Since node.args is an array, you could map over it before it gets written in between the (). Something like:

normalizedArgs = node.args.map(function(arg) {
  return arg + " || data['" + arg + "']";
});

may work

chikamichi commented 9 years ago

Hey, meanwhile I already patched my local combyne to make it work :p

But it's really ugly, just a quick workaround to get to know the codebase:

Compiler.prototype.compileProperty = function (node, encode) {
    // […]
    var evalInContext = function(args) {
      var results = [];
      for (var i = 0; i < args.length; i++) {
        if ((type(args[i]) === 'string') && (!args[i].match(/^'.+'$/)))
          results.push('data["' + args[i] + '"]');
        else
          results.push(args[i]);
      }
      return results;
    }
    var value = [
      "(",
        // If the identifier is a function, then invoke, otherwise return
        // identifier.
        "typeof", identifier, "===", "'function'",
          "?", encode ? "encode(" + identifier + "(" + evalInContext(node.args) + "))" :
            identifier + "(" + evalInContext(node.args) + ")",
          ":", encode ? "encode(" + identifier + ") == null ? '' : encode(" +
            identifier + ")" : identifier + " == null ? '' : " + identifier,
      ")"
    ].join(" ");
    // […]
    return value;
};

As far as I understood it, node.args is a list of stringified arguments. If an argument is itself a string, then it is surrounded by simple quotes, which is kind of the only way to distinguish it from, say, an integer or… a potential variable. Hence the regexp match. But it feels clumsy :speak_no_evil:

chikamichi commented 9 years ago

Hum, quite a more elegant solution you propose here. Let's try that.

chikamichi commented 9 years ago

Well, it's not that easy to make the distinction between potentially valid pure-string arguments and potentially undefined variable arguments, as both are stored as strings in node.args. When mapping the simple way, it may generate useless, cluttered chunks of code (that is: a useless || statement for each pure-string argument). I feel like inspecting the arg to ensure the manipulation is actually needed is somewhat a good trade.

tbranyen commented 9 years ago

Yea, especially if someone passes false this wouldn't work so well. I think there is code already in Combyne to determine the type of an incoming value, maybe search around for that as it could be repurposed. Either way, if you open a PR we can try and find a good solution (or at least add a unit test to it as well so we can refactor).

chikamichi commented 9 years ago

Ok, I'll check that. And open a PR asap. For now I'll stick to my ugly workaround so that I can move on with my work, and get back to inspecting the issue in more details later tomorrow or next week!

tbranyen commented 9 years ago

@chikamichi https://github.com/tbranyen/combyne/pull/62 check out that PR, I think it'll address your issue :)

tbranyen commented 9 years ago

Closing for now, feel free to reopen if there is still an issue.

chikamichi commented 9 years ago

@tbranyen hi, I wonder whether it would be possible to add the same kind of support to %if conditionals?

So that one could write things like {%if modifiedAmount == amount %}…{%else%}…{%endif%}, with both amount and modifiedAmount properties of the rendering context.

tbranyen commented 9 years ago

Hrm, that should be trivial now, I'll try and get that feature in tomorrow

chikamichi commented 9 years ago

@tbranyen hi, just lurking around wondering whether you hacked something :) :cookie: