fix(@schematics/angular): JsonUtils should respect indent

This commit fixes a few issues with json-utils:

1. The implementation is lacking tests
2. The implementation hardcodes indent = 2 in many places and
   does not respect the `indent` parameter passed by users
3. The implementation is buggy when passed an empty object or array

Author: Keen Yee Liau

packages/schematics/angular/utility/json-utils.ts

@@ -22,18 +22,22 @@ export function appendPropertyInAstObject(
   indent: number,
 ) {
   const indentStr = _buildIndent(indent);
-
+  let index = node.start.offset + 1;
   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, ',');
+    const {text, end} = last;
+    const commaIndex = text.endsWith('\n') ? end.offset - 1 : end.offset;
+    recorder.insertRight(commaIndex, ',');
+    index = end.offset;
   }
-
-  recorder.insertLeft(
-    node.end.offset - 1,
-    '  '
-    + `"${propertyName}": ${JSON.stringify(value, null, 2).replace(/\n/g, indentStr)}`
-    + indentStr.slice(0, -2),
+  const content = JSON.stringify(value, null, indent).replace(/\n/g, indentStr);
+  recorder.insertRight(
+    index,
+    (node.properties.length === 0 && indent ? '\n' : '')
+    + ' '.repeat(indent)
+    + `"${propertyName}":${indent ? ' ' : ''}${content}`
+    + indentStr.slice(0, -indent),
   );
 }
 
@@ -77,15 +81,14 @@ export function insertPropertyInAstObjectInOrder(
   }
 
   const indentStr = _buildIndent(indent);
-
   const insertIndex = insertAfterProp === null
     ? node.start.offset + 1
     : insertAfterProp.end.offset + 1;
-
+  const content = JSON.stringify(value, null, indent).replace(/\n/g, indentStr);
   recorder.insertRight(
     insertIndex,
     indentStr
-    + `"${propertyName}": ${JSON.stringify(value, null, 2).replace(/\n/g, indentStr)}`
+    + `"${propertyName}":${indent ? ' ' : ''}${content}`
     + ',',
   );
 }
@@ -98,18 +101,20 @@ export function appendValueInAstArray(
   indent = 4,
 ) {
   const indentStr = _buildIndent(indent);
-
+  let index = node.start.offset + 1;
   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.insertRight(last.end.offset, ',');
+    index = indent ? last.end.offset + 1 : last.end.offset;
   }
 
-  recorder.insertLeft(
-    node.end.offset - 1,
-    '  '
-    + JSON.stringify(value, null, 2).replace(/\n/g, indentStr)
-    + indentStr.slice(0, -2),
+  recorder.insertRight(
+    index,
+    (node.elements.length === 0 && indent ? '\n' : '')
+    + ' '.repeat(indent)
+    + JSON.stringify(value, null, indent).replace(/\n/g, indentStr)
+    + indentStr.slice(0, -indent),
   );
 }
 
@@ -129,5 +134,5 @@ export function findPropertyInAstObject(
 }
 
 function _buildIndent(count: number): string {
-  return '\n' + new Array(count + 1).join(' ');
+  return count ? '\n' + ' '.repeat(count) : '';
 }

packages/schematics/angular/utility/json-utils_spec.ts

@@ -5,77 +5,157 @@
  * 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 { parseJsonAst } from '@angular-devkit/core';
-import { HostTree } from '@angular-devkit/schematics';
+import { JsonAstArray, JsonAstObject, JsonValue, parseJsonAst } from '@angular-devkit/core';
+import { HostTree, UpdateRecorder } from '@angular-devkit/schematics';
 import { UnitTestTree } from '@angular-devkit/schematics/testing';
-import { insertPropertyInAstObjectInOrder } from './json-utils';
-
-type Pojso = {
-  [key: string]: string;
-};
+import {
+  appendPropertyInAstObject,
+  appendValueInAstArray,
+  insertPropertyInAstObjectInOrder,
+} from './json-utils';
 
 describe('json-utils', () => {
+  const a = 'a';
+  const b = 'b';
+  const m = 'm';
+  const z = 'z';
   const filePath = '/temp';
   let tree: UnitTestTree;
+
+  function runTest(testFn: Function, obj: JsonValue, indent: number): string {
+    const content = JSON.stringify(obj, null, indent);
+    tree.create(filePath, content);
+    const ast = parseJsonAst(content);
+    const rec = tree.beginUpdate(filePath);
+    testFn(rec, ast);
+    tree.commitUpdate(rec);
+
+    const result = tree.readContent(filePath);
+    // Clean up the tree by deleting the file.
+    tree.delete(filePath);
+
+    return result;
+  }
+
   beforeEach(() => {
     tree = new UnitTestTree(new HostTree());
   });
 
-  describe('insertPropertyInAstObjectInOrder', () => {
-    function runTest(obj: Pojso, prop: string, val: string): Pojso {
-      const content = JSON.stringify(obj, null, 2);
-      tree.create(filePath, content);
-      const ast = parseJsonAst(content);
-      const rec = tree.beginUpdate(filePath);
-      if (ast.kind === 'object') {
-        insertPropertyInAstObjectInOrder(rec, ast, prop, val, 2);
+  describe('appendPropertyInAstObject', () => {
+    it('should insert multiple props with different indentations', () => {
+      const cases: Array<[{}, string, {}, number]> = [
+        // initial | prop | want | indent
+        [{}, z, {z}, 0],
+        [{z}, m, {z, m}, 0],
+        [{m, z}, a, {m, z, a}, 0],
+        [{a, m, z}, b, {a, m, z, b}, 0],
+        [{}, z, {z}, 2],
+        [{z}, m, {z, m}, 2],
+        [{m, z}, a, {m, z, a}, 2],
+        [{a, m, z}, b, {a, m, z, b}, 2],
+        [{}, z, {z}, 4],
+        [{z}, m, {z, m}, 4],
+        [{m, z}, a, {m, z, a}, 4],
+        [{a, m, z}, b, {a, m, z, b}, 4],
+      ];
+      for (const c of cases) {
+        const [initial, prop, want, indent] = c;
+        const got = runTest((rec: UpdateRecorder, ast: JsonAstObject) => {
+          expect(ast.kind).toBe('object');
+          appendPropertyInAstObject(rec, ast, prop, prop, indent);
+        }, initial, indent);
+        expect(got).toBe(JSON.stringify(want, null, indent));
+        expect(JSON.parse(got)).toEqual(want);
       }
-      tree.commitUpdate(rec);
-
-      const result = JSON.parse(tree.readContent(filePath));
-      // Clean up the tree by deleting the file.
-      tree.delete(filePath);
-
-      return result;
-    }
+    });
+  });
 
+  describe('insertPropertyInAstObjectInOrder', () => {
     it('should insert a first prop', () => {
-      const obj = {
-        m: 'm',
-        z: 'z',
-      };
-      const result = runTest(obj, 'a', 'val');
-      expect(Object.keys(result)).toEqual(['a', 'm', 'z']);
+      const indent = 2;
+      const result = runTest((rec: UpdateRecorder, ast: JsonAstObject) => {
+        expect(ast.kind).toBe('object');
+        insertPropertyInAstObjectInOrder(rec, ast, a, a, indent);
+      }, {m, z}, indent);
+      expect(result).toBe(JSON.stringify({a, m, z}, null, indent));
+      expect(JSON.parse(result)).toEqual({a, m, z});
     });
 
     it('should insert a middle prop', () => {
-      const obj = {
-        a: 'a',
-        z: 'z',
-      };
-      const result = runTest(obj, 'm', 'val');
-      expect(Object.keys(result)).toEqual(['a', 'm', 'z']);
+      const indent = 2;
+      const result = runTest((rec: UpdateRecorder, ast: JsonAstObject) => {
+        expect(ast.kind).toBe('object');
+        insertPropertyInAstObjectInOrder(rec, ast, m, m, indent);
+      }, {a, z}, indent);
+      expect(result).toBe(JSON.stringify({a, m, z}, null, indent));
+      expect(JSON.parse(result)).toEqual({a, m, z});
     });
 
     it('should insert a last prop', () => {
-      const obj = {
-        a: 'a',
-        m: 'm',
-      };
-      const result = runTest(obj, 'z', 'val');
-      expect(Object.keys(result)).toEqual(['a', 'm', 'z']);
+      const indent = 2;
+      const result = runTest((rec: UpdateRecorder, ast: JsonAstObject) => {
+        expect(ast.kind).toBe('object');
+        insertPropertyInAstObjectInOrder(rec, ast, z, z, indent);
+      }, {a, m}, indent);
+      expect(result).toBe(JSON.stringify({a, m, z}, null, indent));
+      expect(JSON.parse(result)).toEqual({a, m, z});
+    });
+
+    it('should insert multiple props with different indentations', () => {
+      const cases: Array<[{}, string, {}, number]> = [
+        // initial | prop | want | indent
+        [{}, z, {z}, 0],
+        [{z}, m, {m, z}, 0],
+        [{m, z}, a, {a, m, z}, 0],
+        [{a, m, z}, b, {a, b, m, z}, 0],
+        [{}, z, {z}, 2],
+        [{z}, m, {m, z}, 2],
+        [{m, z}, a, {a, m, z}, 2],
+        [{a, m, z}, b, {a, b, m, z}, 2],
+        [{}, z, {z}, 4],
+        [{z}, m, {m, z}, 4],
+        [{m, z}, a, {a, m, z}, 4],
+        [{a, m, z}, b, {a, b, m, z}, 4],
+      ];
+      for (const c of cases) {
+        const [initial, prop, want, indent] = c;
+        const got = runTest((rec: UpdateRecorder, ast: JsonAstObject) => {
+          expect(ast.kind).toBe('object');
+          insertPropertyInAstObjectInOrder(rec, ast, prop, prop, indent);
+        }, initial, indent);
+        expect(got).toBe(JSON.stringify(want, null, indent));
+        expect(JSON.parse(got)).toEqual(want);
+      }
     });
+  });
 
-    it('should insert multiple props', () => {
-      let obj = {};
-      obj = runTest(obj, 'z', 'val');
-      expect(Object.keys(obj)).toEqual(['z']);
-      obj = runTest(obj, 'm', 'val');
-      expect(Object.keys(obj)).toEqual(['m', 'z']);
-      obj = runTest(obj, 'a', 'val');
-      expect(Object.keys(obj)).toEqual(['a', 'm', 'z']);
-      obj = runTest(obj, 'b', 'val');
-      expect(Object.keys(obj)).toEqual(['a', 'b', 'm', 'z']);
+  describe('appendValueInAstArray', () => {
+    it('should insert multiple props with different indentations', () => {
+      const cases: Array<[string[], string, {}, number]> = [
+        // initial | value | want | indent
+        [[], z, [z], 0],
+        [[z], m, [z, m], 0],
+        [[m, z], a, [m, z, a], 0],
+        [[a, m, z], b, [a, m, z, b], 0],
+        [[], z, [z], 2],
+        [[z], m, [z, m], 2],
+        [[m, z], a, [m, z, a], 2],
+        [[a, m, z], b, [a, m, z, b], 2],
+        [[], z, [z], 4],
+        [[z], m, [z, m], 4],
+        [[m, z], a, [m, z, a], 4],
+        [[a, m, z], b, [a, m, z, b], 4],
+      ];
+      for (const c of cases) {
+        const [initial, value, want, indent] = c;
+        const got = runTest((rec: UpdateRecorder, ast: JsonAstArray) => {
+          expect(ast.kind).toBe('array');
+          appendValueInAstArray(rec, ast, value, indent);
+        }, initial, indent);
+        expect(got).toBe(JSON.stringify(want, null, indent));
+        expect(JSON.parse(got)).toEqual(want);
+      }
     });
   });
+
 });