fix(@schematics/angular): improve i18n output path option migration

Output paths that use the locale within a locale specific configuration will now be automatically removed.  This will prevent the potential for the migrated configuration to generate an output path with double locale segments.

Author: clydin

packages/schematics/angular/migrations/update-9/update-workspace-config.ts

@@ -5,8 +5,9 @@
  * Use of this source code is governed by an MIT-style license that can be
  * found in the LICENSE file at https://angular.io/license
  */
-import { JsonAstObject } from '@angular-devkit/core';
+import { JsonAstObject, logging } from '@angular-devkit/core';
 import { Rule, Tree, UpdateRecorder } from '@angular-devkit/schematics';
+import { posix } from 'path';
 import { getWorkspacePath } from '../../utility/config';
 import { NodeDependencyType, addPackageJsonDependency, getPackageJsonDependency } from '../../utility/dependencies';
 import {
@@ -25,23 +26,23 @@ export const ANY_COMPONENT_STYLE_BUDGET = {
 };
 
 export function updateWorkspaceConfig(): Rule {
-  return (tree: Tree) => {
+  return (tree, context) => {
     const workspacePath = getWorkspacePath(tree);
     const workspace = getWorkspace(tree);
     const recorder = tree.beginUpdate(workspacePath);
 
-    for (const { target, project } of getTargets(workspace, 'build', Builders.Browser)) {
+    for (const { target } of getTargets(workspace, 'build', Builders.Browser)) {
       updateStyleOrScriptOption('styles', recorder, target);
       updateStyleOrScriptOption('scripts', recorder, target);
       addAnyComponentStyleBudget(recorder, target);
       updateAotOption(tree, recorder, target);
-      addBuilderI18NOptions(recorder, target, project);
+      addBuilderI18NOptions(recorder, target, context.logger);
     }
 
-    for (const { target, project } of getTargets(workspace, 'test', Builders.Karma)) {
+    for (const { target } of getTargets(workspace, 'test', Builders.Karma)) {
       updateStyleOrScriptOption('styles', recorder, target);
       updateStyleOrScriptOption('scripts', recorder, target);
-      addBuilderI18NOptions(recorder, target, project);
+      addBuilderI18NOptions(recorder, target, context.logger);
     }
 
     for (const { target } of getTargets(workspace, 'server', Builders.Server)) {
@@ -148,7 +149,11 @@ function addProjectI18NOptions(
   }
 }
 
-function addBuilderI18NOptions(recorder: UpdateRecorder, builderConfig: JsonAstObject, projectConfig: JsonAstObject) {
+function addBuilderI18NOptions(
+  recorder: UpdateRecorder,
+  builderConfig: JsonAstObject,
+  logger: logging.LoggerApi,
+) {
   const options = getAllOptions(builderConfig);
   const mainOptions = findPropertyInAstObject(builderConfig, 'options');
   const mainBaseHref =
@@ -160,31 +165,75 @@ function addBuilderI18NOptions(recorder: UpdateRecorder, builderConfig: JsonAstO
 
   for (const option of options) {
     const localeId = findPropertyInAstObject(option, 'i18nLocale');
+    const i18nFile = findPropertyInAstObject(option, 'i18nFile');
+
+    // The format is always auto-detected now
+    const i18nFormat = findPropertyInAstObject(option, 'i18nFormat');
+    if (i18nFormat) {
+      removePropertyInAstObject(recorder, option, 'i18nFormat');
+    }
+
+    const outputPath = findPropertyInAstObject(option, 'outputPath');
+    if (
+      localeId &&
+      localeId.kind === 'string' &&
+      i18nFile &&
+      outputPath &&
+      outputPath.kind === 'string'
+    ) {
+      // This first block was intended to remove the redundant output path field
+      // but due to defects in the recorder, removing the option will cause malformed json
+      // if (
+      //   mainOutputPathValue &&
+      //   outputPath.value.match(
+      //     new RegExp(`[/\\\\]?${mainOutputPathValue}[/\\\\]${localeId.value}[/\\\\]?$`),
+      //   )
+      // ) {
+      //   removePropertyInAstObject(recorder, option, 'outputPath');
+      // } else
+      if (outputPath.value.match(new RegExp(`[/\\\\]${localeId.value}[/\\\\]?$`))) {
+        const newOutputPath = outputPath.value.replace(
+          new RegExp(`[/\\\\]${localeId.value}[/\\\\]?$`),
+          '',
+        );
+        const { start, end } = outputPath;
+        recorder.remove(start.offset, end.offset - start.offset);
+        recorder.insertLeft(start.offset, `"${newOutputPath}"`);
+      } else {
+        logger.warn(
+          `Output path value "${outputPath.value}" for locale "${localeId.value}" is not supported with the new localization system. ` +
+            `With the current value, the localized output would be written to "${posix.join(
+              outputPath.value,
+              localeId.value,
+            )}". ` +
+            `Keeping existing options for the target configuration of locale "${localeId.value}".`,
+        );
+
+        continue;
+      }
+    }
+
     if (localeId && localeId.kind === 'string') {
       // add new localize option
       insertPropertyInAstObjectInOrder(recorder, option, 'localize', [localeId.value], 12);
       removePropertyInAstObject(recorder, option, 'i18nLocale');
     }
 
-    const i18nFile = findPropertyInAstObject(option, 'i18nFile');
     if (i18nFile) {
       removePropertyInAstObject(recorder, option, 'i18nFile');
     }
 
-    const i18nFormat = findPropertyInAstObject(option, 'i18nFormat');
-    if (i18nFormat) {
-      removePropertyInAstObject(recorder, option, 'i18nFormat');
-    }
-
     // localize base HREF values are controlled by the i18n configuration
     const baseHref = findPropertyInAstObject(option, 'baseHref');
     if (localeId && i18nFile && baseHref) {
-      removePropertyInAstObject(recorder, option, 'baseHref');
-
       // if the main option set has a non-default base href,
       // ensure that the augmented base href has the correct base value
       if (hasMainBaseHref) {
-        insertPropertyInAstObjectInOrder(recorder, option, 'baseHref', '/', 12);
+        const { start, end } = baseHref;
+        recorder.remove(start.offset, end.offset - start.offset);
+        recorder.insertLeft(start.offset, `"/"`);
+      } else {
+        removePropertyInAstObject(recorder, option, 'baseHref');
       }
     }
   }

packages/schematics/angular/migrations/update-9/update-workspace-config_spec.ts

@@ -301,7 +301,7 @@ describe('Migration to version 9', () => {
     describe('i18n configuration', () => {
       function getI18NConfig(localId: string): object {
         return {
-          outputPath: `dist/my-project-${localId}/`,
+          outputPath: `dist/my-project/${localId}/`,
           i18nFile: `src/locale/messages.${localId}.xlf`,
           i18nFormat: 'xlf',
           i18nLocale: localId,
@@ -362,6 +362,38 @@ describe('Migration to version 9', () => {
           expect(config.configurations.de.baseHref).toBeUndefined();
         });
 
+        it('should remove outputPath option when used with i18n options and contains locale code', async () => {
+          let config = getWorkspaceTargets(tree);
+          config.build.options.outputPath = 'dist';
+          config.build.configurations.fr = { ...getI18NConfig('fr'), outputPath: 'dist/fr' };
+          config.build.configurations.de = { ...getI18NConfig('de'), outputPath: '/dist/de/' };
+          updateWorkspaceTargets(tree, config);
+
+          const tree2 = await schematicRunner.runSchematicAsync('workspace-version-9', {}, tree.branch()).toPromise();
+          config = getWorkspaceTargets(tree2).build;
+          expect(config.configurations.fr.outputPath).toBe('dist');
+          expect(config.configurations.de.outputPath).toBe('/dist');
+        });
+
+        it('should keep old i18n options except format when output path is not supported', async () => {
+          let config = getWorkspaceTargets(tree);
+          config.build.options.outputPath = 'dist';
+          config.build.configurations.fr = { ...getI18NConfig('fr'), outputPath: 'dist/abc' };
+          config.build.configurations.de = { ...getI18NConfig('de'), outputPath: '/dist/123' };
+          updateWorkspaceTargets(tree, config);
+
+          const tree2 = await schematicRunner.runSchematicAsync('workspace-version-9', {}, tree.branch()).toPromise();
+          config = getWorkspaceTargets(tree2).build;
+          expect(config.configurations.fr.outputPath).toBe('dist/abc');
+          expect(config.configurations.fr.i18nFormat).toBeUndefined();
+          expect(config.configurations.fr.i18nFile).toBeDefined();
+          expect(config.configurations.fr.i18nLocale).toBe('fr');
+          expect(config.configurations.de.outputPath).toBe('/dist/123');
+          expect(config.configurations.de.i18nFormat).toBeUndefined();
+          expect(config.configurations.de.i18nFile).toBeDefined();
+          expect(config.configurations.de.i18nLocale).toBe('de');
+        });
+
         it('should keep baseHref option when not used with i18n options', async () => {
           let config = getWorkspaceTargets(tree);
           config.build.options = getI18NConfig('fr');