feat(compiler-cli): add i18n parameters to tsconfig #16365

ocombe · 2017-04-27T07:50:19Z

What kind of change does this PR introduce?

[x] Feature

What is the current behavior? (You can also link to an open issue here)
Right now you cannot add existing i18n parameters to tsconfig, see issue #16232

What is the new behavior?
You can now add parameters to your tsconfig that will be used when you don't provide cli parameters. It works with ng-xi18n and ngc. Since they both have some parameters with a similar name, those options in tsconfig have a slightly different name compared to the cli parameters (with the prefix source for ng-xi18n and the prefix target for ngc).

Does this PR introduce a breaking change? (check one with "x")

[x] No

vicb · 2017-04-27T11:39:16Z

tools/@angular/tsc-wrapped/src/options.ts

+  targetI18nFormat?: string;
+
+  // locale for imported translation files
+  // you should only use this if you have one tsconfig file per locale


you can remove this line "you should ..."
same below

vicb · 2017-04-27T11:40:46Z

packages/compiler-cli/src/codegen.ts

@@ -73,8 +73,8 @@ export class CodeGenerator {
    }
    const {compiler: aotCompiler} = compiler.createAotCompiler(ngCompilerHost, {
      translations: transContent,
-      i18nFormat: cliOptions.i18nFormat,
-      locale: cliOptions.locale,
+      i18nFormat: cliOptions.i18nFormat || options.targetI18nFormat,


|| sourceFormat at the end (useful when they are the same)

For xmb/xtb source & target format are different, this could lead to bugs. The only case where this would work would be for xlf and it's already the default format.

vicb · 2017-04-27T11:41:39Z

packages/compiler-cli/src/extract_i18n.ts

@@ -23,7 +23,9 @@ function extract(
    ngOptions: tsc.AngularCompilerOptions, cliOptions: tsc.I18nExtractionCliOptions,
    program: ts.Program, host: ts.CompilerHost): Promise<void> {
  return Extractor.create(ngOptions, program, host, cliOptions.locale)
-      .extract(cliOptions.i18nFormat !, cliOptions.outFile);
+      .extract(
+          (cliOptions.i18nFormat || ngOptions.sourceI18nFormat) !,


Please take the opportunity of this PR to clean the (...)! hack.

vicb · 2017-04-27T11:43:14Z

tools/@angular/tsc-wrapped/src/options.ts

@@ -81,6 +81,26 @@ interface Options extends ts.CompilerOptions {

  // Whether to enable support for <template> and the template attribute (true by default)
  enableLegacyTemplate?: boolean;
+
+  // format for extracted messages file (xlf, xmb)
+  sourceI18nFormat?: string;


As a general comment on this PR, "source" is confusing because it relates to the file being written which is counter-intuitive - same for "target".

Can you think about better names ?

vicb

See inline comments + need tests

vicb · 2017-04-28T12:13:33Z

packages/compiler-cli/src/codegen.ts

+    const optMissingTranslation = cliOptions.missingTranslation || options.missingTranslation;
+    const transFile = cliOptions.i18nFile || options.i18nFile;
+    let translations: string = '';
+    if (transFile) {
      if (!cliOptions.locale) {


should be locale ?

I wanted to be sure if you really paid attention to the code.

Jokes aside, good catch, fixed

vicb · 2017-04-28T12:16:03Z

packages/compiler-cli/src/codegen.ts

-      i18nFormat: cliOptions.i18nFormat,
-      locale: cliOptions.locale, missingTranslation,
+      translations,
+      i18nFormat: cliOptions.i18nFormat || options.i18nTranslationFormat,


move this to the top with other parameters ?
~~explain why we do || options.i18nTranslationFormat (ie most files contain both messages and translations)~~

vicb · 2017-04-28T12:17:00Z

packages/compiler-cli/src/extract_i18n.ts

@@ -23,7 +23,9 @@ function extract(
    ngOptions: tsc.AngularCompilerOptions, cliOptions: tsc.I18nExtractionCliOptions,
    program: ts.Program, host: ts.CompilerHost): Promise<void> {
  return Extractor.create(ngOptions, program, host, cliOptions.locale)
-      .extract(cliOptions.i18nFormat !, cliOptions.outFile);
+      .extract(
+          cliOptions.i18nFormat || ngOptions.i18nFormat || null,


same thing here, move init to the top, missing i18nTranslationFormat here

format could not be null -> || '' at the end

forget my first comment, you have updated the API

We don't use "i18nTranslationFormat" for the extraction (this is ng-xi18n, not ngc)

vicb · 2017-04-28T12:18:14Z

packages/compiler-cli/src/extract_i18n.ts

-      .extract(cliOptions.i18nFormat !, cliOptions.outFile);
+      .extract(
+          cliOptions.i18nFormat || ngOptions.i18nFormat || null,
+          cliOptions.outFile || ngOptions.outFile || null);


move init at the top

vicb · 2017-04-28T12:21:02Z

tools/@angular/tsc-wrapped/src/options.ts

+  // path & name of your translations file
+  i18nFile?: string;
+
+  // strategy to use for missing translations


specify the possible values in the comment

vicb

see inline comments,
add tests

vicb · 2017-05-01T13:42:08Z

packages/compiler-cli/src/extract_i18n.ts

@@ -22,8 +22,9 @@ import {Extractor} from './extractor';
 function extract(
    ngOptions: tsc.AngularCompilerOptions, cliOptions: tsc.I18nExtractionCliOptions,
    program: ts.Program, host: ts.CompilerHost): Promise<void> {
-  return Extractor.create(ngOptions, program, host, cliOptions.locale)
-      .extract(cliOptions.i18nFormat !, cliOptions.outFile);
+  const format = cliOptions.i18nFormat || ngOptions.i18nFormat || null;


set the default to xliff

vicb · 2017-05-01T13:42:40Z

packages/compiler-cli/src/extractor.ts

@@ -27,7 +27,7 @@ export class Extractor {
      public host: ts.CompilerHost, private ngCompilerHost: CompilerHost,
      private program: ts.Program) {}

-  extract(formatName: string, outFile: string|null): Promise<void> {
+  extract(formatName: string|null, outFile: string|null): Promise<void> {


revert |null

also for the other methods if possible

vicb · 2017-05-01T13:46:22Z

packages/compiler-cli/src/main.ts

-    args: any, consoleError: (s: string) => void = console.error): Promise<number> {
+    args: any, consoleError: (s: string) => void = console.error,
+    codegen:
+        (n: tsc.AngularCompilerOptions, c: tsc.NgcCliOptions, p: ts.Program, h: ts.CompilerHost) =>


why this change ?

It's the only way that I found to add the unit test. I can remove it, but I'll have to remove the test as well

vicb · 2017-05-01T13:46:56Z

packages/compiler-cli/src/ngtools_api.ts

@@ -150,6 +150,6 @@ export class NgTools_InternalApi_NG_2 {
    const extractor = Extractor.create(
        options.angularCompilerOptions, options.program, options.host, locale, hostContext);

-    return extractor.extract(options.i18nFormat !, options.outFile || null);
+    return extractor.extract(options.i18nFormat || null, options.outFile || null);


same, add xliff by default

vicb · 2017-05-10T14:29:21Z

LGTM, needed before merge:

@ocombe to update the tests according to discussions,
travis should be green (the failure does not seem to relate to the changes).

You can now add parameters to your tsconfig that will be used when you don't provide cli parameters. It works with ng-xi18n and ngc. Since they both use some parameters with a similar name, those options in tsconfig can have a slightly different name compared to the cli parameters. Closes angular#16232 Fixes angular#16235

vicb · 2017-05-15T23:31:59Z

Blocked because missing approval.

@alexeagle could you please review. Thanks.

alexeagle

/cc @vikerman

alexeagle · 2017-05-16T01:24:33Z

tools/@angular/tsc-wrapped/src/options.ts

+  locale?: string;
+
+  // path & name for the extracted messages file
+  outFile?: string;


this is probably confusing.

"compilerOptions": { "outDir": "foo", "out": "bar" }, "angularCompilerOptions": { "outFile": "baz" }

I think either these options need to have i18n in the name, or be nested:

"angularCompilerOptions": { "i18n_msgs": { "extract": { "outFile": "msgs.xtb" }, "merge": { "i18nFile": "msgs.xtb" } } }

WDYT?

We used the names of the current cli options (when possible). We can nest them though, what do you think @vicb ?

Isn't MapLike<string[]> the way to nest options? TypeScript does nest them, eg. compilerOptions.paths

alexeagle · 2017-05-16T01:25:20Z

tools/@angular/tsc-wrapped/src/options.ts

+  i18nFormat?: string;
+
+  // locale of your application for extracted messages file
+  locale?: string;


I was expecting multiple locales to appear here. Users shouldn't have N tsconfig files (and note that the tsconfig "extends" behavior doesn't cover unknown config properties like "angularCompilerOptions")

This is the locale used for extracted messages, which is the main locale of your application. There is only one here since you only do one extraction.
You are referring to translationLocale?: string; which is the locale used when you compile the application. I had a comment there that @vicb told me remove which was something like "only use this if you have one tsconfig file / locale".
I agree that it would be better if we could compile all locales in one command, but that is not possible yet. There's a bit refactor coming for the compiler and it will probably change at that point, but for now we can only compile for one locale.

googlebot · 2017-07-26T19:56:43Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

vikerman · 2017-07-27T03:29:41Z

Does this have google3 impact?

vicb · 2017-08-11T18:17:41Z

closing as implemented in the new 5.x compiler flow - see #18494

angular-automatic-lock-bot · 2019-09-12T20:51:52Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

ocombe added area: i18n action: review The PR is still awaiting reviews from at least one requested reviewer feature Issue that requests a new feature labels Apr 27, 2017

ocombe requested a review from vicb April 27, 2017 07:50

googlebot added the cla: yes label Apr 27, 2017

vicb reviewed Apr 27, 2017

View reviewed changes

vicb added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 27, 2017

ocombe force-pushed the feature-i18n-params-tsconfig-16232 branch from 7c6100a to 9d7d54e Compare April 28, 2017 12:03

ocombe mentioned this pull request Apr 28, 2017

fix(compiler-cli): set default format to xlf for serialize #16273

Closed

vicb reviewed Apr 28, 2017

View reviewed changes

ocombe force-pushed the feature-i18n-params-tsconfig-16232 branch from 9d7d54e to 6befe68 Compare April 28, 2017 12:14

vicb reviewed Apr 28, 2017

View reviewed changes

vicb suggested changes Apr 28, 2017

View reviewed changes

ocombe force-pushed the feature-i18n-params-tsconfig-16232 branch from 6befe68 to 8a24b8c Compare April 28, 2017 16:52

vicb reviewed May 1, 2017

View reviewed changes

ocombe force-pushed the feature-i18n-params-tsconfig-16232 branch from 8a24b8c to cbf49f1 Compare May 2, 2017 11:46

ocombe mentioned this pull request May 2, 2017

[i18n] plans #16477

Closed

20 tasks

vicb removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label May 10, 2017

vicb approved these changes May 10, 2017

View reviewed changes

ocombe force-pushed the feature-i18n-params-tsconfig-16232 branch from cbf49f1 to 71a7a3b Compare May 12, 2017 13:47

ocombe added the action: merge The PR is ready for merge by the caretaker label May 12, 2017

vicb added the state: blocked label May 15, 2017

alexeagle reviewed May 16, 2017

View reviewed changes

chuckjaz removed the action: merge The PR is ready for merge by the caretaker label May 19, 2017

mhevery force-pushed the master branch from 08b36cb to 5d4b36f Compare July 20, 2017 21:48

mhevery force-pushed the feature-i18n-params-tsconfig-16232 branch from ae67092 to 71a7a3b Compare July 26, 2017 19:56

googlebot added cla: no and removed cla: yes labels Jul 26, 2017

vicb closed this Aug 11, 2017

ocombe deleted the feature-i18n-params-tsconfig-16232 branch November 10, 2017 08:48

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compiler-cli): add i18n parameters to tsconfig #16365

feat(compiler-cli): add i18n parameters to tsconfig #16365

ocombe commented Apr 27, 2017

vicb Apr 27, 2017 •

edited

vicb Apr 27, 2017

ocombe Apr 28, 2017

vicb Apr 27, 2017

vicb Apr 27, 2017

vicb left a comment

vicb Apr 28, 2017

ocombe Apr 28, 2017

vicb Apr 28, 2017 •

edited

vicb Apr 28, 2017

vicb Apr 28, 2017

vicb Apr 28, 2017

ocombe Apr 28, 2017

vicb Apr 28, 2017

vicb Apr 28, 2017

vicb left a comment

vicb May 1, 2017

vicb May 1, 2017 •

edited

vicb May 1, 2017

ocombe May 2, 2017

vicb May 1, 2017

vicb commented May 10, 2017

vicb commented May 15, 2017

alexeagle left a comment

alexeagle May 16, 2017

ocombe May 16, 2017 •

edited

vicb Aug 5, 2017

alexeagle Aug 7, 2017

alexeagle May 16, 2017

ocombe May 16, 2017 •

edited

googlebot commented Jul 26, 2017

vikerman commented Jul 27, 2017

vicb commented Aug 11, 2017

angular-automatic-lock-bot bot commented Sep 12, 2019

feat(compiler-cli): add i18n parameters to tsconfig #16365

feat(compiler-cli): add i18n parameters to tsconfig #16365

Conversation

ocombe commented Apr 27, 2017

vicb Apr 27, 2017 • edited

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

vicb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Apr 28, 2017 • edited

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

vicb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb May 1, 2017 • edited

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

vicb commented May 10, 2017

vicb commented May 15, 2017

alexeagle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocombe May 16, 2017 • edited

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

ocombe May 16, 2017 • edited

Choose a reason for hiding this comment

googlebot commented Jul 26, 2017

vikerman commented Jul 27, 2017

vicb commented Aug 11, 2017

angular-automatic-lock-bot bot commented Sep 12, 2019

vicb Apr 27, 2017 •

edited

vicb Apr 28, 2017 •

edited

vicb May 1, 2017 •

edited

ocombe May 16, 2017 •

edited

ocombe May 16, 2017 •

edited