Update helper to allow binding hash to be passed

[k-fish]

When translations need to occur across components or via services (for example, a flash message), it's easier to build a hash to send to the service to be rendered.

A second parameter (non hash) should be able to be set to cover cases when a hash has been built and needs to be passed to the {{t}} helper.

I'm not sure if this is in line with the vision for this addon, so this is just an RFC for now, I'm happy for comments or suggestions. Tests to come if this is all 👍

[jamesarosen]

Can you show some example usage? Also: is there a way you can make this backwards compatible?

[k-fish]

To answer your second question first, I believe it should be backwards compatible. The default for the second param is {} which assign will ignore as the empty object has no keys. If you don't send a second param to the helper it has no effect on the translation. I could add an assert perhaps that would mention if the second parameter is an unexpected type?

As for your first question, here is a simple example based off the wiki.

someDynamicKey: "user.followers.title",
someBindingHash: { count: "followersCount" },

These two properties could be set up (before translation) and sent as key and a hash of bindings to a template, passed down to a component, or passed to a service to get rendered elsewhere. It basically behaves like the t macro does.

The handlebars just looks like this:

{{t someDynamicKey someBindingHash}}

There was some talk around making a splat helper for handlebars (handlebars-lang/handlebars.js#1149), but in the mean time it would be nice to have support to pass a hash to this helper.

Thoughts? Or would you like a more thorough example?

[jamesarosen]

What's wrong with

{{t user.followers.title (hash count=followersCount)}}

?

[k-fish]

Maybe I'm missing something? 😄

I'm pretty sure (hash count=followersCount) would be the second parameter, which would currently be ignored because the helper only looks at params[0] for the key. The hash you mention would be params[1]?

[jamesarosen]

Sorry, I meant

{{t user.followers.title count=followersCount}}

[k-fish]

Ah, makes sense.

Nothing is wrong with that if you have a static translation key, and you know the bindings you'll want to apply for that key, like this example.

If you want to only display one translated line of text, for example, a custom error message, but you have multiple translations for different error cases and different bindings for those cases, then what you are currently stuck with, in my opinion, are less desirable options.

You can either make static key translations using the helper for each case with conditionals to switch between them, multiple templates and switch between those, or you have to use the service to translate in javascript, which you still have to make into a computed property, considering you want it to watch locale.

These are more complicated use cases, for sure, but it's nice to have the ability to dynamically pass keys and therefore also bindings for larger applications to cut down on the amount of code that needs to be written.

[jamesarosen]

Nothing is wrong with that if you have a static translation key

{{t user.followers.title}} isn't a static translation key. The value of the key is at this.user.followers.title. {{t "user.followers.title"}} would be the translation key "user.followers.title".

It seems you're trying to add a layer of indirection, but I don't understand what it's doing or why. I think ember, Handlerbars, and ember-i18n already give you all the power you need.

but you have multiple translations for different error cases and different bindings for those cases

Here's how that might look.

// translations:
errors: {
  foo: {
    invalid: "Foo {{foo.name}} is invalid}}"
  },
  bar: {
    invalid: "Bar {{bar.title}} is invalid}}"
  }
}

// component or controller:
errors: Ember.computed('foo', 'bar', function() {
  return [
    { key: 'errors.foo.invalid', foo: this.get('foo') },
    { key: 'errors.bar.invalid', bar: this.get('bar') },
  ];
})

and

{{!-- template --}}
{{#each errors as |error|}}
  {{t error.key error}}
{{/each}}

I think ember, Handlebars, and ember-i18n already give you all the indirection you need. If not, please write out a more full code use-case, because I still don't understand the indirection you're going for.

[k-fish]

I'd like indirection for the bindings. The key was my mishap because like you mentioned you can already pass a dynamic key (I thought you meant a string in your post).

Is this {{t error.key error}} supposed to be currently possible, or did you mean it as a hypothetical of the case I was proposing? Unless I'm missing something, if you pass t error.key error currently, the foo and bar keys won't bind to those translations you mentioned.

[jamesarosen]

In my above example,

{{#each errors as |error|}}
  {{t error.key error}}
{{/each}}

is equivalent to

{{t 'errors.foo.invalid' key='errors.foo.invalid' foo=foo}}
{{t 'errors.bar.invalid' key='errors.bar.invalid' bar=bar}}

The translation doesn't have interpolations for key, so it throws them away. It does have interpolations for foo and bar (respectively), and will interpolate them.

This functionality already exists in v4.0 due to the way Handlebars binding works.

[k-fish]

What I'm trying to say is that {{t error.key error}} does not seem to be equivalent to {{t 'errors.foo.invalid' key='errors.foo.invalid' foo=foo}}, at least not on a fresh build of Ember 2.10 with this addon. A helper parameter, like error, does not become the interpolations hash.

Running the code, and pausing inside the compute function, you see this:

Is there a setting or addon somewhere I'm missing? I used the same code you mentioned, with the exception of changing the translation from "Foo {{foo.name}} is invalid}}" to "Foo {{foo}} is invalid" for simplicity.

I'm not aware of handlebars splatting a positional argument into hash arguments?

[jamesarosen]

Aha! Now I understand :)

This might be a change with the move to Glimmer, or it might be the way Handlebars has always worked and I was expecting the Helper {{t 'foo' foo}} to work like the utility i18n.t('foo' foo).

(In the following examples, foo is { bar: 'baz' }.)

In handlebars-lang/handlebars.js#1050, @mixonic proposed the Handlebars splat operator, which would turn an object into hash params.

{{t 'foo' *foo}}

would be equivalent to

{{ 'foo' bar='baz' }}

@machty and @wycats are working on implementing something like that in handlebars-lang/handlebars.js#1149. Their preferred syntax (to better match ES6's splat operator) is

{{t 'foo' =...foo}}

Since that isn't merged, it obviously won't solve your problems now.

Option 1

We change the t helper (but not i18n.t) to allow a second argument that takes the place of the context hash.

{{t 'foo' foo}}

would be equivalent to

{{t 'foo' bar='baz'}}

The following would emit a warning and ignore the hash options:

{{t 'foo' foo wibble=wobble}}

Option 2

Option 1, but combing context object and hash options is allowed.

{{t 'foo' foo wibble=wobble}}

would be equivalent to

{{t 'foo' bar='baz' wibble=wobble}}

The problem with Option 2 is that we can't merge the context hash into the context object (since we don't want to mutate it) and we can't merge the object into the hash (since it might be an Ember.Object or an instance of a class with prototype properties). The only way to make that work would be to move support down two layers to the translation compiler:

export default function compileTemplate(template, rtl = false) {
  return function renderTemplate(contextHash, contextObject) {
    const result = template
      // interpolate, preferring contextHash properties to those from contextObject:
      .replace(tripleStache, (i, match) => getFromAorB(contextObject, contextObject, match))
      .replace(doubleStache, (i, match) => escapeExpression(getFromAorB(contextObject, contextObject, match)));

    const wrapped = rtl ? `\u202B${result}\u202C` : result;

    return htmlSafe(wrapped);
  };
}

function getFromAorB(a, b, key) {
  const fromA = get(a, key);
  return fromA === undefined ? get(b, key) : fromA;
}

But people do override the template compiler. We would need to warn them if they compile templates to a function that only takes one argument (hash) instead of two (hash, object), at least if they use the extra argument.

It would also mean adding support to the i18n.t utility. Since it's a new, optional third argument, that's not a breaking change.

Summary

If the Handlebars team are close to shipping their own splat operator, I'd prefer to just use that. If they're not, people will probably expect Option 2, so we should put in the effort to make that work.

[k-fish]

Aha! Now I understand :)

😄

This might be a change with the move to Glimmer, or it might be the way Handlebars has always worked and I was expecting the Helper {{t 'foo' foo}} to work like the utility i18n.t('foo' foo)

Exactly, it would be great if the behaviour matched.

With my code in this RFC,{{t 'foo' foo wibble=wobble}} will already work. Why are you worried about the mutating of the context object?

Ember.assign just walks over the object keys of the context hash and copies them into the context object. It won't copy non-enumerable properties (prototype). The only case I can see this being confusing is if the context hash has a value that will override the context object. An assert or documentation can be used to clear that up, however.

[jamesarosen]

Why are you worried about the mutating of the context object?

What if you did the following?

{{t 'foo' myObject foos=1}}
{{t 'foo' myObject foos=2}}

If we copy contextObject into contextHash (which is what this PR does), then we would need to get every property from contextObject because we don't know which ones might be needed in the translation. That sort of eager getting is exactly what Ember, Handlebars, and Glimmer try to avoid for performance reasons. It also means you can't override a contextObject's properties in Handlebars since the object version always wins.

If we copy from contextHash into contextObject, then we would be calling myObject.set('foos', 1) and then myObject.set('foos', 2). Those would conflict and might cause each other to endlessly rerender.

[jamesarosen]

We could, however, change the implementation to make contextHash win over contextObject and keep it all in Helper-land:

function mergedContext(contextObject, contextHash) {
  return Ember.Object.extend(contextHash, {
    unknownProperty(key) {
      return get(contextObject, key);
    }
  }).create();
}

[k-fish]

I was just going to mention unknownProperty.

Could we also perhaps just do Ember.assign({}, contextObject, contextHash)?

Edit: It gives preference to the hash while not setting on the object.

[k-fish]

I can write some tests to check all the cases we've been mentioning?

[jamesarosen]

Could we also perhaps just do Ember.assign({}, contextObject, contextHash)? It gives preference to the hash while not setting on the object.

It still does a get for every key in the contextObject, which might be very slow.

[k-fish]

Ah, that's a good point. It is better to be lazy 😆 . I'll change the code around.

[k-fish]

Apparently in glimmer 2 you can't extend helper hashes anymore as they are EmptyObject. emberjs/ember.js#14668

[jamesarosen]

How about

Ember.Object.extend({
  unknownProperty(key) {
    const fromHash = get(hashContext, key);
    fromHash === undefined ? get(objectContext, key) : fromHash;
  }
}).create()

[k-fish]

Yup 9d3b4c3#diff-5bd47a20c3d2ec5eb46a7b2a10404b4cR8

[k-fish]

Thoughts on the tests and the code so far?

[jamesarosen]

I think we're going to run into problems when a property of objectContext changes. We'll want a test for

{{t 'some.key' obj hashKey=hashValue}}

where we change obj.someProperty and hashValue and make sure both are reflected in the translated text.

[k-fish]

That's a fair point. I'll add a test.

[jamesarosen]

This PR now makes it obvious how cumbersome the existing test suite is here. I'd like to rework as much as possible into component (integration) tests that

1. inject some translations
2. set up interpolation variables
3. render a template that uses them
4. modify interpolations as needed

That doesn't have to happen before we merge this, but I'd like to have it happen soon.

[k-fish]

I was thinking that integration tests would be better when I was writing this. 😄

[jamesarosen]

Integration tests for the helper: #425

[k-fish]

Do you want me to wait until the tests are in to fix these ones or merge this first and come back and fix these tests?

[jamesarosen]

I think it might be easier to review the new tests here if we switch to integration tests first. I'm sorry to make you do more work. I'm happy to help migrate the tests!

[k-fish]

Sure, it's no problem.

[jamesarosen]

No longer used

[jamesarosen]

Released in v4.5.0

[k-fish]

Thanks @jamesarosen 🎉