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.

fixes 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`;
@@ -54,13 +54,13 @@ export default Blueprint.extend({
 
     let defaultPrefix = '';
     if (this.project.ngConfig &&
-        this.project.ngConfig.apps[0] &&
-        this.project.ngConfig.apps[0].prefix) {
+      this.project.ngConfig.apps[0] &&
+      this.project.ngConfig.apps[0].prefix) {
       defaultPrefix = this.project.ngConfig.apps[0].prefix;
     }
 
     let prefix = (this.options.prefix === 'false' || this.options.prefix === '')
-                 ? '' : (this.options.prefix || defaultPrefix);
+      ? '' : (this.options.prefix || defaultPrefix);
     prefix = prefix && `${prefix}-`;
 
     this.selector = stringUtils.dasherize(prefix + parsedPath.name);
@@ -75,8 +75,8 @@ export default Blueprint.extend({
   locals: function (options: any) {
     this.styleExt = 'css';
     if (this.project.ngConfig &&
-        this.project.ngConfig.defaults &&
-        this.project.ngConfig.defaults.styleExt) {
+      this.project.ngConfig.defaults &&
+      this.project.ngConfig.defaults.styleExt) {
       this.styleExt = this.project.ngConfig.defaults.styleExt;
     }
 
@@ -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

@@ -204,6 +204,14 @@ function _addSymbolToNgModuleMetadata(ngModulePath: string, metadataField: strin
         console.log('No app module found. Please add your new class to your component.');
         return new NoopChange();
       }
+
+      let shouldInsertChange = true;
+      if (Array.isArray(node)) {
+        const nodeArray = node as any as Array<ts.Node>;
+        const symbolsArray = nodeArray.map(node => node.getText());
+        shouldInsertChange = !(symbolsArray.indexOf(symbolName) > -1);
+      }
+
       if (Array.isArray(node)) {
         node = node[node.length - 1];
       }
@@ -242,10 +250,16 @@ function _addSymbolToNgModuleMetadata(ngModulePath: string, metadataField: strin
         }
       }
 
-      const insert = new InsertChange(ngModulePath, position, toInsert);
       const importInsert: Change = insertImport(
         ngModulePath, symbolName.replace(/\..*$/, ''), importPath);
-      return new MultiChange([insert, importInsert]);
+      const changes = [importInsert];
+
+      if (shouldInsertChange) {
+        const insert = new InsertChange(ngModulePath, position, toInsert);
+        changes.push(insert);
+      }
+
+      return new MultiChange(changes);
     });
 }
 

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}.`);
+      }
+    });
+}