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

Use asset modules and resourceQuery #22252

Merged
merged 2 commits into from
Dec 8, 2021
Merged

Use asset modules and resourceQuery #22252

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
1ccce7d
refactor(@ngtools/webpack): remove direct angular resource loader
alan-agius4 Nov 30, 2021
4c1049e
fix(@angular-devkit/build-angular): differentiate components and glob…
alan-agius4 Dec 2, 2021
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
22 changes: 22 additions & 0 deletions packages/angular_devkit/build_angular/src/builders/browser/specs/styles_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,4 +655,26 @@ describe('Browser Builder styles', () => {
result = await browserBuild(architect, host, target, { optimization: true });
expect(await result.files['styles.css']).toContain('rgba(0,0,0,.15)');
});

it('works when using the same css file in `styles` and `stylesUrl`', async () => {
host.writeMultipleFiles({
'src/styles.css': `
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is used as a global style by default right? Is there some way we can explicitly list this as a global file as part of this test? As is, we're implicitly depending on this default which could change and invalidate the intent of the test but probably leave it still passing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is correct. Added to explicitly set the styles value to src/styles.css

div { color: red }
`,
'./src/app/app.component.ts': `
import { Component } from '@angular/core';

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

await browserBuild(architect, host, target, { styles: ['src/styles.css'] });
});
});
2 changes: 1 addition & 1 deletion ...ages/angular_devkit/build_angular/src/builders/browser/tests/options/named-chunks_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { BASE_OPTIONS, BROWSER_BUILDER_INFO, describeBuilder } from '../setup';

const MAIN_OUTPUT = 'dist/main.js';
const NAMED_LAZY_OUTPUT = 'dist/src_lazy-module_ts.js';
const UNNAMED_LAZY_OUTPUT = 'dist/8.js';
const UNNAMED_LAZY_OUTPUT = 'dist/459.js';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of my own curiosity, where does this come from and why does it need to change? Is this some kind of hash of a lazy loaded module which happened to change slightly from this commit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ID generation is internal to Webpack, I think it changed due to the changes in resource paths due to the additional of the query parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha, thanks for the clarification.


describeBuilder(buildWebpackBrowser, BROWSER_BUILDER_INFO, (harness) => {
describe('Option: "namedChunks"', () => {
Expand Down
6 changes: 6 additions & 0 deletions packages/angular_devkit/build_angular/src/webpack/configs/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,12 @@ export async function getCommonConfig(wco: WebpackConfigOptions): Promise<Config
: undefined,

rules: [
{
test: /\.?(svg|html)$/,
// Only process HTML and SVG which are known Angular component resources.
resourceQuery: /\?ngResource/,
type: 'asset/source',
},
{
// Mark files inside `rxjs/add` as containing side effects.
// If this is fixed upstream and the fixed version becomes the minimum
Expand Down
4 changes: 2 additions & 2 deletions packages/angular_devkit/build_angular/src/webpack/configs/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,14 +377,14 @@ export function getStylesConfig(wco: WebpackConfigOptions): Configuration {
oneOf: [
// Component styles are all styles except defined global styles
{
exclude: globalStylePaths,
use: componentStyleLoaders,
resourceQuery: /\?ngResource/,
type: 'asset/source',
},
// Global styles are only defined global styles
{
include: globalStylePaths,
use: globalStyleLoaders,
resourceQuery: { not: [/\?ngResource/] },
},
],
},
Expand Down
8 changes: 1 addition & 7 deletions packages/ngtools/webpack/src/ivy/transformation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,14 @@ export function createJitTransformers(
builder: ts.BuilderProgram,
compilerCli: typeof import('@angular/compiler-cli'),
options: {
directTemplateLoading?: boolean;
inlineStyleFileExtension?: string;
},
): ts.CustomTransformers {
const getTypeChecker = () => builder.getProgram().getTypeChecker();

return {
before: [
replaceResources(
() => true,
getTypeChecker,
options.directTemplateLoading,
options.inlineStyleFileExtension,
),
replaceResources(() => true, getTypeChecker, options.inlineStyleFileExtension),
compilerCli.constructorParametersDownlevelTransform(builder.getProgram()),
],
};
Expand Down
19 changes: 0 additions & 19 deletions packages/ngtools/webpack/src/loaders/direct-resource.ts
View file
Open in desktop

This file was deleted.

28 changes: 18 additions & 10 deletions packages/ngtools/webpack/src/resource_loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
InlineAngularResourceLoaderPath,
InlineAngularResourceSymbol,
} from './loaders/inline-resource';
import { NG_COMPONENT_RESOURCE_QUERY } from './transformers/replace_resources';

interface CompilationOutput {
content: string;
Expand Down Expand Up @@ -110,17 +111,24 @@ export class WebpackResourceLoader {
throw new Error('WebpackResourceLoader cannot be used without parentCompilation');
}

// Create a special URL for reading the resource from memory
const entry =
filePath ||
(resourceType
? `${containingFile}-${this.outputPathCounter}.${fileExtension}!=!${this.inlineDataLoaderPath}!${containingFile}`
: // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
`angular-resource:${resourceType},${createHash('md5').update(data!).digest('hex')}`);
const getEntry = (): string => {
alan-agius4 marked this conversation as resolved.
Show resolved Hide resolved
if (filePath) {
return `${filePath}?${NG_COMPONENT_RESOURCE_QUERY}`;
} else if (resourceType) {
return (
// app.component.ts-2.css?ngResource!=!@ngtools/webpack/src/loaders/inline-resource.js!app.component.ts
`${containingFile}-${this.outputPathCounter}.${fileExtension}` +
`?${NG_COMPONENT_RESOURCE_QUERY}!=!${this.inlineDataLoaderPath}!${containingFile}`
alan-agius4 marked this conversation as resolved.
Show resolved Hide resolved
);
} else if (data) {
// Create a special URL for reading the resource from memory
return `angular-resource:${resourceType},${createHash('md5').update(data).digest('hex')}`;
}

if (!entry) {
throw new Error(`"filePath" or "data" must be specified.`);
}
throw new Error(`"filePath", "resourceType" or "data" must be specified.`);
};

const entry = getEntry();

// Simple sanity check.
if (filePath?.match(/\.[jt]s$/)) {
Expand Down
27 changes: 11 additions & 16 deletions packages/ngtools/webpack/src/transformers/replace_resources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@
*/

import * as ts from 'typescript';
import { DirectAngularResourceLoaderPath } from '../loaders/direct-resource';
import { InlineAngularResourceLoaderPath } from '../loaders/inline-resource';

export const NG_COMPONENT_RESOURCE_QUERY = 'ngResource';

export function replaceResources(
shouldTransform: (fileName: string) => boolean,
getTypeChecker: () => ts.TypeChecker,
directTemplateLoading = false,
inlineStyleFileExtension?: string,
): ts.TransformerFactory<ts.SourceFile> {
return (context: ts.TransformationContext) => {
Expand All @@ -30,7 +30,6 @@ export function replaceResources(
nodeFactory,
node,
typeChecker,
directTemplateLoading,
resourceImportDeclarations,
moduleKind,
inlineStyleFileExtension,
Expand Down Expand Up @@ -81,7 +80,6 @@ function visitDecorator(
nodeFactory: ts.NodeFactory,
node: ts.Decorator,
typeChecker: ts.TypeChecker,
directTemplateLoading: boolean,
resourceImportDeclarations: ts.ImportDeclaration[],
moduleKind?: ts.ModuleKind,
inlineStyleFileExtension?: string,
Expand Down Expand Up @@ -111,7 +109,6 @@ function visitDecorator(
nodeFactory,
node,
styleReplacements,
directTemplateLoading,
resourceImportDeclarations,
moduleKind,
inlineStyleFileExtension,
Expand Down Expand Up @@ -144,7 +141,6 @@ function visitComponentMetadata(
nodeFactory: ts.NodeFactory,
node: ts.ObjectLiteralElementLike,
styleReplacements: ts.Expression[],
directTemplateLoading: boolean,
resourceImportDeclarations: ts.ImportDeclaration[],
moduleKind: ts.ModuleKind = ts.ModuleKind.ES2015,
inlineStyleFileExtension?: string,
Expand All @@ -159,11 +155,7 @@ function visitComponentMetadata(
return undefined;

case 'templateUrl':
const loaderOptions = moduleKind < ts.ModuleKind.ES2015 ? '?esModule=false' : '';
const url = getResourceUrl(
node.initializer,
directTemplateLoading ? `!${DirectAngularResourceLoaderPath}${loaderOptions}!` : '',
);
const url = getResourceUrl(node.initializer);
if (!url) {
return node;
}
Expand Down Expand Up @@ -200,9 +192,12 @@ function visitComponentMetadata(
if (inlineStyleFileExtension) {
const data = Buffer.from(node.text).toString('base64');
const containingFile = node.getSourceFile().fileName;
url = `${containingFile}.${inlineStyleFileExtension}!=!${InlineAngularResourceLoaderPath}?data=${encodeURIComponent(
data,
)}!${containingFile}`;
// app.component.ts.css?ngResource!=!@ngtools/webpack/src/loaders/inline-resource.js?data=...!app.component.ts
url =
alan-agius4 marked this conversation as resolved.
Show resolved Hide resolved
`${containingFile}.${inlineStyleFileExtension}?${NG_COMPONENT_RESOURCE_QUERY}` +
`!=!${InlineAngularResourceLoaderPath}?data=${encodeURIComponent(
data,
)}!${containingFile}`;
} else {
return nodeFactory.createStringLiteral(node.text);
}
Expand Down Expand Up @@ -230,13 +225,13 @@ function visitComponentMetadata(
}
}

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

return `${loader}${/^\.?\.\//.test(node.text) ? '' : './'}${node.text}`;
return `${/^\.?\.\//.test(node.text) ? '' : './'}${node.text}?${NG_COMPONENT_RESOURCE_QUERY}`;
}

function isComponentDecorator(node: ts.Node, typeChecker: ts.TypeChecker): node is ts.Decorator {
Expand Down