fix(@schematics/angular): use the JSON AST for changing JSON files in migration

hansl · hansl · commit df09951c3f06 · 2018-06-05T17:55:18.000-07:00
Fix #10396
diff --git a/packages/schematics/angular/migrations/update-6/index.ts b/packages/schematics/angular/migrations/update-6/index.ts
@@ -5,7 +5,16 @@
  * 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 { JsonObject, Path, join, normalize, tags } from '@angular-devkit/core';
+import {
+  JsonObject,
+  JsonParseMode,
+  Path,
+  join,
+  normalize,
+  parseJson,
+  parseJsonAst,
+  tags,
+} from '@angular-devkit/core';
 import {
   Rule,
   SchematicContext,
@@ -16,6 +25,11 @@ import {
 import { NodePackageInstallTask } from '@angular-devkit/schematics/tasks';
 import { AppConfig, CliConfig } from '../../utility/config';
 import { latestVersions } from '../../utility/latest-versions';
+import {
+  appendPropertyInAstObject,
+  appendValueInAstArray,
+  findPropertyInAstObject,
+} from './json-utils';
 
 const defaults = {
   appRoot: 'src',
@@ -492,8 +506,6 @@ function extractProjectsConfig(config: CliConfig, tree: Tree): JsonObject {
         root: project.root,
         sourceRoot: project.root,
         projectType: 'application',
-        cli: {},
-        schematics: {},
       };
 
       const e2eArchitect: JsonObject = {};
@@ -542,51 +554,91 @@ function updateSpecTsConfig(config: CliConfig): Rule {
   return (host: Tree, context: SchematicContext) => {
     const apps = config.apps || [];
     apps.forEach((app: AppConfig, idx: number) => {
-      const tsSpecConfigPath =
-        join(app.root as Path, app.testTsconfig || defaults.testTsConfig);
+      const testTsConfig = app.testTsconfig || defaults.testTsConfig;
+      const tsSpecConfigPath = join(normalize(app.root || ''), testTsConfig);
       const buffer = host.read(tsSpecConfigPath);
+
       if (!buffer) {
         return;
       }
-      const tsCfg = JSON.parse(buffer.toString());
-      if (!tsCfg.files) {
-        tsCfg.files = [];
+
+
+      const tsCfgAst = parseJsonAst(buffer.toString(), JsonParseMode.Loose);
+      if (tsCfgAst.kind != 'object') {
+        throw new SchematicsException('Invalid tsconfig. Was expecting an object');
       }
 
-      // Ensure the spec tsconfig contains the polyfills file
-      if (tsCfg.files.indexOf(app.polyfills || defaults.polyfills) === -1) {
-        tsCfg.files.push(app.polyfills || defaults.polyfills);
-        host.overwrite(tsSpecConfigPath, JSON.stringify(tsCfg, null, 2));
+      const filesAstNode = findPropertyInAstObject(tsCfgAst, 'files');
+      if (filesAstNode && filesAstNode.kind != 'array') {
+        throw new SchematicsException('Invalid tsconfig "files" property; expected an array.');
       }
+
+      const recorder = host.beginUpdate(tsSpecConfigPath);
+
+      const polyfills = app.polyfills || defaults.polyfills;
+      if (!filesAstNode) {
+        // Do nothing if the files array does not exist. This means exclude or include are
+        // set and we shouldn't mess with that.
+      } else {
+        if (filesAstNode.value.indexOf(polyfills) == -1) {
+          appendValueInAstArray(recorder, filesAstNode, polyfills);
+        }
+      }
+
+      host.commitUpdate(recorder);
     });
   };
 }
 
-function updatePackageJson(packageManager?: string) {
+function updatePackageJson(config: CliConfig) {
   return (host: Tree, context: SchematicContext) => {
     const pkgPath = '/package.json';
     const buffer = host.read(pkgPath);
     if (buffer == null) {
       throw new SchematicsException('Could not read package.json');
     }
-    const content = buffer.toString();
-    const pkg = JSON.parse(content);
+    const pkgAst = parseJsonAst(buffer.toString(), JsonParseMode.Strict);
 
-    if (pkg === null || typeof pkg !== 'object' || Array.isArray(pkg)) {
+    if (pkgAst.kind != 'object') {
       throw new SchematicsException('Error reading package.json');
     }
-    if (!pkg.devDependencies) {
-      pkg.devDependencies = {};
+
+    const devDependenciesNode = findPropertyInAstObject(pkgAst, 'devDependencies');
+    if (devDependenciesNode && devDependenciesNode.kind != 'object') {
+      throw new SchematicsException('Error reading package.json; devDependency is not an object.');
     }
 
-    pkg.devDependencies['@angular-devkit/build-angular'] = latestVersions.DevkitBuildAngular;
+    const recorder = host.beginUpdate(pkgPath);
+    const depName = '@angular-devkit/build-angular';
+    if (!devDependenciesNode) {
+      // Haven't found the devDependencies key, add it to the root of the package.json.
+      appendPropertyInAstObject(recorder, pkgAst, 'devDependencies', {
+        [depName]: latestVersions.DevkitBuildAngular,
+      });
+    } else {
+      // Check if there's a build-angular key.
+      const buildAngularNode = findPropertyInAstObject(devDependenciesNode, depName);
+
+      if (!buildAngularNode) {
+        // No build-angular package, add it.
+        appendPropertyInAstObject(
+          recorder,
+          devDependenciesNode,
+          depName,
+          latestVersions.DevkitBuildAngular,
+        );
+      } else {
+        const { end, start } = buildAngularNode;
+        recorder.remove(start.offset, end.offset - start.offset);
+        recorder.insertRight(start.offset, JSON.stringify(latestVersions.DevkitBuildAngular));
+      }
+    }
 
-    host.overwrite(pkgPath, JSON.stringify(pkg, null, 2));
+    host.commitUpdate(recorder);
 
-    if (packageManager && !['npm', 'yarn', 'cnpm'].includes(packageManager)) {
-      packageManager = undefined;
-    }
-    context.addTask(new NodePackageInstallTask({ packageManager }));
+    context.addTask(new NodePackageInstallTask({
+      packageManager: config.packageManager === 'default' ? undefined : config.packageManager,
+    }));
 
     return host;
   };
@@ -597,19 +649,52 @@ function updateTsLintConfig(): Rule {
     const tsLintPath = '/tslint.json';
     const buffer = host.read(tsLintPath);
     if (!buffer) {
-      return;
+      return host;
     }
-    const tsCfg = JSON.parse(buffer.toString());
+    const tsCfgAst = parseJsonAst(buffer.toString(), JsonParseMode.Loose);
 
-    if (tsCfg.rules && tsCfg.rules['import-blacklist'] &&
-        tsCfg.rules['import-blacklist'].indexOf('rxjs') !== -1) {
+    if (tsCfgAst.kind != 'object') {
+      return host;
+    }
 
-      tsCfg.rules['import-blacklist'] = tsCfg.rules['import-blacklist']
-        .filter((rule: string | boolean) => rule !== 'rxjs');
+    const rulesNode = findPropertyInAstObject(tsCfgAst, 'rules');
+    if (!rulesNode || rulesNode.kind != 'object') {
+      return host;
+    }
 
-      host.overwrite(tsLintPath, JSON.stringify(tsCfg, null, 2));
+    const importBlacklistNode = findPropertyInAstObject(rulesNode, 'import-blacklist');
+    if (!importBlacklistNode || importBlacklistNode.kind != 'array') {
+      return host;
     }
 
+    const recorder = host.beginUpdate(tsLintPath);
+    for (let i = 0; i < importBlacklistNode.elements.length; i++) {
+      const element = importBlacklistNode.elements[i];
+      if (element.kind == 'string' && element.value == 'rxjs') {
+        const { start, end } = element;
+        // Remove this element.
+        if (i == importBlacklistNode.elements.length - 1) {
+          // Last element.
+          if (i > 0) {
+            // Not first, there's a comma to remove before.
+            const previous = importBlacklistNode.elements[i - 1];
+            recorder.remove(previous.end.offset, end.offset - previous.end.offset);
+          } else {
+            // Only element, just remove the whole rule.
+            const { start, end } = importBlacklistNode;
+            recorder.remove(start.offset, end.offset - start.offset);
+            recorder.insertLeft(start.offset, '[]');
+          }
+        } else {
+          // Middle, just remove the whole node (up to next node start).
+          const next = importBlacklistNode.elements[i + 1];
+          recorder.remove(start.offset, next.start.offset - start.offset);
+        }
+      }
+    }
+
+    host.commitUpdate(recorder);
+
     return host;
   };
 }
@@ -627,13 +712,17 @@ export default function (): Rule {
     if (configBuffer == null) {
       throw new SchematicsException(`Could not find configuration file (${configPath})`);
     }
-    const config = JSON.parse(configBuffer.toString());
+    const config = parseJson(configBuffer.toString(), JsonParseMode.Loose);
+
+    if (typeof config != 'object' || Array.isArray(config) || config === null) {
+      throw new SchematicsException('Invalid angular-cli.json configuration; expected an object.');
+    }
 
     return chain([
       migrateKarmaConfiguration(config),
       migrateConfiguration(config),
       updateSpecTsConfig(config),
-      updatePackageJson(config.packageManager),
+      updatePackageJson(config),
       updateTsLintConfig(),
       (host: Tree, context: SchematicContext) => {
         context.logger.warn(tags.oneLine`Some configuration options have been changed,
diff --git a/packages/schematics/angular/migrations/update-6/index_spec.ts b/packages/schematics/angular/migrations/update-6/index_spec.ts
@@ -9,6 +9,7 @@ import { JsonObject } from '@angular-devkit/core';
 import { EmptyTree } from '@angular-devkit/schematics';
 import { SchematicTestRunner, UnitTestTree } from '@angular-devkit/schematics/testing';
 import * as path from 'path';
+import { latestVersions } from '../../utility/latest-versions';
 
 
 describe('Migration to v6', () => {
@@ -691,7 +692,32 @@ describe('Migration to v6', () => {
       tree = schematicRunner.runSchematic('migration-01', defaultOptions, tree);
       const content = tree.readContent('/package.json');
       const pkg = JSON.parse(content);
-      expect(pkg.devDependencies['@angular-devkit/build-angular']).toBeDefined();
+      expect(pkg.devDependencies['@angular-devkit/build-angular'])
+        .toBe(latestVersions.DevkitBuildAngular);
+    });
+
+    it('should add a dev dependency to @angular-devkit/build-angular (present)', () => {
+      tree.create(oldConfigPath, JSON.stringify(baseConfig, null, 2));
+      tree.overwrite('/package.json', JSON.stringify({
+        devDependencies: {
+          '@angular-devkit/build-angular': '0.0.0',
+        },
+      }, null, 2));
+      tree = schematicRunner.runSchematic('migration-01', defaultOptions, tree);
+      const content = tree.readContent('/package.json');
+      const pkg = JSON.parse(content);
+      expect(pkg.devDependencies['@angular-devkit/build-angular'])
+        .toBe(latestVersions.DevkitBuildAngular);
+    });
+
+    it('should add a dev dependency to @angular-devkit/build-angular (no dev)', () => {
+      tree.create(oldConfigPath, JSON.stringify(baseConfig, null, 2));
+      tree.overwrite('/package.json', JSON.stringify({}, null, 2));
+      tree = schematicRunner.runSchematic('migration-01', defaultOptions, tree);
+      const content = tree.readContent('/package.json');
+      const pkg = JSON.parse(content);
+      expect(pkg.devDependencies['@angular-devkit/build-angular'])
+        .toBe(latestVersions.DevkitBuildAngular);
     });
   });
 
@@ -702,7 +728,7 @@ describe('Migration to v6', () => {
     beforeEach(() => {
       tslintConfig = {
         rules: {
-          'import-blacklist': ['rxjs'],
+          'import-blacklist': ['some', 'rxjs', 'else'],
         },
       };
     });
@@ -712,8 +738,34 @@ describe('Migration to v6', () => {
       tree.create(tslintPath, JSON.stringify(tslintConfig, null, 2));
       tree = schematicRunner.runSchematic('migration-01', defaultOptions, tree);
       const tslint = JSON.parse(tree.readContent(tslintPath));
-      const blacklist = tslint.rules['import-blacklist'];
-      expect(blacklist).toEqual([]);
+      expect(tslint.rules['import-blacklist']).toEqual(['some', 'else']);
+    });
+
+    it('should remove "rxjs" from the "import-blacklist" rule (only)', () => {
+      tree.create(oldConfigPath, JSON.stringify(baseConfig, null, 2));
+      tslintConfig.rules['import-blacklist'] = ['rxjs'];
+      tree.create(tslintPath, JSON.stringify(tslintConfig, null, 2));
+      tree = schematicRunner.runSchematic('migration-01', defaultOptions, tree);
+      const tslint = JSON.parse(tree.readContent(tslintPath));
+      expect(tslint.rules['import-blacklist']).toEqual([]);
+    });
+
+    it('should remove "rxjs" from the "import-blacklist" rule (first)', () => {
+      tree.create(oldConfigPath, JSON.stringify(baseConfig, null, 2));
+      tslintConfig.rules['import-blacklist'] = ['rxjs', 'else'];
+      tree.create(tslintPath, JSON.stringify(tslintConfig, null, 2));
+      tree = schematicRunner.runSchematic('migration-01', defaultOptions, tree);
+      const tslint = JSON.parse(tree.readContent(tslintPath));
+      expect(tslint.rules['import-blacklist']).toEqual(['else']);
+    });
+
+    it('should remove "rxjs" from the "import-blacklist" rule (last)', () => {
+      tree.create(oldConfigPath, JSON.stringify(baseConfig, null, 2));
+      tslintConfig.rules['import-blacklist'] = ['some', 'rxjs'];
+      tree.create(tslintPath, JSON.stringify(tslintConfig, null, 2));
+      tree = schematicRunner.runSchematic('migration-01', defaultOptions, tree);
+      const tslint = JSON.parse(tree.readContent(tslintPath));
+      expect(tslint.rules['import-blacklist']).toEqual(['some']);
     });
 
     it('should work if "rxjs" is not in the "import-blacklist" rule', () => {
diff --git a/packages/schematics/angular/migrations/update-6/json-utils.ts b/packages/schematics/angular/migrations/update-6/json-utils.ts
@@ -0,0 +1,70 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * 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 { JsonAstArray, JsonAstNode, JsonAstObject, JsonValue } from '@angular-devkit/core';
+import { UpdateRecorder } from '@angular-devkit/schematics';
+
+export function appendPropertyInAstObject(
+  recorder: UpdateRecorder,
+  node: JsonAstObject,
+  propertyName: string,
+  value: JsonValue,
+  indent = 4,
+) {
+  const indentStr = '\n' + new Array(indent + 1).join(' ');
+
+  if (node.properties.length > 0) {
+    // Insert comma.
+    const last = node.properties[node.properties.length - 1];
+    recorder.insertRight(last.start.offset + last.text.replace(/\s+$/, '').length, ',');
+  }
+
+  recorder.insertLeft(
+    node.end.offset - 1,
+    '  '
+    + `"${propertyName}": ${JSON.stringify(value, null, 2).replace(/\n/g, indentStr)}`
+    + indentStr.slice(0, -2),
+  );
+}
+
+
+export function appendValueInAstArray(
+  recorder: UpdateRecorder,
+  node: JsonAstArray,
+  value: JsonValue,
+  indent = 4,
+) {
+  const indentStr = '\n' + new Array(indent + 1).join(' ');
+
+  if (node.elements.length > 0) {
+    // Insert comma.
+    const last = node.elements[node.elements.length - 1];
+    recorder.insertRight(last.start.offset + last.text.replace(/\s+$/, '').length, ',');
+  }
+
+  recorder.insertLeft(
+    node.end.offset - 1,
+    '  '
+    + JSON.stringify(value, null, 2).replace(/\n/g, indentStr)
+    + indentStr.slice(0, -2),
+  );
+}
+
+
+export function findPropertyInAstObject(
+  node: JsonAstObject,
+  propertyName: string,
+): JsonAstNode | null {
+  let maybeNode: JsonAstNode | null = null;
+  for (const property of node.properties) {
+    if (property.key.value == propertyName) {
+      maybeNode = property.value;
+    }
+  }
+
+  return maybeNode;
+}
