Skip to content

Commit

Permalink
fix(@angular/cli): generate command now ignores duplicate component s…
Browse files Browse the repository at this point in the history 
…ymbol (#4446)

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

Closes #4323
  • Loading branch information
@delasteve @hansl
delasteve authored and hansl committed Feb 17, 2017
1 parent 9d29cbc commit 9c25f74
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 27 deletions.
31 changes: 21 additions & 10 deletions packages/@angular/cli/blueprints/component/index.ts
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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`;
Expand Down Expand Up @@ -119,7 +119,7 @@ export default Blueprint.extend({
};
},

files: function() {
files: function () {
let fileList = getFiles.call(this) as Array<string>;

if (this.options && this.options.inlineTemplate) {
Expand Down Expand Up @@ -154,7 +154,7 @@ export default Blueprint.extend({
};
},

afterInstall: function(options: any) {
afterInstall: function (options: any) {
if (options.dryRun) {
return;
}
Expand All @@ -166,6 +166,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))
Expand All @@ -175,10 +177,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);
Expand Down
23 changes: 23 additions & 0 deletions packages/@angular/cli/lib/ast-tools/ast-utils.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
37 changes: 21 additions & 16 deletions packages/@angular/cli/lib/ast-tools/ast-utils.ts
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import * as ts from 'typescript';
import * as fs from 'fs';
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 { 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 'rxjs/add/observable/empty';
import 'rxjs/add/observable/of';
import 'rxjs/add/operator/do';
Expand Down Expand Up @@ -162,17 +161,17 @@ 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;
return id.getFullText(source) == identifier
&& angularImports[id.getFullText(source)] === 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;
Expand All @@ -186,7 +185,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);
}


Expand Down Expand Up @@ -229,14 +228,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;
Expand All @@ -245,11 +244,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];
}

Expand All @@ -258,7 +263,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`;
Expand Down Expand Up @@ -326,7 +331,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);
}

12 changes: 11 additions & 1 deletion tests/acceptance/generate-component.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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);
Expand Down
24 changes: 24 additions & 0 deletions tests/e2e/tests/generate/component/component-duplicate.ts
View file
Original file line number Diff line number Diff line change
@@ -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}.`);
}
});
}

0 comments on commit 9c25f74

Please sign in to comment.