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

Defining translations inside engines #797

Open
buschtoens opened this issue Feb 20, 2019 · 5 comments
Open

Defining translations inside engines #797

buschtoens opened this issue Feb 20, 2019 · 5 comments
Assignees

Comments

@buschtoens
Copy link
Member

I've noticed that publicOnly does not play well with engines. If enabled, the generated translation file is placed in the host app's translations dir and not in the engine's translations dir.

Disabling publicOnly also doesn't work, because the translation is not bundled into engine.js or engine-vendor.js, but seemingly gets merged into the host app's app tree:

ember-intl/index.js

Lines 111 to 142 in 57691dd

treeForApp(tree) {
let trees = [tree];
if (!this.opts.publicOnly) {
trees.push(
this.generateTranslationTree({
outputPath: 'translations',
filename(key) {
return `${key}.js`;
},
wrapEntry(obj) {
return `export default ${stringify(obj)};`;
}
})
);
}
if (tree && this.locales.length) {
let cldrTree = extract(tree, {
locales: this.locales,
relativeFields: true,
numberFields: true,
destDir: 'cldrs',
prelude: '/*jslint eqeq: true*/\n',
moduleType: 'es6'
});
trees.push(cldrTree);
}
return mergeTrees(trees, { overwrite: true });
},

Input

publicOnly: true

client
├── package.json       # name: "client"
├── app
│   └── ...
└── translations
    └── de-de-x-host.yml

client-packages/engines/
└── direct-sales
    ├── package.json   # name: "@clark/engine-direct-sales"
    ├── addon
    │   └── ...
    └── translations
        └── de-de-x-engine.yml

Expected Output

dist
├── asset-manifest.json
├── assets
│   ├── client.js
│   └── vendor.js
├── engines-dist
│   └── @clark
│       └── engine-direct-sales
│           ├── assets
│           │   ├── engine-vendor.js
│           │   └── engine.js
│           ├── config
│           │   └── environment.js
│           └── translations
│               └── de-de-x-engine.json
└── translations
    └── de-de-x-host.json

Actual Output

dist
├── asset-manifest.json
├── assets
│   ├── client.js
│   └── vendor.js
├── engines-dist
│   └── @clark
│       └── engine-direct-sales
│           ├── assets
│           │   ├── engine-vendor.js
│           │   └── engine.js
│           └── config
│               └── environment.js
└── translations
    ├── de-de-x-host.json
    └── de-de-x-engine.json

Problem

If the engine and host app would share the same locale name, e.g. just de-de, one would clobber the other. Moving this to the "root" also makes it harder for an engine to reliably load the translation, I would assume.

I think this is just an oversight, since ember-intl has rarely been used with ember-engines.

Solution

We special-handle engines and merge into treeForAddon instead of treeForApp. In general, we might want to allow addons that specify ember-intl as a dependency to configure this.

@jasonmit
Copy link
Member

jasonmit commented Feb 22, 2019

Disabling publicOnly also doesn't work, because the translation is not bundled into engine.js or engine-vendor.js, but seemingly gets merged into the host app's app tree

Yeah, it takes the same path as an addon so the behavior of bundling into the app is expected. This works sufficiently for most, but I could see how someone might expect scoping of some sort and the translations bundled into the engine bundle.

That said, the behavior of publicOnly: true sounds like something we should definitely fix.

@buschtoens buschtoens self-assigned this Mar 1, 2019
@brettburley
Copy link

I've been playing around with ember-intl with ember-engines a bit to see what the current behavior is.

@buschtoens does your engine itself list ember-intl as one of its dependencies in the package.json? In the testing I've done, I've found that as long as it's listed there, both variants of publicOnly will bundle the translations into either the engine.js or engines-dist/engine-name/translations/ files as appropriate. However, it will also hoist them into the app's translation files as well because of the general addon behavior @jasonmit described.

Including this issue, here's a list of current behavior that was unexpected to me when using ember-intl with engines:

  1. The host application gets all engine's translations bundled into its translation files.
  2. Each engine has a translation file that includes the same set as the host app (i.e. all of the host app's translations, all of the host app's engines' translations, and all of the host app's addons' translations).
  3. ember-intl uses the config for the host app when building translations for each engine (e.g. if the host app is publicOnly true, this will apply to all engines, regardless of what ember-intl config they may define locally).

The first issue is a result of treating all addons the same, as @jasonmit mentioned, in buildTranslationTree: https://github.com/ember-intl/ember-intl/blob/master/lib/broccoli/build-translation-tree.js#L15, and the later two issues are a result of using this.app and this.project in https://github.com/ember-intl/ember-intl/blob/master/index.js#L50 which point to the host app, rather than using this.owner which would represent the engine's own config/path. I think for engines that the current behavior of bundling into the host app is superfluous, since those keys are likely not used, but not sure how folks might currently be using this behavior and how many applications might break as a result.

Given that the local intl service in an engine will look up translations only for the engine (since config.modulePrefix is the engine name), I think I would expect that translations would be scoped to the engine. Also @buschtoens, I think the current behavior of using the treeForApp hook even for engines is correct since ember will put these files into the engine's namespace already (since the all of the app's modules are namespaced). I think that I would also expect the engine's ember-intl config to be respected since the engine has to make assumptions on how to initialize the service and potentially load translations depending on that setting. If it depended on the host apps config, the engine wouldn't know if it needs to fetch its own translations, for example.

@catz
Copy link

catz commented Sep 16, 2020

Hello everyone,

My question is a bit opposite. I do see translations as a copy of translation folder from the host app in every in-repo engine. can I somehow disable this generation? We load them manually (publicOnly: true) and honestly I see no reason in this sort of "duplication". What do you think?

@catz
Copy link

catz commented Sep 20, 2020

In our app all our engines are lazy + nested lazy. So, ember build creates a copy of translation folders in every engine.
This seems strange for me. Why do we need it? And as I said we load them manually in host app via service, publicOnly = true.
I checked https://github.com/rancher/ui repo that heavily uses lazy engines too and when I built it I didn't see translations in
engines-dist/* folders. That was surprise. What's the difference?
The solution here is that Rancher has one non-lazy engine and when I added the same dummy non-lazy engine, translations in engines-dist/* disappeared. One important moment this dummy engine needs to have "ember-intl": "*" in their package.json

I created a test repo https://github.com/catz/lazy-intl-frontend

  1. all engines are lazy
catz@MacBook-Pro-2 lazy-intl-frontend :) [../lazy-intl-frontend] git:master $ ember build --environment=production
catz@MacBook-Pro-2 lazy-intl-frontend :) [../lazy-intl-frontend] git:master $ ls dist/engines-dist/profile/
assets		config		translations
  1. lib/dummy/index.js lazyLoading -> false
catz@MacBook-Pro-2 lazy-intl-frontend :) [../lazy-intl-frontend] git:master $ ember build --environment=production
catz@MacBook-Pro-2 lazy-intl-frontend :) [../lazy-intl-frontend] git:master $ ls dist/engines-dist/profile/
assets	config

I suppose there might be some reason to do builds that way but I can not figure out it especially when publicOnly is true. Does that all make sense ?

@srsgores
Copy link

srsgores commented Oct 5, 2021

Has anyone found a workaround for this? We're about to migrate over to engines and would like a solution if anyone has one.

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

5 participants