Allow overriding the locale for individual translations #312

digitalcora · 2015-09-11T20:47:16Z

I had some free time, so here is my initial stab at #298. I couldn't find a way to stub the ENV value so I could test that it's correctly loaded into the service, but I manually confirmed that it works.

digitalcora · 2015-09-11T21:25:03Z

addon/services/i18n.js

+        (this.get('allowLocaleOverride') !== undefined),
+        { id: 'ember-i18n.locale-override' }
+      );
+      locale = new Locale(data.locale, this.container);


This part seems inefficient, but addressing it would probably require a larger change (for instance, locales could be cached on the service when they are initialized).

we only want to warn if data.locale is defined

we can make it slightly more efficient by saving off the warning text as a const at the top of the file

we could cache Locales in a closure-local map in the locale.js file and use the fact that returning an object from a constructor aborts the construction:

const seenLocales = {}; function Locale(name, container) { if (seenLocales[name]) { return seenLocales[name]; } seenLocales[name] = this; ... }

we only want to warn if data.locale is defined

This is inside the if (data.locale && ...) so the warning should only be seen in that case.

we could cache Locales in a closure-local map in the locale.js file

Clever! I will see if this works.

This is inside the if (data.locale && ...)

d'oh

Though I can't help thinking that from the outside, it's very counter-intuitive that new Locale only really constructs a new locale the first time it's called with a given argument, and thereafter just returns the previously-constructed object. At that point it's really more like a lazily-populated hash. Maybe a "locales" service or something?

That file is private, so we can change its API. We can change the exported function to localeFor(name, container), which uses the map and the Locale constructor.

Sounds like a good simple solution, I will try going in that direction.

digitalcora · 2015-09-14T14:56:38Z

@jamesarosen I tried changing things to use your localeFor-style proposal, but that caused test failures related to adding translations at runtime. Strangely, using your originally proposed approach (returning from the constructor) does work.

I've addressed the outstanding feedback and rebased. The .string issue seems to have mysteriously disappeared... so I guess this should be good to go.

digitalcora · 2015-09-28T18:13:21Z

@jamesarosen Any further changes needed? Could this be merged and released? (I guess it should have a changelog entry, I could write that if you like)

digitalcora · 2015-09-28T20:01:51Z

When testing this in my app, I discovered a need to find whether a given translation exists in another locale. So I extracted _getLocaleWithOverride and applied it to exists as well. Not sure what's up with the test failures on Travis – I don't get them locally, and they seem unrelated to my changes.

jamesarosen · 2015-10-01T18:55:03Z

addon/utils/locale.js

@@ -13,6 +15,10 @@ export default class Locale {
  //  3. Default `rtl` to `false`
  //  4. Ensure `pluralForm` is defined
  constructor(id, container) {
+    if (initializedLocales[id]) {
+      return initializedLocales[id];
+    }


You never populate initializedLocales with this after the if.

Argh. You're completely right. Unfortunately, actually doing this causes adding translations at runtime to stop working. So I guess this method of caching won't work after all. I'm really not sure how to handle this correctly, the architecture is hard for me to follow... should I just take it out for now? I can personally live with the sub-optimal performance, since my use case is not very intensive (this may be a case of premature optimization).

Can you try removing the if around locale.rebuild() in addTranslations?

I think we need to do some substantial re-architecting anyway to support #255. Probably best to think about the two problems together.

Just tried that, but the test for adding translations at runtime still fails.

OK. I'll give #255 a shot first. I may try moving some of the addTranslations logic into Locale so the I18nService doesn't have to worry about state management.

Alright. For now I'll proceed with using this branch in my app, since it seems stable and I haven't noticed any performance hits (I have a small number of translations, and the cross-locale lookup is only performed once on page load). I will keep an eye on that task.

digitalcora · 2015-10-29T19:56:16Z

@jamesarosen I've resolved conflicts with current master and removed the defunct attempt at caching. How would you like to handle this? Should it still wait on #255? Can it be merged with a potential caveat about performance? (I've been using this in production for a month now and haven't seen any breaking issues with it, but also haven't examined performance too closely)

jamesarosen · 2015-11-10T19:34:36Z

Sorry I've let this sit so long. I want to work on #255 very soon. I'll try to eke out some time this week. Once I have a better picture of that work, I'll let you know what we should do with this. I definitely want either this code or this idea in the project.

digitalcora · 2015-12-01T21:09:58Z

Rebased this on v4.2.0 to resolve conflicts. @jamesarosen how's it going? 😃

jamesarosen · 2015-12-16T18:29:06Z

I'm hoping to do some of this heavier work over the break right before the new year. I'll keep you updated.

digitalcora · 2016-01-19T00:42:12Z

Hi @jamesarosen, just want to confirm whether this is still slated to be merged at some point. In our limited use case we've had no problems with it, and have been using it in production since October – but we don't want to maintain a fork of ember-i18n indefinitely.

Of course, I'd be happy to write a changelog entry and wiki documentation for this feature, or any other friction-reducing tasks that would help.

jamesarosen · 2016-01-20T03:56:59Z

There's not much movement on ember-i18n these days because I'm working on a release for another project right now. But after that release will be some I18n work, which will be a perfect time to pick this up again.

Thanks for testing it out!

sbsurf · 2016-06-02T03:21:49Z

This would definitely be useful. I could use this as a way to inject missing translation fallbacks, while overriding the missingMessage function to just return the english translation.

sandstrom · 2016-08-23T22:10:03Z

@jamesarosen This would be a wonderful addition! ⛵

Is there anything in particular holding this PR back? (except the failing build 😄 )

sandstrom · 2016-08-23T22:15:46Z

@Grantovich Great initiative!

What do you think about this:

this.trigger('missing', this.get('locale'), key, data); // current
this.trigger('missing', locale.id, key, data); // my suggestion

Otherwise the missing message will trigger with the wrong locale, no?

source:

ember-i18n/addon/services/i18n.js

Line 41 in 9e84028

this.trigger('missing', this.get('locale'), key, data);

digitalcora · 2016-08-24T14:38:21Z

@sandstrom Thanks for the catch – I haven't been keeping this up-to-date lately since it seems there isn't much interest in merging it, and our project that uses this fork is in maintenance mode. If @jamesarosen gives the go-ahead, I would be happy to revisit it.

(tagging @jasonmit since from #381 (comment) it looks like we might be duplicating some effort)

jpsirois · 2017-12-06T18:07:58Z

I just had this exact same need for a project. And I extended the i18n services with the changes from this PR and it just work. Is this something still in consideration? I think this PR is worth consideration.

sandstrom · 2017-12-07T09:11:37Z

friendly ping @jamesarosen 😄

jamesarosen · 2017-12-11T22:49:42Z

I don't have any objections. I'd love one of the other maintainers to take a look and help shepherd this.

jamesarosen · 2018-10-23T23:53:09Z

jamesarosen/ember-i18n has been deprecated in favor of ember-intl.

digitalcora reviewed Sep 11, 2015
View reviewed changes

jamesarosen reviewed Oct 1, 2015
View reviewed changes

jamesarosen mentioned this pull request Jul 27, 2016

default translations and access to Locale #386

Closed

Allow overriding the locale for individual translations

825b6db

jamesarosen closed this Oct 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow overriding the locale for individual translations #312

Allow overriding the locale for individual translations #312

digitalcora commented Sep 11, 2015

digitalcora Sep 11, 2015

jamesarosen Sep 11, 2015

digitalcora Sep 11, 2015

jamesarosen Sep 11, 2015

digitalcora Sep 11, 2015

jamesarosen Sep 11, 2015

digitalcora Sep 11, 2015

digitalcora commented Sep 14, 2015

digitalcora commented Sep 28, 2015

digitalcora commented Sep 28, 2015

jamesarosen Oct 1, 2015

digitalcora Oct 1, 2015

jamesarosen Oct 1, 2015

digitalcora Oct 1, 2015

jamesarosen Oct 1, 2015

digitalcora Oct 1, 2015

digitalcora commented Oct 29, 2015

jamesarosen commented Nov 10, 2015

digitalcora commented Dec 1, 2015

jamesarosen commented Dec 16, 2015

digitalcora commented Jan 19, 2016

jamesarosen commented Jan 20, 2016

sbsurf commented Jun 2, 2016

sandstrom commented Aug 23, 2016 •

edited

Loading

sandstrom commented Aug 23, 2016 •

edited

Loading

digitalcora commented Aug 24, 2016

jpsirois commented Dec 6, 2017

sandstrom commented Dec 7, 2017

jamesarosen commented Dec 11, 2017

jamesarosen commented Oct 23, 2018

Allow overriding the locale for individual translations #312

Allow overriding the locale for individual translations #312

Conversation

digitalcora commented Sep 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalcora commented Sep 14, 2015

digitalcora commented Sep 28, 2015

digitalcora commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digitalcora commented Oct 29, 2015

jamesarosen commented Nov 10, 2015

digitalcora commented Dec 1, 2015

jamesarosen commented Dec 16, 2015

digitalcora commented Jan 19, 2016

jamesarosen commented Jan 20, 2016

sbsurf commented Jun 2, 2016

sandstrom commented Aug 23, 2016 • edited Loading

sandstrom commented Aug 23, 2016 • edited Loading

digitalcora commented Aug 24, 2016

jpsirois commented Dec 6, 2017

sandstrom commented Dec 7, 2017

jamesarosen commented Dec 11, 2017

jamesarosen commented Oct 23, 2018

sandstrom commented Aug 23, 2016 •

edited

Loading

sandstrom commented Aug 23, 2016 •

edited

Loading