Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Account for string literals in styles and the new styleUrls #25812

Merged
merged 2 commits into from Sep 13, 2023
Merged

Account for string literals in styles and the new styleUrls #25812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17f0cd5
fix(@ngtools/webpack): account for styles specified as string literal…
crisbeto Sep 11, 2023
734a46e
fix(@angular-devkit/build-angular): account for styles specified as s…
crisbeto Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
113 changes: 64 additions & 49 deletions packages/angular_devkit/build_angular/src/tools/esbuild/angular/jit-resource-transformer.ts
View file
Open in desktop
Expand Up @@ -143,75 +143,90 @@ function visitComponentMetadata(
switch (node.name.text) {
case 'templateUrl':
// Only analyze string literals
if (
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I couldn't figure out where this logic is tested so I didn't add any tests. I can add some if somebody can point me in the right direction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can add a test similar to https://github.com/angular/angular-cli/blob/734a46e99ff94d5fffd93f223e4b2b7646790f78/packages/angular_devkit/build_angular/src/builders/application/tests/behavior/component-stylesheets_spec.ts or an E2E https://github.com/angular/angular-cli/tree/main/tests/legacy-cli/e2e/tests/build

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm okay, it looks like I can't write a test there, because the change hasn't landed in a release yet so writing a component with the new properties results in a compilation error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually even if this compiled, it wouldn't be testing the behavior we're looking for, because the transformer only runs on JIT code. As far as I can tell, these tests are running AOT compilation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually even if this compiled, it wouldn't be testing the behavior we're looking for, because the transformer only runs on JIT code. As far as I can tell, these tests are running AOT compilation.

You can switch between AOT and JIT by providing the aot as an option in

angular-cli/packages/angular_devkit/build_angular/src/builders/application/tests/behavior/component-stylesheets_spec.ts

Line 20 in 734a46e

...BASE_OPTIONS,

Let's land it for now, and follow up with a unit test in a separate PR once the changes in the FW are released on NPM.

!ts.isStringLiteral(node.initializer) &&
!ts.isNoSubstitutionTemplateLiteral(node.initializer)
) {
if (!ts.isStringLiteralLike(node.initializer)) {
return node;
}

const url = node.initializer.text;
if (!url) {
return node;
}

return nodeFactory.updatePropertyAssignment(
node,
nodeFactory.createIdentifier('template'),
createResourceImport(
nodeFactory,
generateJitFileUri(url, 'template'),
resourceImportDeclarations,
),
);
return node.initializer.text.length === 0
? node
: nodeFactory.updatePropertyAssignment(
node,
nodeFactory.createIdentifier('template'),
createResourceImport(
nodeFactory,
generateJitFileUri(node.initializer.text, 'template'),
resourceImportDeclarations,
),
);
case 'styles':
if (!ts.isArrayLiteralExpression(node.initializer)) {
return node;
if (ts.isStringLiteralLike(node.initializer)) {
styleReplacements.unshift(
createResourceImport(
nodeFactory,
generateJitInlineUri(node.initializer.text, 'style'),
resourceImportDeclarations,
),
);

return undefined;
}

const inlineStyles = ts.visitNodes(node.initializer.elements, (node) => {
if (!ts.isStringLiteral(node) && !ts.isNoSubstitutionTemplateLiteral(node)) {
return node;
}
if (ts.isArrayLiteralExpression(node.initializer)) {
const inlineStyles = ts.visitNodes(node.initializer.elements, (node) => {
if (!ts.isStringLiteralLike(node)) {
return node;
}

return node.text.length === 0
? undefined // An empty inline style is equivalent to not having a style element
: createResourceImport(
nodeFactory,
generateJitInlineUri(node.text, 'style'),
resourceImportDeclarations,
);
}) as ts.NodeArray<ts.Expression>;

// Inline styles should be placed first
styleReplacements.unshift(...inlineStyles);

// The inline styles will be added afterwards in combination with any external styles
return undefined;
}

const contents = node.text;
if (!contents) {
// An empty inline style is equivalent to not having a style element
return undefined;
}
return node;

return createResourceImport(
nodeFactory,
generateJitInlineUri(contents, 'style'),
resourceImportDeclarations,
case 'styleUrl':
if (ts.isStringLiteralLike(node.initializer)) {
styleReplacements.push(
createResourceImport(
nodeFactory,
generateJitFileUri(node.initializer.text, 'style'),
resourceImportDeclarations,
),
);
}) as ts.NodeArray<ts.Expression>;

// Inline styles should be placed first
styleReplacements.unshift(...inlineStyles);
return undefined;
}

return node;

// The inline styles will be added afterwards in combination with any external styles
return undefined;
case 'styleUrls':
if (!ts.isArrayLiteralExpression(node.initializer)) {
return node;
}

const externalStyles = ts.visitNodes(node.initializer.elements, (node) => {
if (!ts.isStringLiteral(node) && !ts.isNoSubstitutionTemplateLiteral(node)) {
return node;
}

const url = node.text;
if (!url) {
if (!ts.isStringLiteralLike(node)) {
return node;
}

return createResourceImport(
nodeFactory,
generateJitFileUri(url, 'style'),
resourceImportDeclarations,
);
return node.text
? createResourceImport(
nodeFactory,
generateJitFileUri(node.text, 'style'),
resourceImportDeclarations,
)
: undefined;
}) as ts.NodeArray<ts.Expression>;

// External styles are applied after any inline styles
Expand Down
96 changes: 63 additions & 33 deletions packages/ngtools/webpack/src/transformers/replace_resources.ts
View file
Open in desktop
Expand Up @@ -180,42 +180,37 @@ function visitComponentMetadata(
importName,
);
case 'styles':
case 'styleUrl':
case 'styleUrls':
if (!ts.isArrayLiteralExpression(node.initializer)) {
const isInlineStyle = name === 'styles';
let styles: Iterable<ts.Expression>;

if (ts.isStringLiteralLike(node.initializer)) {
styles = [
transformInlineStyleLiteral(
node.initializer,
nodeFactory,
isInlineStyle,
inlineStyleFileExtension,
resourceImportDeclarations,
moduleKind,
) as ts.StringLiteralLike,
];
} else if (ts.isArrayLiteralExpression(node.initializer)) {
styles = ts.visitNodes(node.initializer.elements, (node) =>
transformInlineStyleLiteral(
node,
nodeFactory,
isInlineStyle,
inlineStyleFileExtension,
resourceImportDeclarations,
moduleKind,
),
) as ts.NodeArray<ts.Expression>;
} else {
return node;
}

const isInlineStyle = name === 'styles';
const styles = ts.visitNodes(node.initializer.elements, (node) => {
if (!ts.isStringLiteral(node) && !ts.isNoSubstitutionTemplateLiteral(node)) {
return node;
}

let url;
if (isInlineStyle) {
if (inlineStyleFileExtension) {
const data = Buffer.from(node.text).toString('base64');
const containingFile = node.getSourceFile().fileName;
// app.component.ts.css?ngResource!=!@ngtools/webpack/src/loaders/inline-resource.js?data=...!app.component.ts
url =
`${containingFile}.${inlineStyleFileExtension}?${NG_COMPONENT_RESOURCE_QUERY}` +
`!=!${InlineAngularResourceLoaderPath}?data=${encodeURIComponent(
data,
)}!${containingFile}`;
} else {
return nodeFactory.createStringLiteral(node.text);
}
} else {
url = getResourceUrl(node);
}

if (!url) {
return node;
}

return createResourceImport(nodeFactory, url, resourceImportDeclarations, moduleKind);
}) as ts.NodeArray<ts.Expression>;

// Styles should be placed first
if (isInlineStyle) {
styleReplacements.unshift(...styles);
Expand All @@ -229,9 +224,44 @@ function visitComponentMetadata(
}
}

function transformInlineStyleLiteral(
node: ts.Node,
nodeFactory: ts.NodeFactory,
isInlineStyle: boolean,
inlineStyleFileExtension: string | undefined,
resourceImportDeclarations: ts.ImportDeclaration[],
moduleKind: ts.ModuleKind,
) {
if (!ts.isStringLiteralLike(node)) {
return node;
}

if (!isInlineStyle) {
const url = getResourceUrl(node);

return url
? createResourceImport(nodeFactory, url, resourceImportDeclarations, moduleKind)
: node;
}

if (!inlineStyleFileExtension) {
return nodeFactory.createStringLiteral(node.text);
}

const data = Buffer.from(node.text).toString('base64');
const containingFile = node.getSourceFile().fileName;

// app.component.ts.css?ngResource!=!@ngtools/webpack/src/loaders/inline-resource.js?data=...!app.component.ts
const url =
`${containingFile}.${inlineStyleFileExtension}?${NG_COMPONENT_RESOURCE_QUERY}` +
`!=!${InlineAngularResourceLoaderPath}?data=${encodeURIComponent(data)}!${containingFile}`;

return createResourceImport(nodeFactory, url, resourceImportDeclarations, moduleKind);
}

export function getResourceUrl(node: ts.Node): string | null {
// only analyze strings
if (!ts.isStringLiteral(node) && !ts.isNoSubstitutionTemplateLiteral(node)) {
if (!ts.isStringLiteralLike(node)) {
return null;
}

Expand Down
39 changes: 39 additions & 0 deletions packages/ngtools/webpack/src/transformers/replace_resources_spec.ts
View file
Open in desktop
Expand Up @@ -299,6 +299,45 @@ describe('@ngtools/webpack transformers', () => {
expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`);
});

it('should replace resources specified as string literals', () => {
const input = tags.stripIndent`
import { Component } from '@angular/core';

@Component({
selector: 'app-root',
templateUrl: './app.component.html',
styles: 'h2 {font-size: 10px}',
styleUrl: './app.component.css'
})
export class AppComponent {
title = 'app';
}
`;
const output = tags.stripIndent`
import { __decorate } from "tslib";
import __NG_CLI_RESOURCE__0 from "./app.component.html?ngResource";
import __NG_CLI_RESOURCE__1 from "./app.component.css?ngResource";
import { Component } from '@angular/core';

let AppComponent = class AppComponent {
constructor() {
this.title = 'app';
}
};
AppComponent = __decorate([
Component({
selector: 'app-root',
template: __NG_CLI_RESOURCE__0,
styles: ["h2 {font-size: 10px}", __NG_CLI_RESOURCE__1]
})
], AppComponent);
export { AppComponent };
`;

const result = transform(input);
expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`);
});

it('should not replace resources if not in Component decorator', () => {
const input = tags.stripIndent`
import { Component } from '@angular/core';
Expand Down