fix(generate): ignore duplicate component symbol in module declarations

"ng g c X" will no longer insert the component symbol in the NgModule's declaration array should it already exist.

Closes angular#4323

Author: delasteve

packages/@angular/cli/blueprints/component/index.ts

@@ -1,8 +1,8 @@
-import {NodeHost} from '../../lib/ast-tools';
+import { NodeHost } from '../../lib/ast-tools';
 
-const path = require('path');
-const fs = require('fs');
-const chalk = require('chalk');
+import * as fs from 'fs';
+import * as path from 'path';
+import * as chalk from 'chalk';
 const Blueprint = require('../../ember-cli/lib/models/blueprint');
 const dynamicPathParser = require('../../utilities/dynamic-path-parser');
 const findParentModule = require('../../utilities/find-parent-module').default;
@@ -26,7 +26,7 @@ export default Blueprint.extend({
     { name: 'export', type: Boolean, default: false }
   ],
 
-  beforeInstall: function(options: any) {
+  beforeInstall: function (options: any) {
     if (options.module) {
       // Resolve path to module
       const modulePath = options.module.endsWith('.ts') ? options.module : `${options.module}.ts`;
@@ -115,7 +115,7 @@ export default Blueprint.extend({
     };
   },
 
-  files: function() {
+  files: function () {
     let fileList = getFiles.call(this) as Array<string>;
 
     if (this.options && this.options.inlineTemplate) {
@@ -150,7 +150,7 @@ export default Blueprint.extend({
     };
   },
 
-  afterInstall: function(options: any) {
+  afterInstall: function (options: any) {
     if (options.dryRun) {
       return;
     }
@@ -162,6 +162,8 @@ export default Blueprint.extend({
     const importPath = componentDir ? `./${componentDir}/${fileName}` : `./${fileName}`;
 
     if (!options.skipImport) {
+      const preChange = fs.readFileSync(this.pathToModule, 'utf8');
+
       returns.push(
         astUtils.addDeclarationToModule(this.pathToModule, className, importPath)
           .then((change: any) => change.apply(NodeHost))
@@ -171,10 +173,19 @@ export default Blueprint.extend({
                 .then((change: any) => change.apply(NodeHost));
             }
             return result;
+          })
+          .then(() => {
+            const postChange = fs.readFileSync(this.pathToModule, 'utf8');
+            let moduleStatus = 'update';
+
+            if (postChange === preChange) {
+              moduleStatus = 'identical';
+            }
+
+            this._writeStatusToUI(chalk.yellow,
+              moduleStatus,
+              path.relative(this.project.root, this.pathToModule));
           }));
-      this._writeStatusToUI(chalk.yellow,
-                            'update',
-                            path.relative(this.project.root, this.pathToModule));
     }
 
     return Promise.all(returns);

packages/@angular/cli/lib/ast-tools/ast-utils.spec.ts

@@ -227,6 +227,29 @@ class Module {}`
       });
   });
 
+  it('does not append duplicate declarations', () => {
+    return addDeclarationToModule('2.ts', 'MyClass', 'MyImportPath')
+      .then(change => change.apply(NodeHost))
+      .then(() => addDeclarationToModule('2.ts', 'MyClass', 'MyImportPath'))
+      .then(change => change.apply(NodeHost))
+      .then(() => readFile('2.ts', 'utf-8'))
+      .then(content => {
+        expect(content).toEqual(
+          '\n' +
+          'import {NgModule} from \'@angular/core\';\n' +
+          'import { MyClass } from \'MyImportPath\';\n' +
+          '\n' +
+          '@NgModule({\n' +
+          '  declarations: [\n' +
+          '    Other,\n' +
+          '    MyClass\n' +
+          '  ]\n' +
+          '})\n' +
+          'class Module {}'
+        );
+      });
+  });
+
   it('works with array with declarations', () => {
     return addDeclarationToModule('2.ts', 'MyClass', 'MyImportPath')
       .then(change => change.apply(NodeHost))

packages/@angular/cli/lib/ast-tools/ast-utils.ts

@@ -1,16 +1,16 @@
 import * as ts from 'typescript';
 import * as fs from 'fs';
-import {Symbols} from '@angular/tsc-wrapped/src/symbols';
+import { Symbols } from '@angular/tsc-wrapped/src/symbols';
 import {
   isMetadataImportedSymbolReferenceExpression,
   isMetadataModuleReferenceExpression
 } from '@angular/tsc-wrapped';
-import {Change, InsertChange, NoopChange, MultiChange} from './change';
-import {findNodes} from './node';
-import {insertImport} from './route-utils';
+import { Change, InsertChange, NoopChange, MultiChange } from './change';
+import { findNodes } from './node';
+import { insertImport } from './route-utils';
 
-import {Observable} from 'rxjs/Observable';
-import {ReplaySubject} from 'rxjs/ReplaySubject';
+import { Observable } from 'rxjs/Observable';
+import { ReplaySubject } from 'rxjs/ReplaySubject';
 import 'rxjs/add/observable/empty';
 import 'rxjs/add/observable/of';
 import 'rxjs/add/operator/do';
@@ -112,26 +112,26 @@ export function getDecoratorMetadata(source: ts.SourceFile, identifier: string,
   return getSourceNodes(source)
     .filter(node => {
       return node.kind == ts.SyntaxKind.Decorator
-          && (<ts.Decorator>node).expression.kind == ts.SyntaxKind.CallExpression;
+          && (node as ts.Decorator).expression.kind == ts.SyntaxKind.CallExpression;
     })
-    .map(node => <ts.CallExpression>(<ts.Decorator>node).expression)
+    .map(node => (node as ts.Decorator).expression as ts.CallExpression)
     .filter(expr => {
       if (expr.expression.kind == ts.SyntaxKind.Identifier) {
-        const id = <ts.Identifier>expr.expression;
+        const id = expr.expression as ts.Identifier;
         const metaData = symbols.resolve(id.getFullText(source));
         if (isMetadataImportedSymbolReferenceExpression(metaData)) {
           return metaData.name == identifier && metaData.module == module;
         }
       } else if (expr.expression.kind == ts.SyntaxKind.PropertyAccessExpression) {
         // This covers foo.NgModule when importing * as foo.
-        const paExpr = <ts.PropertyAccessExpression>expr.expression;
+        const paExpr = expr.expression as ts.PropertyAccessExpression;
         // If the left expression is not an identifier, just give up at that point.
         if (paExpr.expression.kind !== ts.SyntaxKind.Identifier) {
           return false;
         }
 
         const id = paExpr.name;
-        const moduleId = <ts.Identifier>paExpr.expression;
+        const moduleId = paExpr.expression as ts.Identifier;
         const moduleMetaData = symbols.resolve(moduleId.getFullText(source));
         if (isMetadataModuleReferenceExpression(moduleMetaData)) {
           return moduleMetaData.module == module && id.getFullText(source) == identifier;
@@ -141,7 +141,7 @@ export function getDecoratorMetadata(source: ts.SourceFile, identifier: string,
     })
     .filter(expr => expr.arguments[0]
                  && expr.arguments[0].kind == ts.SyntaxKind.ObjectLiteralExpression)
-    .map(expr => <ts.ObjectLiteralExpression>expr.arguments[0]);
+    .map(expr => expr.arguments[0] as ts.ObjectLiteralExpression);
 }
 
 
@@ -184,14 +184,14 @@ function _addSymbolToNgModuleMetadata(ngModulePath: string, metadataField: strin
         return metadata.toPromise();
       }
 
-      const assignment = <ts.PropertyAssignment>matchingProperties[0];
+      const assignment = matchingProperties[0] as ts.PropertyAssignment;
 
       // If it's not an array, nothing we can do really.
       if (assignment.initializer.kind !== ts.SyntaxKind.ArrayLiteralExpression) {
         return null;
       }
 
-      const arrLiteral = <ts.ArrayLiteralExpression>assignment.initializer;
+      const arrLiteral = assignment.initializer as ts.ArrayLiteralExpression;
       if (arrLiteral.elements.length == 0) {
         // Forward the property.
         return arrLiteral;
@@ -200,11 +200,17 @@ function _addSymbolToNgModuleMetadata(ngModulePath: string, metadataField: strin
     })
     .then((node: ts.Node) => {
       if (!node) {
-        /* eslint-disable no-console */
         console.log('No app module found. Please add your new class to your component.');
         return new NoopChange();
       }
+
       if (Array.isArray(node)) {
+        const nodeArray = node as any as Array<ts.Node>;
+        const symbolsArray = nodeArray.map(node => node.getText());
+        if (symbolsArray.includes(symbolName)) {
+          return new NoopChange();
+        }
+
         node = node[node.length - 1];
       }
 
@@ -213,7 +219,7 @@ function _addSymbolToNgModuleMetadata(ngModulePath: string, metadataField: strin
       if (node.kind == ts.SyntaxKind.ObjectLiteralExpression) {
         // We haven't found the field in the metadata declaration. Insert a new
         // field.
-        let expr = <ts.ObjectLiteralExpression>node;
+        let expr = node as ts.ObjectLiteralExpression;
         if (expr.properties.length == 0) {
           position = expr.getEnd() - 1;
           toInsert = `  ${metadataField}: [${symbolName}]\n`;
@@ -281,7 +287,7 @@ export function addProviderToModule(modulePath: string, classifiedName: string,
  * Custom function to insert an export into NgModule. It also imports it.
  */
 export function addExportToModule(modulePath: string, classifiedName: string,
-                                    importPath: string): Promise<Change> {
+                                  importPath: string): Promise<Change> {
   return _addSymbolToNgModuleMetadata(modulePath, 'exports', classifiedName, importPath);
 }
 

tests/acceptance/generate-component.spec.js

@@ -42,6 +42,16 @@ describe('Acceptance: ng generate component', function () {
       });
   });
 
+  it('generating my-comp twice does not add two declarations to module', function () {
+    const appModule = path.join(root, 'tmp/foo/src/app/app.module.ts');
+    return ng(['generate', 'component', 'my-comp'])
+      .then(() => ng(['generate', 'component', 'my-comp']))
+      .then(() => readFile(appModule, 'utf-8'))
+      .then(content => {
+        expect(content).matches(/declarations:\s+\[\r?\n\s+AppComponent,\r?\n\s+MyCompComponent\r?\n\s+\]/m);
+      });
+  });
+
   it('test' + path.sep + 'my-comp', function () {
     fs.mkdirsSync(path.join(root, 'tmp', 'foo', 'src', 'app', 'test'));
     return ng(['generate', 'component', 'test' + path.sep + 'my-comp']).then(() => {
@@ -206,7 +216,7 @@ describe('Acceptance: ng generate component', function () {
     });
   });
 
-  it('my-comp --no-spec', function() {
+  it('my-comp --no-spec', function () {
     return ng(['generate', 'component', 'my-comp', '--no-spec']).then(() => {
       var testPath = path.join(root, 'tmp', 'foo', 'src', 'app', 'my-comp', 'my-comp.component.spec.ts');
       expect(existsSync(testPath)).to.equal(false);

tests/e2e/tests/generate/component/component-duplicate.ts

@@ -0,0 +1,24 @@
+import * as path from 'path';
+import { ng } from '../../../utils/process';
+import { oneLine } from 'common-tags';
+
+export default function () {
+  return ng('generate', 'component', 'test-component')
+    .then((output) => {
+      if (!output.match(/update src[\\|\/]app[\\|\/]app.module.ts/)) {
+        throw new Error(oneLine`
+          Expected to match
+          "update src${path.sep}app${path.sep}app.module.ts"
+          in ${output}.`);
+      }
+    })
+    .then(() => ng('generate', 'component', 'test-component'))
+    .then((output) => {
+      if (!output.match(/identical src[\\|\/]app[\\|\/]app.module.ts/)) {
+        throw new Error(oneLine`
+          Expected to match
+          "identical src${path.sep}app${path.sep}app.module.ts"
+          in ${output}.`);
+      }
+    });
+}