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

delasteve · delasteve · commit 72af96b0a126 · 2017-02-06T17:40:06.000-05:00
"ng g c X" will no longer insert the component symbol in the NgModule's declaration array should it already exist. Closes angular#4323
diff --git a/packages/@angular/cli/blueprints/component/index.ts b/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);
diff --git a/packages/@angular/cli/lib/ast-tools/ast-utils.spec.ts b/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))
diff --git a/packages/@angular/cli/lib/ast-tools/ast-utils.ts b/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';
@@ -81,7 +81,7 @@ function nodesByPosition(first: ts.Node, second: ts.Node): number {
  * @throw Error if toInsert is first occurence but fall back is not set
  */
 export function insertAfterLastOccurrence(nodes: ts.Node[], toInsert: string,
-    file: string, fallbackPos?: number, syntaxKind?: ts.SyntaxKind): Change {
+  file: string, fallbackPos?: number, syntaxKind?: ts.SyntaxKind): Change {
   let lastItem = nodes.sort(nodesByPosition).pop();
   if (syntaxKind) {
     lastItem = findNodes(lastItem, syntaxKind).sort(nodesByPosition).pop();
@@ -106,13 +106,13 @@ export function getContentOfKeyLiteral(source: ts.SourceFile, node: ts.Node): st
 
 
 export function getDecoratorMetadata(source: ts.SourceFile, identifier: string,
-                                     module: string): Observable<ts.Node> {
+  module: string): Observable<ts.Node> {
   const symbols = new Symbols(source as any);
 
   return getSourceNodes(source)
     .filter(node => {
       return node.kind == ts.SyntaxKind.Decorator
-          && (<ts.Decorator>node).expression.kind == ts.SyntaxKind.CallExpression;
+        && (<ts.Decorator>node).expression.kind == ts.SyntaxKind.CallExpression;
     })
     .map(node => <ts.CallExpression>(<ts.Decorator>node).expression)
     .filter(expr => {
@@ -140,13 +140,13 @@ export function getDecoratorMetadata(source: ts.SourceFile, identifier: string,
       return false;
     })
     .filter(expr => expr.arguments[0]
-                 && expr.arguments[0].kind == ts.SyntaxKind.ObjectLiteralExpression)
+      && expr.arguments[0].kind == ts.SyntaxKind.ObjectLiteralExpression)
     .map(expr => <ts.ObjectLiteralExpression>expr.arguments[0]);
 }
 
 
 function _addSymbolToNgModuleMetadata(ngModulePath: string, metadataField: string,
-                                      symbolName: string, importPath: string) {
+  symbolName: string, importPath: string) {
   const source: ts.SourceFile = getSource(ngModulePath);
   let metadata = getDecoratorMetadata(source, 'NgModule', '@angular/core');
 
@@ -200,10 +200,18 @@ 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();
+        }
+      }
+
       if (Array.isArray(node)) {
         node = node[node.length - 1];
       }
@@ -213,7 +221,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`;
@@ -254,7 +262,7 @@ function _addSymbolToNgModuleMetadata(ngModulePath: string, metadataField: strin
 * into NgModule declarations. It also imports the component.
 */
 export function addDeclarationToModule(modulePath: string, classifiedName: string,
-                                       importPath: string): Promise<Change> {
+  importPath: string): Promise<Change> {
 
   return _addSymbolToNgModuleMetadata(modulePath, 'declarations', classifiedName, importPath);
 }
@@ -264,7 +272,7 @@ export function addDeclarationToModule(modulePath: string, classifiedName: strin
  * into NgModule declarations. It also imports the component.
  */
 export function addImportToModule(modulePath: string, classifiedName: string,
-                                  importPath: string): Promise<Change> {
+  importPath: string): Promise<Change> {
 
   return _addSymbolToNgModuleMetadata(modulePath, 'imports', classifiedName, importPath);
 }
@@ -273,15 +281,15 @@ export function addImportToModule(modulePath: string, classifiedName: string,
  * Custom function to insert a provider into NgModule. It also imports it.
  */
 export function addProviderToModule(modulePath: string, classifiedName: string,
-                                    importPath: string): Promise<Change> {
+  importPath: string): Promise<Change> {
   return _addSymbolToNgModuleMetadata(modulePath, 'providers', classifiedName, importPath);
 }
 
 /**
  * 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);
 }
 
diff --git a/packages/@angular/cli/tsconfig.json b/packages/@angular/cli/tsconfig.json
@@ -13,7 +13,9 @@
     "sourceMap": true,
     "sourceRoot": "/",
     "target": "es5",
-    "lib": ["es6"],
+    "lib": [
+      "es2016"
+    ],
     "skipLibCheck": true,
     "typeRoots": [
       "../../../node_modules/@types"
diff --git a/tests/acceptance/generate-component.spec.js b/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);
diff --git a/tests/e2e/tests/generate/component/component-duplicate.ts b/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}.`);
+      }
+    });
+}
