Skip to content

Commit

Permalink
fix(@schematics/angular): provide actionable error message when routi…
Browse files Browse the repository at this point in the history
…ng declaration cannot be found

Due to incorrect castings previously the code would crash when the module doesn't contain an routing module with the following error:

```
Cannot read property 'properties' of undefined
```

Closes #21397

(cherry picked from commit 7db433b)
  • Loading branch information
alan-agius4 authored and clydin committed Apr 4, 2022
1 parent c97c8e7 commit bbe74b8
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 40 deletions.
63 changes: 34 additions & 29 deletions packages/schematics/angular/utility/ast-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ export function getSourceNodes(sourceFile: ts.SourceFile): ts.Node[] {

export function findNode(node: ts.Node, kind: ts.SyntaxKind, text: string): ts.Node | null {
if (node.kind === kind && node.getText() === text) {
// throw new Error(node.getText());
return node;
}

Expand Down Expand Up @@ -367,29 +366,28 @@ export function addSymbolToNgModuleMetadata(
importPath: string | null = null,
): Change[] {
const nodes = getDecoratorMetadata(source, 'NgModule', '@angular/core');
let node: any = nodes[0]; // eslint-disable-line @typescript-eslint/no-explicit-any
const node = nodes[0];

// Find the decorator declaration.
if (!node) {
if (!node || !ts.isObjectLiteralExpression(node)) {
return [];
}

// Get all the children property assignment of object literals.
const matchingProperties = getMetadataField(node as ts.ObjectLiteralExpression, metadataField);
const matchingProperties = getMetadataField(node, metadataField);

if (matchingProperties.length == 0) {
// We haven't found the field in the metadata declaration. Insert a new field.
const expr = node as ts.ObjectLiteralExpression;
let position: number;
let toInsert: string;
if (expr.properties.length == 0) {
position = expr.getEnd() - 1;
if (node.properties.length == 0) {
position = node.getEnd() - 1;
toInsert = `\n ${metadataField}: [\n${tags.indentBy(4)`${symbolName}`}\n ]\n`;
} else {
node = expr.properties[expr.properties.length - 1];
position = node.getEnd();
const childNode = node.properties[node.properties.length - 1];
position = childNode.getEnd();
// Get the indentation of the last element, if any.
const text = node.getFullText(source);
const text = childNode.getFullText(source);
const matches = text.match(/^(\r?\n)(\s*)/);
if (matches) {
toInsert =
Expand All @@ -408,47 +406,48 @@ export function addSymbolToNgModuleMetadata(
return [new InsertChange(ngModulePath, position, toInsert)];
}
}
const assignment = matchingProperties[0] as ts.PropertyAssignment;
const assignment = matchingProperties[0];

// If it's not an array, nothing we can do really.
if (assignment.initializer.kind !== ts.SyntaxKind.ArrayLiteralExpression) {
if (
!ts.isPropertyAssignment(assignment) ||
!ts.isArrayLiteralExpression(assignment.initializer)
) {
return [];
}

const arrLiteral = assignment.initializer as ts.ArrayLiteralExpression;
if (arrLiteral.elements.length == 0) {
// Forward the property.
node = arrLiteral;
} else {
node = arrLiteral.elements;
}
let expresssion: ts.Expression | ts.ArrayLiteralExpression;
const assignmentInit = assignment.initializer;
const elements = assignmentInit.elements;

if (Array.isArray(node)) {
const nodeArray = (node as {}) as Array<ts.Node>;
const symbolsArray = nodeArray.map((node) => tags.oneLine`${node.getText()}`);
if (elements.length) {
const symbolsArray = elements.map((node) => tags.oneLine`${node.getText()}`);
if (symbolsArray.includes(tags.oneLine`${symbolName}`)) {
return [];
}

node = node[node.length - 1];
expresssion = elements[elements.length - 1];
} else {
expresssion = assignmentInit;
}

let toInsert: string;
let position = node.getEnd();
if (node.kind == ts.SyntaxKind.ArrayLiteralExpression) {
let position = expresssion.getEnd();
if (ts.isArrayLiteralExpression(expresssion)) {
// We found the field but it's empty. Insert it just before the `]`.
position--;
toInsert = `\n${tags.indentBy(4)`${symbolName}`}\n `;
} else {
// Get the indentation of the last element, if any.
const text = node.getFullText(source);
const text = expresssion.getFullText(source);
const matches = text.match(/^(\r?\n)(\s*)/);
if (matches) {
toInsert = `,${matches[1]}${tags.indentBy(matches[2].length)`${symbolName}`}`;
} else {
toInsert = `, ${symbolName}`;
}
}

if (importPath !== null) {
return [
new InsertChange(ngModulePath, position, toInsert),
Expand Down Expand Up @@ -604,9 +603,12 @@ export function getEnvironmentExportName(source: ts.SourceFile): string | null {
*/
export function getRouterModuleDeclaration(source: ts.SourceFile): ts.Expression | undefined {
const result = getDecoratorMetadata(source, 'NgModule', '@angular/core');
const node = result[0] as ts.ObjectLiteralExpression;
const matchingProperties = getMetadataField(node, 'imports');
const node = result[0];
if (!node || !ts.isObjectLiteralExpression(node)) {
return undefined;
}

const matchingProperties = getMetadataField(node, 'imports');
if (!matchingProperties) {
return;
}
Expand Down Expand Up @@ -634,7 +636,10 @@ export function addRouteDeclarationToModule(
): Change {
const routerModuleExpr = getRouterModuleDeclaration(source);
if (!routerModuleExpr) {
throw new Error(`Couldn't find a route declaration in ${fileToAdd}.`);
throw new Error(
`Couldn't find a route declaration in ${fileToAdd}.\n` +
`Use the '--module' option to specify a different routing module.`,
);
}
const scopeConfigMethodArgs = (routerModuleExpr as ts.CallExpression).arguments;
if (!scopeConfigMethodArgs.length) {
Expand Down
17 changes: 14 additions & 3 deletions packages/schematics/angular/utility/ast-utils_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ describe('ast utils', () => {
const elements = (arrayNode.pop() as ts.ArrayLiteralExpression).elements;

const change = insertAfterLastOccurrence(
(elements as unknown) as ts.Node[],
elements as unknown as ts.Node[],
`, 'bar'`,
filePath,
elements.pos,
Expand All @@ -281,7 +281,7 @@ describe('ast utils', () => {
const elements = (arrayNode.pop() as ts.ArrayLiteralExpression).elements;

const change = insertAfterLastOccurrence(
(elements as unknown) as ts.Node[],
elements as unknown as ts.Node[],
`'bar'`,
filePath,
elements.pos,
Expand Down Expand Up @@ -312,7 +312,7 @@ describe('ast utils', () => {

const source = getTsSource(modulePath, moduleContent);
const change = () => addRouteDeclarationToModule(source, './src/app', '');
expect(change).toThrowError(`Couldn't find a route declaration in ./src/app.`);
expect(change).toThrowError(/Couldn't find a route declaration in \.\/src\/app/);
});

it(`should throw an error when router module doesn't have arguments`, () => {
Expand Down Expand Up @@ -632,6 +632,17 @@ describe('ast utils', () => {
/RouterModule\.forRoot\(\[\r?\n?\s*{ path: 'foo', component: FooComponent },\r?\n?\s*{ path: 'bar', component: BarComponent }\r?\n?\s*\]\)/,
);
});

it('should error if sourcefile is empty', () => {
const change = () =>
addRouteDeclarationToModule(
getTsSource(modulePath, ''),
'./src/app',
`{ path: 'foo', component: FooComponent }`,
);

expect(change).toThrowError(/Couldn't find a route declaration in \.\/src\/app/);
});
});

describe('findNodes', () => {
Expand Down
14 changes: 6 additions & 8 deletions packages/schematics/angular/utility/find-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,9 @@ export function findModuleFromOptions(host: Tree, options: ModuleOptions): Path

const candidatesDirs = [...candidateSet].sort((a, b) => b.length - a.length);
for (const c of candidatesDirs) {
const candidateFiles = [
'',
`${moduleBaseName}.ts`,
`${moduleBaseName}${moduleExt}`,
].map((x) => join(c, x));
const candidateFiles = ['', `${moduleBaseName}.ts`, `${moduleBaseName}${moduleExt}`].map(
(x) => join(c, x),
);

for (const sc of candidateFiles) {
if (host.exists(sc)) {
Expand Down Expand Up @@ -96,7 +94,7 @@ export function findModule(
return join(dir.path, filteredMatches[0]);
} else if (filteredMatches.length > 1) {
throw new Error(
'More than one module matches. Use the skip-import option to skip importing ' +
`More than one module matches. Use the '--skip-import' option to skip importing ` +
'the component into the closest module or use the module option to specify a module.',
);
}
Expand All @@ -107,8 +105,8 @@ export function findModule(
const errorMsg = foundRoutingModule
? 'Could not find a non Routing NgModule.' +
`\nModules with suffix '${routingModuleExt}' are strictly reserved for routing.` +
'\nUse the skip-import option to skip importing in NgModule.'
: 'Could not find an NgModule. Use the skip-import option to skip importing in NgModule.';
`\nUse the '--skip-import' option to skip importing in NgModule.`
: `Could not find an NgModule. Use the '--skip-import' option to skip importing in NgModule.`;

throw new Error(errorMsg);
}
Expand Down

0 comments on commit bbe74b8

Please sign in to comment.