Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion packages/ngtools/webpack/src/transformers/replace_resources.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export function replaceResources(
resourceImportDeclarations,
moduleKind,
inlineStyleFileExtension,
node,
),
),
...(ts.getModifiers(node) ?? []),
Expand Down Expand Up @@ -87,6 +88,7 @@ function visitDecorator(
resourceImportDeclarations: ts.ImportDeclaration[],
moduleKind?: ts.ModuleKind,
inlineStyleFileExtension?: string,
classDeclaration?: ts.ClassDeclaration,
): ts.Decorator {
if (!isComponentDecorator(node, typeChecker)) {
return node;
Expand All @@ -106,6 +108,8 @@ function visitDecorator(
const objectExpression = args[0];
const styleReplacements: ts.Expression[] = [];

const className = classDeclaration?.name?.text ?? 'Unknown';

// visit all properties
let properties = ts.visitNodes(objectExpression.properties, (node) =>
ts.isObjectLiteralElementLike(node)
Expand All @@ -116,6 +120,7 @@ function visitDecorator(
resourceImportDeclarations,
moduleKind,
inlineStyleFileExtension,
className,
)
: node,
) as ts.NodeArray<ts.ObjectLiteralElementLike>;
Expand Down Expand Up @@ -148,6 +153,7 @@ function visitComponentMetadata(
resourceImportDeclarations: ts.ImportDeclaration[],
moduleKind: ts.ModuleKind = ts.ModuleKind.ES2015,
inlineStyleFileExtension?: string,
className?: string,
): ts.ObjectLiteralElementLike | undefined {
if (!ts.isPropertyAssignment(node) || ts.isComputedPropertyName(node.name)) {
return node;
Expand All @@ -161,7 +167,14 @@ function visitComponentMetadata(
case 'templateUrl': {
const url = getResourceUrl(node.initializer);
if (!url) {
return node;
const sourceFile = node.getSourceFile();
const { line } = sourceFile.getLineAndCharacterOfPosition(node.initializer.getStart());

throw new Error(
`Component '${className}' in '${sourceFile.fileName}' contains a non-string literal ` +
`'templateUrl' value at line ${line + 1}. The 'templateUrl' property must be a ` +
`string literal. Expressions, variables, or other dynamic values are not supported.`,
);
Comment on lines +173 to +177
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For improved readability and maintainability, consider using a template literal for this error message instead of string concatenation.

throw new Error(`Component '${className}' in '${sourceFile.fileName}' contains a non-string literal 'templateUrl' value at line ${line + 1}. The 'templateUrl' property must be a string literal. Expressions, variables, or other dynamic values are not supported.`);

}
Comment on lines 169 to 178
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This change correctly adds a build-time error for dynamic templateUrl values. However, the PR description states that this should also apply to styleUrls, but that case is not handled. Non-string literal values in styleUrls are still silently ignored, which can lead to confusing runtime issues.

To ensure consistency and prevent these issues, a similar error should be thrown for non-string literal expressions in styleUrl and styleUrls.

This would likely involve:

  • In visitComponentMetadata, for the styleUrl/styleUrls case, check if node.initializer is not a string or array literal and throw an error if so.
  • Passing the className to transformInlineStyleLiteral.
  • In transformInlineStyleLiteral, when !ts.isStringLiteralLike(node) and !isInlineStyle, throw an error similar to the one for templateUrl.

Additionally, please add tests for these styleUrls cases to replace_resources_spec.ts to cover these scenarios.


const importName = createResourceImport(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -469,4 +469,46 @@ describe('find_resources', () => {
const result = transform(input, false);
expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`);
});

it('should throw an error when templateUrl is a conditional expression', () => {
const input = tags.stripIndent`
import { Component } from '@angular/core';

@Component({
selector: 'app-root',
templateUrl: true === true
? './app.component.html'
: './app.component.copy.html',
styleUrls: ['./app.component.css', './app.component.2.css']
})
export class AppComponent {
title = 'app';
}
`;

expect(() => transform(input)).toThrowError(
/Component 'AppComponent'.*contains a non-string literal 'templateUrl' value/,
);
});

it('should throw an error when templateUrl is a variable reference', () => {
const input = tags.stripIndent`
import { Component } from '@angular/core';

const myTemplate = './app.component.html';

@Component({
selector: 'app-root',
templateUrl: myTemplate,
styleUrls: ['./app.component.css']
})
export class AppComponent {
title = 'app';
}
`;

expect(() => transform(input)).toThrowError(
/Component 'AppComponent'.*contains a non-string literal 'templateUrl' value/,
);
});
});
Loading