Skip to content
This repository has been archived by the owner on Apr 18, 2023. It is now read-only.

RFC: support for translations in addons #255

Closed
jamesarosen opened this issue Jun 24, 2015 · 14 comments
Closed

RFC: support for translations in addons #255

jamesarosen opened this issue Jun 24, 2015 · 14 comments

Comments

@jamesarosen
Copy link
Owner

In the discussion below, I will use two hypothetical addons, ember-paginate and ember-select.

Problem

Currently, if an addon wants to provide default translations that the app can consume and optionally override, there are two options:

Require App to Manage

Each addon has an addon/locales/en/translations.js file and similar files for all locales it supports. The app must import these in its own translations files:

// my-app/app/locales/en/translations.js:

import paginationTranslations from "ember-paginate/locales/en/translations";
import selectTranslations from "ember-select/locales/en/translations";
import Ember from "ember";

const translations = {};
Ember.merge(translations, paginationTranslations);
Ember.merge(translations, selectTranslations);

Ember.merge(translations, {
  "app.title": "My App"
});

export default translations;

That's a lot of boilerplate, especially since it needs to be repeated for every locale.

Addon Injects Translations

The addon could inject translations at runtime with an instance-initializer:

// ember-paginate/app/instance-initializers/ember-paginate.js:
import en from "ember-paginate/locales/en/translations.js";
import fr from "ember-paginate/locales/fr/translations.js";
import zh from "ember-paginate/locales/zh/translations.js";

export default {
  name: 'ember-paginate',
  initialize: function({ container }) {
    const i18n = container.lookup('service:i18n');
    i18n.addTranslations("en", en);
    i18n.addTranslations("fr", fr);
    i18n.addTranslations("zh", zh);
  }
};

That's far better from an app-developer standpoint, but worse for addon authors.

Goal

The goal of this RFC is to make it so easy for both addon- and app developers to support i18n that they do by default.

Proposal

The key to this implementation is to loosen the restriction on the names of translation files. ember-paginate would have ember-paginate/app/locales/en/ember-paginate.js. ember-select might have ember-select/app/locales/en/ember-select.js.

It would then be the responsibility of ember-i18n's Locale to merge translations from all of the various packages. The new logic to get the collected translations for locale ab-CDE would be

  1. enumerate locale:ab-CDE/*, excluding locale:ab-CDE/config
  2. merge those packages in alphabetical order
  3. walk up the locale-specificity to ab and repeat

emberjs/ember.js#11393 introduced a new private API, knownForType, that would be useful for enumerating these packages.

Ancillary Benefits

This change would also mean that app authors (or even addon authors) could break up their translations into multiple files. That would aid organization. I have not yet evaluated how that might work with pods.

Complications

There is potential for name collision across addons. It would thus be strongly recommended that addons define translations in my-addon/app/:locale/my-addon.js.

This change would mean there no longer necessarily be a locale:ab-CDE/translations. That means that addTranslations("ab-CDE", {...}) wouldn't have an obvious place to register its translations. We would want translations added through addTranslations to override anything from the lookup path. Thus, I propose addTranslations looks up or creates locale:ab-CDE/_runtime with highest precedence in the merging.

This change also suggests a change to the generator. One option would be to have the locale generator accept an optional second package argument, with the default translations. Alternatively, there could be a second locale-package or translations generator that requires a package argument.

/cc @jkarsrud @rwjblue

@jamesarosen
Copy link
Owner Author

@jkarsrud @rwjblue do you have any thoughts on this RFC? If you like it, I'm happy to spend some time this weekend or next to get it working.

@jamesarosen
Copy link
Owner Author

@bcardarella I know you support i18n in ember-validations. Do you have any thoughts on this RFC?

@jkarsrud
Copy link

@jamesarosen I like the proposal, it seems like a really clean way for addons to define its own translations. It beats having to create an instance-initializer to add the translations, even though that is a manageable way to implement it if you don't have a lot of locales.

@rwjblue
Copy link
Contributor

rwjblue commented Aug 29, 2015

@jamesarosen - The proposal looks good to me. We can also work on making knownForType public (it is only private because we didn't have other use cases at the time).

@jamesarosen
Copy link
Owner Author

Has anyone given pod-vs-not-pod any thought? Do I have to care or will the resolver handle that for me? I remember seeing a few bugs around that in v4.0, but I may have already ironed them all out.

@rwjblue
Copy link
Contributor

rwjblue commented Sep 9, 2015

Do I have to care or will the resolver handle that for me?

I believe that the resolver should do the heavy lifting here.

@jamesarosen
Copy link
Owner Author

We can also work on making knownForType public

That would be great! Where do you think that would go? On container directly as a registry-proxy method?

My plan is code something like

findTranslations(container, locale) {
  const result = {};

  let allLocaleFactories = null;
  if (container.knownForType) {
    allLocaleFactories = container.knownForType('locale')
  } else {
    allLocaleFactories = container._registry.knownForType('locale');
  }

  // do some filtering on locale here

  Object.keys(allLocaleFactories).sort().forEach(function(key) {
    if (/\/config$/.test(key)) { return; }
    Ember.merge(result, allLocaleFactories[key]);
  });

  return result;
}

@jamesarosen
Copy link
Owner Author

And knownForType is only Ember 2.0.0+. I'll need to include an implementation in ember-i18n for 1.11-1.13, but that will be difficult because there are different Resolvers that have different knownForType implementations :/

@rwjblue
Copy link
Contributor

rwjblue commented Sep 13, 2015

knownForType is available since Ember 1.13.

@jamesarosen
Copy link
Owner Author

Hey look at that! It sure is :)

That still doesn't cover the full compatibility for this library. But maybe this feature could be 1.13+.

@rwjblue
Copy link
Contributor

rwjblue commented Sep 13, 2015

let allLocaleFactories = null;
if (container.knownForType) {
allLocaleFactories = container.knownForType('locale')
} else {
allLocaleFactories = container._registry.knownForType('locale');
}

You shouldn't need this if/else, container.knownForType combines its internal items (things that were manually registered) with the registry's items (which itself defers to the resolver).

@rwjblue
Copy link
Contributor

rwjblue commented Sep 13, 2015

Nevermind, I'm crazy container.knownForType is not a thing. Sorry about that.

@simonihmig
Copy link

Hi there, just wanted to keep this discussion going...

There were some arguments exchanged regarding this topic over here: jasonmit/ember-i18n-cp-validations#14

Some of the thoughts there:

  • the "Require App to Manage" approach would include all the addon's translation files in the bundled javascript, even when not used. An addon that is coming with a lot of translation would probably bloat the app considerably. One could probably tackle that with the addon doing a funnel on its treeForAddon, but that would require additional work for every addon..
  • supply the translations as blueprints, so the developer is in control what to include in his build.

I just wanted to move this over here, so we have a chance to develop a consistent strategy for all addons out there, for a) the current state of ember-i18n, b) the time when the proposal above lands in ember-i18n (is this being worked on?), and c) for a smooth transition between a) and b) from the perspective of an addon's author.

@jamesarosen
Copy link
Owner Author

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

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

No branches or pull requests

4 participants