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.

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

Use variable as argument of a function #61

Closed
chikamichi opened this issue Dec 18, 2014 · 12 comments
Closed

Use variable as argument of a function #61

chikamichi opened this issue Dec 18, 2014 · 12 comments

Comments

@chikamichi
Copy link

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
Copy link
Owner

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 👍

@tbranyen
Copy link
Owner

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
Copy link
Author

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 🙊

@chikamichi
Copy link
Author

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

@chikamichi
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

@chikamichi #62 check out that PR, I think it'll address your issue :)

@tbranyen
Copy link
Owner

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

@chikamichi
Copy link
Author

@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
Copy link
Owner

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

@chikamichi
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants