From c9d90786d0a6421bbb21b9d1649d031b34e3fa5d Mon Sep 17 00:00:00 2001 From: Bjarki Date: Mon, 19 Aug 2024 17:56:05 +0000 Subject: [PATCH 01/36] fix(upgrade): Address Trusted Types violations in @angular/upgrade (#57454) Angular applications that are AngularJS hybrids are currently unable to adopt Trusted Types due to violations eminating from an innerHTML assignment in the @angular/upgrade package. This commit allows developers of such applications to optionally ignore this class of violations by configuring the Trusted Types header to allow the new angular#unsafe-upgrade policy. Note that the policy is explicitly labeled as unsafe as it does not in any way mitigate the security risk of using AngularJS in an Angular application, but does unblock Trusted Types adoption enabling XSS protection for other parts of the application. The implementation follows the approach taken in @angular/core; see packages/core/src/util/security. PR Close #57454 --- adev/src/content/guide/security.md | 13 ++-- .../src/common/src/security/trusted_types.ts | 61 +++++++++++++++++++ .../common/src/security/trusted_types_defs.ts | 42 +++++++++++++ .../upgrade/src/common/src/upgrade_helper.ts | 23 ++++--- .../src/dynamic/src/upgrade_ng1_adapter.ts | 3 +- 5 files changed, 123 insertions(+), 19 deletions(-) create mode 100644 packages/upgrade/src/common/src/security/trusted_types.ts create mode 100644 packages/upgrade/src/common/src/security/trusted_types_defs.ts diff --git a/adev/src/content/guide/security.md b/adev/src/content/guide/security.md index 5fe208d096574..c3b5aaaeae630 100644 --- a/adev/src/content/guide/security.md +++ b/adev/src/content/guide/security.md @@ -201,12 +201,13 @@ See [caniuse.com/trusted-types](https://caniuse.com/trusted-types) for the curre To enforce Trusted Types for your application, you must configure your application's web server to emit HTTP headers with one of the following Angular policies: -| Policies | Detail | -| :---------------------- | :----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `angular` | This policy is used in security-reviewed code that is internal to Angular, and is required for Angular to function when Trusted Types are enforced. Any inline template values or content sanitized by Angular is treated as safe by this policy. | -| `angular#unsafe-bypass` | This policy is used for applications that use any of the methods in Angular's [DomSanitizer](api/platform-browser/DomSanitizer) that bypass security, such as `bypassSecurityTrustHtml`. Any application that uses these methods must enable this policy. | -| `angular#unsafe-jit` | This policy is used by the [Just-In-Time (JIT) compiler](api/core/Compiler). You must enable this policy if your application interacts directly with the JIT compiler or is running in JIT mode using the [platform browser dynamic](api/platform-browser-dynamic/platformBrowserDynamic). | -| `angular#bundler` | This policy is used by the Angular CLI bundler when creating lazy chunk files. | +| Policies | Detail | +| :----------------------- | :----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `angular` | This policy is used in security-reviewed code that is internal to Angular, and is required for Angular to function when Trusted Types are enforced. Any inline template values or content sanitized by Angular is treated as safe by this policy. | +| `angular#bundler` | This policy is used by the Angular CLI bundler when creating lazy chunk files. | +| `angular#unsafe-bypass` | This policy is used for applications that use any of the methods in Angular's [DomSanitizer](api/platform-browser/DomSanitizer) that bypass security, such as `bypassSecurityTrustHtml`. Any application that uses these methods must enable this policy. | +| `angular#unsafe-jit` | This policy is used by the [Just-In-Time (JIT) compiler](api/core/Compiler). You must enable this policy if your application interacts directly with the JIT compiler or is running in JIT mode using the [platform browser dynamic](api/platform-browser-dynamic/platformBrowserDynamic). | +| `angular#unsafe-upgrade` | This policy is used by the [@angular/upgrade](api/upgrade/static/UpgradeModule) package. You must enable this policy if your application is an AngularJS hybrid. | You should configure the HTTP headers for Trusted Types in the following locations: diff --git a/packages/upgrade/src/common/src/security/trusted_types.ts b/packages/upgrade/src/common/src/security/trusted_types.ts new file mode 100644 index 0000000000000..d8c2cdd79a44c --- /dev/null +++ b/packages/upgrade/src/common/src/security/trusted_types.ts @@ -0,0 +1,61 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +/** + * @fileoverview + * A module to facilitate use of a Trusted Types policy internally within + * the upgrade package. It lazily constructs the Trusted Types policy, providing + * helper utilities for promoting strings to Trusted Types. When Trusted Types + * are not available, strings are used as a fallback. + * @security All use of this module is security-sensitive and should go through + * security review. + */ + +import {TrustedHTML, TrustedTypePolicy, TrustedTypePolicyFactory} from './trusted_types_defs'; + +/** + * The Trusted Types policy, or null if Trusted Types are not + * enabled/supported, or undefined if the policy has not been created yet. + */ +let policy: TrustedTypePolicy | null | undefined; + +/** + * Returns the Trusted Types policy, or null if Trusted Types are not + * enabled/supported. The first call to this function will create the policy. + */ +function getPolicy(): TrustedTypePolicy | null { + if (policy === undefined) { + policy = null; + const windowWithTrustedTypes = window as unknown as {trustedTypes?: TrustedTypePolicyFactory}; + if (windowWithTrustedTypes.trustedTypes) { + try { + policy = windowWithTrustedTypes.trustedTypes.createPolicy('angular#unsafe-upgrade', { + createHTML: (s: string) => s, + }); + } catch { + // trustedTypes.createPolicy throws if called with a name that is + // already registered, even in report-only mode. Until the API changes, + // catch the error not to break the applications functionally. In such + // cases, the code will fall back to using strings. + } + } + } + return policy; +} + +/** + * Unsafely promote a legacy AngularJS template to a TrustedHTML, falling back + * to strings when Trusted Types are not available. + * @security This is a security-sensitive function; any use of this function + * must go through security review. In particular, the template string should + * always be under full control of the application author, as untrusted input + * can cause an XSS vulnerability. + */ +export function trustedHTMLFromLegacyTemplate(html: string): TrustedHTML | string { + return getPolicy()?.createHTML(html) || html; +} diff --git a/packages/upgrade/src/common/src/security/trusted_types_defs.ts b/packages/upgrade/src/common/src/security/trusted_types_defs.ts new file mode 100644 index 0000000000000..7799f7f0ce502 --- /dev/null +++ b/packages/upgrade/src/common/src/security/trusted_types_defs.ts @@ -0,0 +1,42 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +/** + * @fileoverview + * While Angular only uses Trusted Types internally for the time being, + * references to Trusted Types could leak into our public API, which would force + * anyone compiling against @angular/upgrade to provide the @types/trusted-types + * package in their compilation unit. + * + * Until https://github.com/microsoft/TypeScript/issues/30024 is resolved, we + * will keep Angular's public API surface free of references to Trusted Types. + * For internal and semi-private APIs that need to reference Trusted Types, the + * minimal type definitions for the Trusted Types API provided by this module + * should be used instead. They are marked as "declare" to prevent them from + * being renamed by compiler optimization. + * + * Adapted from + * https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/trusted-types/index.d.ts + * but restricted to the API surface used within Angular, mimicking the approach + * in packages/core/src/util/security/trusted_type_defs.ts. + */ + +export type TrustedHTML = string & { + __brand__: 'TrustedHTML'; +}; + +export interface TrustedTypePolicyFactory { + createPolicy( + policyName: string, + policyOptions: {createHTML?: (input: string) => string}, + ): TrustedTypePolicy; +} + +export interface TrustedTypePolicy { + createHTML(input: string): TrustedHTML; +} diff --git a/packages/upgrade/src/common/src/upgrade_helper.ts b/packages/upgrade/src/common/src/upgrade_helper.ts index 2d2bed03e1889..003a7669a38b7 100644 --- a/packages/upgrade/src/common/src/upgrade_helper.ts +++ b/packages/upgrade/src/common/src/upgrade_helper.ts @@ -26,6 +26,8 @@ import { } from './angular1'; import {$COMPILE, $CONTROLLER, $HTTP_BACKEND, $INJECTOR, $TEMPLATE_CACHE} from './constants'; import {cleanData, controllerKey, directiveNormalize, isFunction} from './util'; +import {TrustedHTML} from './security/trusted_types_defs'; +import {trustedHTMLFromLegacyTemplate} from './security/trusted_types'; // Constants const REQUIRE_PREFIX_RE = /^(\^\^?)?(\?)?(\^\^?)?/; @@ -91,16 +93,16 @@ export class UpgradeHelper { directive: IDirective, fetchRemoteTemplate = false, $element?: IAugmentedJQuery, - ): string | Promise { + ): string | TrustedHTML | Promise { if (directive.template !== undefined) { - return getOrCall(directive.template, $element); + return trustedHTMLFromLegacyTemplate(getOrCall(directive.template, $element)); } else if (directive.templateUrl) { const $templateCache = $injector.get($TEMPLATE_CACHE) as ITemplateCacheService; const url = getOrCall(directive.templateUrl, $element); const template = $templateCache.get(url); if (template !== undefined) { - return template; + return trustedHTMLFromLegacyTemplate(template); } else if (!fetchRemoteTemplate) { throw new Error('loading directive templates asynchronously is not supported'); } @@ -109,7 +111,7 @@ export class UpgradeHelper { const $httpBackend = $injector.get($HTTP_BACKEND) as IHttpBackendService; $httpBackend('GET', url, null, (status: number, response: string) => { if (status === 200) { - resolve($templateCache.put(url, response)); + resolve(trustedHTMLFromLegacyTemplate($templateCache.put(url, response))); } else { reject(`GET component template from '${url}' returned '${status}: ${response}'`); } @@ -131,14 +133,11 @@ export class UpgradeHelper { return controller; } - compileTemplate(template?: string): ILinkFn { + compileTemplate(template?: string | TrustedHTML): ILinkFn { if (template === undefined) { - template = UpgradeHelper.getTemplate( - this.$injector, - this.directive, - false, - this.$element, - ) as string; + template = UpgradeHelper.getTemplate(this.$injector, this.directive, false, this.$element) as + | string + | TrustedHTML; } return this.compileHtml(template); @@ -251,7 +250,7 @@ export class UpgradeHelper { return requiredControllers; } - private compileHtml(html: string): ILinkFn { + private compileHtml(html: string | TrustedHTML): ILinkFn { this.element.innerHTML = html; return this.$compile(this.element.childNodes); } diff --git a/packages/upgrade/src/dynamic/src/upgrade_ng1_adapter.ts b/packages/upgrade/src/dynamic/src/upgrade_ng1_adapter.ts index bcf5b7656e7e2..21b4bcc17ef4f 100644 --- a/packages/upgrade/src/dynamic/src/upgrade_ng1_adapter.ts +++ b/packages/upgrade/src/dynamic/src/upgrade_ng1_adapter.ts @@ -36,6 +36,7 @@ import { UpgradeHelper, } from '../../common/src/upgrade_helper'; import {isFunction, strictEquals} from '../../common/src/util'; +import {trustedHTMLFromLegacyTemplate} from '../../common/src/security/trusted_types'; const CAMEL_CASE = /([A-Z])/g; const INITIAL_VALUE = { @@ -230,7 +231,7 @@ class UpgradeNg1ComponentAdapter implements OnInit, OnChanges, DoCheck { ngOnInit() { // Collect contents, insert and compile template const attachChildNodes: ILinkFn | undefined = this.helper.prepareTransclusion(); - const linkFn = this.helper.compileTemplate(this.template); + const linkFn = this.helper.compileTemplate(trustedHTMLFromLegacyTemplate(this.template)); // Instantiate controller (if not already done so) const controllerType = this.directive.controller; From 34ce46f7dc230354194d2a3e309c09f1164e7bfa Mon Sep 17 00:00:00 2001 From: Matthieu Riegler Date: Mon, 19 Aug 2024 11:45:21 +0200 Subject: [PATCH 02/36] docs: fix formating in class-binding (#57447) fixes #57430 PR Close #57447 --- .../content/guide/templates/class-binding.md | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/adev/src/content/guide/templates/class-binding.md b/adev/src/content/guide/templates/class-binding.md index 5564b22fadcc5..1e2fedb236d4a 100644 --- a/adev/src/content/guide/templates/class-binding.md +++ b/adev/src/content/guide/templates/class-binding.md @@ -31,12 +31,13 @@ If there are multiple bindings to the same class name, Angular uses styling prec The following table summarizes class binding syntax. -| Binding Type | Syntax | Input Type | Example Input Values | -|:--- |:--- |:--- |:--- | -| Single class binding | `[class.sale]="onSale"` | boolean | undefined | null | `true`, `false` | -| Multi-class binding | `[class]="classExpression"` | `string` | `"my-class-1 my-class-2 my-class-3"` | -| Multi-class binding | `[class]="classExpression"` | Record | `{foo: true, bar: false}` | -| Multi-class binding | `[class]="classExpression"` | Array | `['foo', 'bar']` | +| Binding Type | Syntax | Input Type | Example Input Values | +| :------------------- | :-------------------------- | :---------------------- | :----------------------------------- | +| Single class binding | `[class.sale]="onSale"` | `boolean|undefined|null`| `true`, `false` | +| Multi-class binding | `[class]="classExpression"` | `string` | `"my-class-1 my-class-2 my-class-3"` | +| Multi-class binding | `[class]="classExpression"` | `Record` | `{foo: true, bar: false}` | +| Multi-class binding | `[class]="classExpression"` | `Array` | `['foo', 'bar']` | + ## Binding to a single style @@ -74,12 +75,12 @@ If there are multiple bindings to the same style attribute, Angular uses styling The following table summarizes style binding syntax. -| Binding Type | Syntax | Input Type | Example Input Values | -|:--- |:--- |:--- |:--- | -| Single style binding | `[style.width]="width"` | string | undefined | null | `"100px"` | -| Single style binding with units | `[style.width.px]="width"` | number | undefined | null | `100` | -| Multi-style binding | `[style]="styleExpression"` | `string` | `"width: 100px; height: 100px"` | -| Multi-style binding | `[style]="styleExpression"` | Record | `{width: '100px', height: '100px'}` | +| Binding Type | Syntax | Input Type | Example Input Values | +| :------------------------------ | :-------------------------- | :---------------------- | :------------------------------ | +| Single style binding | `[style.width]="width"` | `string| undefined|null`| `"100px"` | +| Single style binding with units | `[style.width.px]="width"` | `number| undefined|null`| `100` | +| Multi-style binding | `[style]="styleExpression"` | `string` | `"width: 100px; height: 100px"` | +| Multi-style binding | `[style]="styleExpression"` | `Record` | `{width: '100px', height: '100px'}` | ## Styling precedence From 4716c3b9660b01f4ef3642fb774270b7f4a13d1a Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Fri, 23 Aug 2024 13:35:37 -0400 Subject: [PATCH 03/36] perf(compiler-cli): reduce duplicate component style resolution (#57502) Previously, the component handler was processing and resolving stylesheets referenced via `styleUrl`/`styleUrls` multiple times when generating the compiler metadata for components. The style resource information collection for such styles has been further consolidated to avoid repeat resource loader resolve calls which potentially could be expensive. Further optimization is possible for the inline style case. However, inline styles here only require AST traversal and no potentially expensive external resolve calls. PR Close #57502 --- .../annotations/component/src/handler.ts | 20 ++++--- .../annotations/component/src/resources.ts | 58 ++++--------------- 2 files changed, 23 insertions(+), 55 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts index 909c666e56d5e..e0341a4c91a08 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts @@ -160,7 +160,7 @@ import { import { _extractTemplateStyleUrls, extractComponentStyleUrls, - extractStyleResources, + extractInlineStyleResources, extractTemplate, makeResourceNotFoundError, ParsedTemplateWithSource, @@ -681,7 +681,7 @@ export class ComponentDecoratorHandler // component. let styles: string[] = []; - const styleResources = extractStyleResources(this.resourceLoader, component, containingFile); + const styleResources = extractInlineStyleResources(component); const styleUrls: StyleUrlMeta[] = [ ...extractComponentStyleUrls(this.evaluator, component), ..._extractTemplateStyleUrls(template), @@ -690,6 +690,16 @@ export class ComponentDecoratorHandler for (const styleUrl of styleUrls) { try { const resourceUrl = this.resourceLoader.resolve(styleUrl.url, containingFile); + if ( + styleUrl.source === ResourceTypeForDiagnostics.StylesheetFromDecorator && + ts.isStringLiteralLike(styleUrl.expression) + ) { + // Only string literal values from the decorator are considered style resources + styleResources.add({ + path: absoluteFrom(resourceUrl), + expression: styleUrl.expression, + }); + } const resourceStr = this.resourceLoader.load(resourceUrl); styles.push(resourceStr); if (this.depTracker !== null) { @@ -711,11 +721,7 @@ export class ComponentDecoratorHandler ? ResourceTypeForDiagnostics.StylesheetFromDecorator : ResourceTypeForDiagnostics.StylesheetFromTemplate; diagnostics.push( - makeResourceNotFoundError( - styleUrl.url, - styleUrl.nodeForError, - resourceType, - ).toDiagnostic(), + makeResourceNotFoundError(styleUrl.url, styleUrl.expression, resourceType).toDiagnostic(), ); } } diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/resources.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/resources.ts index e17bce455126d..0ac502b37fdae 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/resources.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/resources.ts @@ -38,7 +38,7 @@ import { */ export interface StyleUrlMeta { url: string; - nodeForError: ts.Node; + expression: ts.Expression; source: | ResourceTypeForDiagnostics.StylesheetFromTemplate | ResourceTypeForDiagnostics.StylesheetFromDecorator; @@ -123,7 +123,9 @@ export interface ExternalTemplateDeclaration extends CommonTemplateDeclaration { export type TemplateDeclaration = InlineTemplateDeclaration | ExternalTemplateDeclaration; /** Determines the node to use for debugging purposes for the given TemplateDeclaration. */ -export function getTemplateDeclarationNodeForError(declaration: TemplateDeclaration): ts.Node { +export function getTemplateDeclarationNodeForError( + declaration: TemplateDeclaration, +): ts.Expression { return declaration.isInline ? declaration.expression : declaration.templateUrlExpression; } @@ -629,7 +631,7 @@ export function extractComponentStyleUrls( { url: styleUrl, source: ResourceTypeForDiagnostics.StylesheetFromDecorator, - nodeForError: styleUrlExpr, + expression: styleUrlExpr, }, ]; } @@ -657,7 +659,7 @@ function extractStyleUrlsFromExpression( styleUrls.push({ url: styleUrl, source: ResourceTypeForDiagnostics.StylesheetFromDecorator, - nodeForError: styleUrlExpr, + expression: styleUrlExpr, }); } } @@ -675,7 +677,7 @@ function extractStyleUrlsFromExpression( styleUrls.push({ url: styleUrl, source: ResourceTypeForDiagnostics.StylesheetFromDecorator, - nodeForError: styleUrlsExpr, + expression: styleUrlsExpr, }); } } @@ -683,36 +685,12 @@ function extractStyleUrlsFromExpression( return styleUrls; } -export function extractStyleResources( - resourceLoader: ResourceLoader, - component: Map, - containingFile: string, -): ReadonlySet { +export function extractInlineStyleResources(component: Map): Set { const styles = new Set(); function stringLiteralElements(array: ts.ArrayLiteralExpression): ts.StringLiteralLike[] { return array.elements.filter((e): e is ts.StringLiteralLike => ts.isStringLiteralLike(e)); } - // If styleUrls is a literal array, process each resource url individually and register ones that - // are string literals. If `styleUrl` is specified, register a single stylesheet. Note that - // `styleUrl` and `styleUrls` are mutually-exclusive. This is validated in - // `extractComponentStyleUrls`. - const styleUrlExpr = component.get('styleUrl'); - const styleUrlsExpr = component.get('styleUrls'); - if (styleUrlsExpr !== undefined && ts.isArrayLiteralExpression(styleUrlsExpr)) { - for (const expression of stringLiteralElements(styleUrlsExpr)) { - const resource = stringLiteralUrlToResource(resourceLoader, expression, containingFile); - if (resource !== null) { - styles.add(resource); - } - } - } else if (styleUrlExpr !== undefined && ts.isStringLiteralLike(styleUrlExpr)) { - const resource = stringLiteralUrlToResource(resourceLoader, styleUrlExpr, containingFile); - if (resource !== null) { - styles.add(resource); - } - } - const stylesExpr = component.get('styles'); if (stylesExpr !== undefined) { if (ts.isArrayLiteralExpression(stylesExpr)) { @@ -727,31 +705,15 @@ export function extractStyleResources( return styles; } -function stringLiteralUrlToResource( - resourceLoader: ResourceLoader, - expression: ts.StringLiteralLike, - containingFile: string, -): Resource | null { - try { - const resourceUrl = resourceLoader.resolve(expression.text, containingFile); - return {path: absoluteFrom(resourceUrl), expression}; - } catch { - // Errors in style resource extraction do not need to be handled here. We will produce - // diagnostics for each one that fails in the analysis, after we evaluate the `styleUrls` - // expression to determine _all_ style resources, not just the string literals. - return null; - } -} - export function _extractTemplateStyleUrls(template: ParsedTemplateWithSource): StyleUrlMeta[] { if (template.styleUrls === null) { return []; } - const nodeForError = getTemplateDeclarationNodeForError(template.declaration); + const expression = getTemplateDeclarationNodeForError(template.declaration); return template.styleUrls.map((url) => ({ url, source: ResourceTypeForDiagnostics.StylesheetFromTemplate, - nodeForError, + expression, })); } From 46a9569ba8fe81c5d99c717ad7268485cb6f4e30 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sat, 24 Aug 2024 09:35:46 +0200 Subject: [PATCH 04/36] build: update to TypeScript 5.6 RC (#57507) Updates the repo to the release candidate of TypeScript 5.6. PR Close #57507 --- integration/typings_test_ts56/package.json | 2 +- integration/typings_test_ts56/yarn.lock | 8 ++++---- package.json | 2 +- yarn.lock | 8 ++++---- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/integration/typings_test_ts56/package.json b/integration/typings_test_ts56/package.json index d84d5ee79ea47..80ecf63ff3e9b 100644 --- a/integration/typings_test_ts56/package.json +++ b/integration/typings_test_ts56/package.json @@ -20,7 +20,7 @@ "@angular/upgrade": "file:../../dist/packages-dist/upgrade", "@types/jasmine": "file:../../node_modules/@types/jasmine", "rxjs": "file:../../node_modules/rxjs", - "typescript": "5.6.0-beta", + "typescript": "5.6.1-rc", "zone.js": "0.14.10" }, "scripts": { diff --git a/integration/typings_test_ts56/yarn.lock b/integration/typings_test_ts56/yarn.lock index 376c3feea09ad..37287ec17a971 100644 --- a/integration/typings_test_ts56/yarn.lock +++ b/integration/typings_test_ts56/yarn.lock @@ -746,10 +746,10 @@ tslib@^2.1.0, tslib@^2.3.0: resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.6.3.tgz#0438f810ad7a9edcde7a241c3d80db693c8cbfe0" integrity sha512-xNvxJEOUiWPGhUuUdQgAJPKOOJfGnIyKySOc09XkKsgdUV/3E2zvwZYdejjmRgPCgcym1juLH3226yA7sEFJKQ== -typescript@5.6.0-beta: - version "5.6.0-beta" - resolved "https://registry.yarnpkg.com/typescript/-/typescript-5.6.0-beta.tgz#0e5a341a32ad4454656ff9aea918bd14e1aedf1f" - integrity sha512-wqVumY25SwHmCJvZPHqbWfjSyWdAqTTUM+LqaPpFccOnln54EZoPr8Ra7qJV8HMEdFYcDc+S5xro0RyvaHvIvw== +typescript@5.6.1-rc: + version "5.6.1-rc" + resolved "https://registry.yarnpkg.com/typescript/-/typescript-5.6.1-rc.tgz#d5e4d7d8170174fed607b74cc32aba3d77018e02" + integrity sha512-E3b2+1zEFu84jB0YQi9BORDjz9+jGbwwy1Zi3G0LUNw7a7cePUrHMRNy8aPh53nXpkFGVHSxIZo5vKTfYaFiBQ== update-browserslist-db@^1.0.16: version "1.0.16" diff --git a/package.json b/package.json index cfaf2f5608a67..18868c4c69e8d 100644 --- a/package.json +++ b/package.json @@ -147,7 +147,7 @@ "tslib": "^2.3.0", "tslint": "6.1.3", "tsx": "^4.7.2", - "typescript": "5.6.0-beta", + "typescript": "5.6.1-rc", "webtreemap": "^2.0.1", "ws": "^8.15.0", "xhr2": "0.2.1", diff --git a/yarn.lock b/yarn.lock index 5a16001aaa229..7d318b37cd0de 100644 --- a/yarn.lock +++ b/yarn.lock @@ -16945,10 +16945,10 @@ typescript@5.5.4, typescript@^5.4.4, typescript@^5.4.5, typescript@^5.5.4: resolved "https://registry.yarnpkg.com/typescript/-/typescript-5.5.4.tgz#d9852d6c82bad2d2eda4fd74a5762a8f5909e9ba" integrity sha512-Mtq29sKDAEYP7aljRgtPOpTvOfbwRWlS6dPRzwjdE+C0R4brX/GUyhHSecbHMFLNBLcJIPt9nl9yG5TZ1weH+Q== -typescript@5.6.0-beta: - version "5.6.0-beta" - resolved "https://registry.yarnpkg.com/typescript/-/typescript-5.6.0-beta.tgz#0e5a341a32ad4454656ff9aea918bd14e1aedf1f" - integrity sha512-wqVumY25SwHmCJvZPHqbWfjSyWdAqTTUM+LqaPpFccOnln54EZoPr8Ra7qJV8HMEdFYcDc+S5xro0RyvaHvIvw== +typescript@5.6.1-rc: + version "5.6.1-rc" + resolved "https://registry.yarnpkg.com/typescript/-/typescript-5.6.1-rc.tgz#d5e4d7d8170174fed607b74cc32aba3d77018e02" + integrity sha512-E3b2+1zEFu84jB0YQi9BORDjz9+jGbwwy1Zi3G0LUNw7a7cePUrHMRNy8aPh53nXpkFGVHSxIZo5vKTfYaFiBQ== typescript@~4.9.0: version "4.9.5" From 6307f976d2fd70699bab4d8666e953858198d5b0 Mon Sep 17 00:00:00 2001 From: Angular Robot Date: Sat, 24 Aug 2024 08:42:47 +0000 Subject: [PATCH 05/36] docs: update Angular CLI help [main] (#57508) Updated Angular CLI help contents. PR Close #57508 --- adev/src/content/cli/help/build-info.json | 2 +- adev/src/content/cli/help/completion.json | 2 +- adev/src/content/cli/help/extract-i18n.json | 6 ------ adev/src/content/cli/help/generate.json | 6 ++++++ adev/src/content/cli/help/serve.json | 6 ------ 5 files changed, 8 insertions(+), 14 deletions(-) diff --git a/adev/src/content/cli/help/build-info.json b/adev/src/content/cli/help/build-info.json index 23ff11ecb7bd4..58faedde78024 100644 --- a/adev/src/content/cli/help/build-info.json +++ b/adev/src/content/cli/help/build-info.json @@ -1,4 +1,4 @@ { "branchName": "refs/heads/main", - "sha": "b08d472c072301d166137a1bc3634607682de2ad" + "sha": "3513cd179d428e6c8d6529f1010dac65282a8a44" } \ No newline at end of file diff --git a/adev/src/content/cli/help/completion.json b/adev/src/content/cli/help/completion.json index d0c4ba7bfb2c5..32a2e398588eb 100644 --- a/adev/src/content/cli/help/completion.json +++ b/adev/src/content/cli/help/completion.json @@ -3,7 +3,7 @@ "command": "ng completion", "shortDescription": "Set up Angular CLI autocompletion for your terminal.", "longDescriptionRelativePath": "@angular/cli/src/commands/completion/long-description.md", - "longDescription": "Setting up autocompletion configures your terminal, so pressing the `` key while in the middle\nof typing will display various commands and options available to you. This makes it very easy to\ndiscover and use CLI commands without lots of memorization.\n\n![A demo of Angular CLI autocompletion in a terminal. The user types several partial `ng` commands,\nusing autocompletion to finish several arguments and list contextual options.\n](assets/images/guide/cli/completion.gif)\n\n## Automated setup\n\nThe CLI should prompt and ask to set up autocompletion for you the first time you use it (v14+).\nSimply answer \"Yes\" and the CLI will take care of the rest.\n\n```\n$ ng serve\n? Would you like to enable autocompletion? This will set up your terminal so pressing TAB while typing Angular CLI commands will show possible options and autocomplete arguments. (Enabling autocompletion will modify configuration files in your home directory.) Yes\nAppended `source <(ng completion script)` to `/home/my-username/.bashrc`. Restart your terminal or run:\n\nsource <(ng completion script)\n\nto autocomplete `ng` commands.\n\n# Serve output...\n```\n\nIf you already refused the prompt, it won't ask again. But you can run `ng completion` to\ndo the same thing automatically.\n\nThis modifies your terminal environment to load Angular CLI autocompletion, but can't update your\ncurrent terminal session. Either restart it or run `source <(ng completion script)` directly to\nenable autocompletion in your current session.\n\nTest it out by typing `ng ser` and it should autocomplete to `ng serve`. Ambiguous arguments\nwill show all possible options and their documentation, such as `ng generate `.\n\n## Manual setup\n\nSome users may have highly customized terminal setups, possibly with configuration files checked\ninto source control with an opinionated structure. `ng completion` only ever appends Angular's setup\nto an existing configuration file for your current shell, or creates one if none exists. If you want\nmore control over exactly where this configuration lives, you can manually set it up by having your\nshell run at startup:\n\n```bash\nsource <(ng completion script)\n```\n\nThis is equivalent to what `ng completion` will automatically set up, and gives power users more\nflexibility in their environments when desired.\n\n## Platform support\n\nAngular CLI supports autocompletion for the Bash and Zsh shells on MacOS and Linux operating\nsystems. On Windows, Git Bash and [Windows Subsystem for Linux](https://docs.microsoft.com/windows/wsl/)\nusing Bash or Zsh are supported.\n\n## Global install\n\nAutocompletion works by configuring your terminal to invoke the Angular CLI on startup to load the\nsetup script. This means the terminal must be able to find and execute the Angular CLI, typically\nthrough a global install that places the binary on the user's `$PATH`. If you get\n`command not found: ng`, make sure the CLI is installed globally which you can do with the `-g`\nflag:\n\n```bash\nnpm install -g @angular/cli\n```\n", + "longDescription": "Setting up autocompletion configures your terminal, so pressing the `` key while in the middle\nof typing will display various commands and options available to you. This makes it very easy to\ndiscover and use CLI commands without lots of memorization.\n\n![A demo of Angular CLI autocompletion in a terminal. The user types several partial `ng` commands,\nusing autocompletion to finish several arguments and list contextual options.\n](assets/images/guide/cli/completion.gif)\n\n## Automated setup\n\nThe CLI should prompt and ask to set up autocompletion for you the first time you use it (v14+).\nSimply answer \"Yes\" and the CLI will take care of the rest.\n\n```\n$ ng serve\n? Would you like to enable autocompletion? This will set up your terminal so pressing TAB while typing Angular CLI commands will show possible options and autocomplete arguments. (Enabling autocompletion will modify configuration files in your home directory.) Yes\nAppended `source <(ng completion script)` to `/home/my-username/.bashrc`. Restart your terminal or run:\n\nsource <(ng completion script)\n\nto autocomplete `ng` commands.\n\n# Serve output...\n```\n\nIf you already refused the prompt, it won't ask again. But you can run `ng completion` to\ndo the same thing automatically.\n\nThis modifies your terminal environment to load Angular CLI autocompletion, but can't update your\ncurrent terminal session. Either restart it or run `source <(ng completion script)` directly to\nenable autocompletion in your current session.\n\nTest it out by typing `ng ser` and it should autocomplete to `ng serve`. Ambiguous arguments\nwill show all possible options and their documentation, such as `ng generate `.\n\n## Manual setup\n\nSome users may have highly customized terminal setups, possibly with configuration files checked\ninto source control with an opinionated structure. `ng completion` only ever appends Angular's setup\nto an existing configuration file for your current shell, or creates one if none exists. If you want\nmore control over exactly where this configuration lives, you can manually set it up by having your\nshell run at startup:\n\n```bash\nsource <(ng completion script)\n```\n\nThis is equivalent to what `ng completion` will automatically set up, and gives power users more\nflexibility in their environments when desired.\n\n## Platform support\n\nAngular CLI supports autocompletion for the Bash and Zsh shells on MacOS and Linux operating\nsystems. On Windows, Git Bash and [Windows Subsystem for Linux](https://docs.microsoft.com/en-us/windows/wsl/)\nusing Bash or Zsh are supported.\n\n## Global install\n\nAutocompletion works by configuring your terminal to invoke the Angular CLI on startup to load the\nsetup script. This means the terminal must be able to find and execute the Angular CLI, typically\nthrough a global install that places the binary on the user's `$PATH`. If you get\n`command not found: ng`, make sure the CLI is installed globally which you can do with the `-g`\nflag:\n\n```bash\nnpm install -g @angular/cli\n```\n", "aliases": [], "deprecated": false, "options": [ diff --git a/adev/src/content/cli/help/extract-i18n.json b/adev/src/content/cli/help/extract-i18n.json index 617aacb3d2a29..e1426d0d06153 100644 --- a/adev/src/content/cli/help/extract-i18n.json +++ b/adev/src/content/cli/help/extract-i18n.json @@ -5,12 +5,6 @@ "aliases": [], "deprecated": false, "options": [ - { - "name": "browser-target", - "type": "string", - "deprecated": "Use 'buildTarget' instead.", - "description": "A browser builder target to extract i18n messages in the format of `project:target[:configuration]`. You can also pass in more than one configuration name as a comma-separated list. Example: `project:target:production,staging`." - }, { "name": "build-target", "type": "string", diff --git a/adev/src/content/cli/help/generate.json b/adev/src/content/cli/help/generate.json index a6d62df446341..74ce78d3e5c2a 100644 --- a/adev/src/content/cli/help/generate.json +++ b/adev/src/content/cli/help/generate.json @@ -247,6 +247,12 @@ "default": false, "description": "The declaring NgModule exports this component." }, + { + "name": "export-default", + "type": "boolean", + "default": false, + "description": "Use default export for the component instead of a named export." + }, { "name": "flat", "type": "boolean", diff --git a/adev/src/content/cli/help/serve.json b/adev/src/content/cli/help/serve.json index 347e172e67af0..14b5774994387 100644 --- a/adev/src/content/cli/help/serve.json +++ b/adev/src/content/cli/help/serve.json @@ -13,12 +13,6 @@ "type": "array", "description": "List of hosts that are allowed to access the dev server. This option has no effect when using the 'application' or other esbuild-based builders." }, - { - "name": "browser-target", - "type": "string", - "deprecated": "Use 'buildTarget' instead.", - "description": "A browser builder target to serve in the format of `project:target[:configuration]`. You can also pass in more than one configuration name as a comma-separated list. Example: `project:target:production,staging`." - }, { "name": "build-target", "type": "string", From 969dadc8e2fad8ca9d892858bdadbe3abb13de95 Mon Sep 17 00:00:00 2001 From: realstealthninja <68815218+realstealthninja@users.noreply.github.com> Date: Mon, 26 Aug 2024 09:28:27 +0530 Subject: [PATCH 06/36] docs: fix typo in recommendations.ts (#57521) PR Close #57521 --- adev/src/app/features/update/recommendations.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adev/src/app/features/update/recommendations.ts b/adev/src/app/features/update/recommendations.ts index 73e178b1b57f9..6ab86d3af40a7 100644 --- a/adev/src/app/features/update/recommendations.ts +++ b/adev/src/app/features/update/recommendations.ts @@ -2094,7 +2094,7 @@ export const RECOMMENDATIONS: Step[] = [ level: ApplicationComplexity.Basic, step: 'v17 zone.js support', action: - 'Make sure that you are using a supported version of Zone.js before you upgrade your application. Angular v16 supports Zone.js version 0.14.x or later.', + 'Make sure that you are using a supported version of Zone.js before you upgrade your application. Angular v17 supports Zone.js version 0.14.x or later.', }, { possibleIn: 1700, From bfda774995235d0d05990de56a975be99dd259d4 Mon Sep 17 00:00:00 2001 From: AleksanderBodurri Date: Mon, 6 May 2024 19:06:31 -0400 Subject: [PATCH 07/36] fix(devtools): catch invalidated extension error to prevent devtools from spamming console (#55697) When a browser extension is updated it becomes invalidated on currently open pages. If that extension then tries to send a message to those pages through `chrome.runtime.sendMessage(..)` then an error is thrown in the console For Angular DevTools, this results in spamming the console with "Uncaught Error: Extension context invalidated." errors. This commit catches that error and removes the event listener that triggers the `chrome.runtime.sendMessage(...)` call. PR Close #55697 --- .../shell-browser/src/app/content-script.ts | 19 +++++++++++++++++++ .../shell-browser/src/app/ng-validate.ts | 6 ------ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/devtools/projects/shell-browser/src/app/content-script.ts b/devtools/projects/shell-browser/src/app/content-script.ts index 8b168fbb933fa..86271521ebd11 100644 --- a/devtools/projects/shell-browser/src/app/content-script.ts +++ b/devtools/projects/shell-browser/src/app/content-script.ts @@ -90,3 +90,22 @@ if (!backendInitialized) { }; retry(); } + +const proxyEventFromWindowToDevToolsExtension = (event: MessageEvent) => { + if (event.source === window && event.data) { + try { + chrome.runtime.sendMessage(event.data); + } catch (e) { + const {message} = e as Error; + if (message.includes('Extension context invalidated.')) { + console.error( + 'Angular DevTools: Disconnecting content script due to invalid extension context. Please reload the page.', + ); + window.removeEventListener('message', proxyEventFromWindowToDevToolsExtension); + } + throw e; + } + } +}; + +window.addEventListener('message', proxyEventFromWindowToDevToolsExtension); diff --git a/devtools/projects/shell-browser/src/app/ng-validate.ts b/devtools/projects/shell-browser/src/app/ng-validate.ts index 6d41ba06c4bc7..178e360120afe 100644 --- a/devtools/projects/shell-browser/src/app/ng-validate.ts +++ b/devtools/projects/shell-browser/src/app/ng-validate.ts @@ -8,12 +8,6 @@ /// -window.addEventListener('message', (event: MessageEvent) => { - if (event.source === window && event.data) { - chrome.runtime.sendMessage(event.data); - } -}); - if (document.contentType === 'text/html') { const script = document.createElement('script'); script.src = chrome.runtime.getURL('app/detect_angular_for_extension_icon_bundle.js'); From 564a8d587148f39dfdde58d5c6d5a4c921d116c4 Mon Sep 17 00:00:00 2001 From: Michael van der Luit <3525053+mvdluit@users.noreply.github.com> Date: Sat, 20 Jul 2024 21:04:53 +0200 Subject: [PATCH 08/36] docs(docs-infra): Add query as an url query param to the api reference (#57062) add an url query param to perform a search on api references directly via the url PR Close #57062 --- adev/src/app/app-scroller.ts | 7 +++++ .../api-items-section.component.html | 2 +- .../api-reference-list.component.spec.ts | 27 +++++++++++++++++ .../api-reference-list.component.ts | 29 +++++++++++++++++-- 4 files changed, 61 insertions(+), 4 deletions(-) diff --git a/adev/src/app/app-scroller.ts b/adev/src/app/app-scroller.ts index 9346b19b5488f..2655994e9a829 100644 --- a/adev/src/app/app-scroller.ts +++ b/adev/src/app/app-scroller.ts @@ -41,6 +41,13 @@ export class AppScroller { this._lastScrollEvent = e; }), filter(() => !this.disableScrolling), + filter(() => { + const info = this.router.lastSuccessfulNavigation?.extras.info as Record< + 'disableScrolling', + boolean + >; + return !info?.['disableScrolling']; + }), switchMap((e) => { return firstValueFrom( this.appRef.isStable.pipe( diff --git a/adev/src/app/features/references/api-items-section/api-items-section.component.html b/adev/src/app/features/references/api-items-section/api-items-section.component.html index fad7762997bce..0e951948b5d3c 100644 --- a/adev/src/app/features/references/api-items-section/api-items-section.component.html +++ b/adev/src/app/features/references/api-items-section/api-items-section.component.html @@ -3,7 +3,7 @@

@if (group.isFeatured) { star } - {{ group.title }} + {{ group.title }}

diff --git a/adev/src/app/features/references/api-reference-list/api-reference-list.component.spec.ts b/adev/src/app/features/references/api-reference-list/api-reference-list.component.spec.ts index 24c195056d07f..5e5c42d041dfd 100644 --- a/adev/src/app/features/references/api-reference-list/api-reference-list.component.spec.ts +++ b/adev/src/app/features/references/api-reference-list/api-reference-list.component.spec.ts @@ -14,6 +14,7 @@ import {signal} from '@angular/core'; import {ApiItemType} from '../interfaces/api-item-type'; import {RouterTestingHarness} from '@angular/router/testing'; import {provideRouter} from '@angular/router'; +import {Location} from '@angular/common'; describe('ApiReferenceList', () => { let component: ApiReferenceList; @@ -117,4 +118,30 @@ describe('ApiReferenceList', () => { harness.navigateByUrl(`/api`); expect(component.type()).toBe(ALL_STATUSES_KEY); }); + + it('should set the value of the queryParam equal to the query value', async () => { + const location = TestBed.inject(Location); + component.query.set('item1'); + await fixture.whenStable(); + expect(location.path()).toBe(`?query=item1&type=All`); + }); + + it('should keep the values of existing queryParams and set new queryParam equal to the type', async () => { + const location = TestBed.inject(Location); + + component.query.set('item1'); + await fixture.whenStable(); + expect(location.path()).toBe(`?query=item1&type=All`); + + component.filterByItemType(ApiItemType.BLOCK); + await fixture.whenStable(); + expect(location.path()).toBe(`?query=item1&type=${ApiItemType.BLOCK}`); + }); + + it('should display all items when query and type are undefined', async () => { + component.query.set(undefined); + component.type.set(undefined); + await fixture.whenStable(); + expect(component.filteredGroups()![0].items).toEqual([fakeItem1, fakeItem2]); + }); }); diff --git a/adev/src/app/features/references/api-reference-list/api-reference-list.component.ts b/adev/src/app/features/references/api-reference-list/api-reference-list.component.ts index 30ff3d1e28348..5320b4251f19a 100644 --- a/adev/src/app/features/references/api-reference-list/api-reference-list.component.ts +++ b/adev/src/app/features/references/api-reference-list/api-reference-list.component.ts @@ -23,6 +23,7 @@ import ApiItemsSection from '../api-items-section/api-items-section.component'; import {FormsModule} from '@angular/forms'; import {SlideToggle, TextField} from '@angular/docs'; import {NgFor, NgIf} from '@angular/common'; +import {Params, Router} from '@angular/router'; import {ApiItemType} from '../interfaces/api-item-type'; import {ApiReferenceManager} from './api-reference-manager.service'; import ApiItemLabel from '../api-item-label/api-item-label.component'; @@ -50,6 +51,7 @@ export const ALL_STATUSES_KEY = 'All'; }) export default class ApiReferenceList { private readonly apiReferenceManager = inject(ApiReferenceManager); + private readonly router = inject(Router); filterInput = viewChild.required(TextField, {read: ElementRef}); private readonly injector = inject(EnvironmentInjector); @@ -71,9 +73,28 @@ export default class ApiReferenceList { {injector: this.injector}, ); }); + + effect( + () => { + const params: Params = { + 'query': this.query() ? this.query() : null, + 'type': this.type() ? this.type() : null, + }; + + this.router.navigate([], { + queryParams: params, + replaceUrl: true, + preserveFragment: true, + info: { + disableScrolling: true, + }, + }); + }, + {allowSignalWrites: true}, + ); } - query = signal(''); + query = model(''); includeDeprecated = signal(false); type = model(ALL_STATUSES_KEY); @@ -87,8 +108,10 @@ export default class ApiReferenceList { id: group.id, items: group.items.filter((apiItem) => { return ( - (this.query() - ? apiItem.title.toLocaleLowerCase().includes(this.query().toLocaleLowerCase()) + (this.query() !== undefined + ? apiItem.title + .toLocaleLowerCase() + .includes((this.query() as string).toLocaleLowerCase()) : true) && (this.includeDeprecated() ? true : apiItem.isDeprecated === this.includeDeprecated()) && (this.type() === undefined || From 122af30373066e5b407f03bb6860e7b858e090a0 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 15 Aug 2024 10:28:48 -0700 Subject: [PATCH 09/36] refactor(core): Merge autoDetectChanges behaviors between fixtures (#57416) This commit updates the implementations of `autoDetectChanges` to be shared between the zone-based and zoneless fixtures. This now allows `autoDetect` to be turned off for zoneless fixtures after it was previously on because the host view is no longer directly attached to `ApplicationRef`. PR Close #57416 --- .../core/src/application/application_ref.ts | 2 - .../core/testing/src/component_fixture.ts | 67 ++++++------------- 2 files changed, 21 insertions(+), 48 deletions(-) diff --git a/packages/core/src/application/application_ref.ts b/packages/core/src/application/application_ref.ts index 52412b9ba4a49..76edf61827521 100644 --- a/packages/core/src/application/application_ref.ts +++ b/packages/core/src/application/application_ref.ts @@ -331,7 +331,6 @@ export class ApplicationRef { // Needed for ComponentFixture temporarily during migration of autoDetect behavior // Eventually the hostView of the fixture should just attach to ApplicationRef. private externalTestViews: Set> = new Set(); - private beforeRender = new Subject(); /** @internal */ afterTick = new Subject(); /** @internal */ @@ -662,7 +661,6 @@ export class ApplicationRef { this.dirtyFlags |= ApplicationRefDirtyFlags.AfterRender; // Check all potentially dirty views. - this.beforeRender.next(useGlobalCheck); for (let {_lView, notifyErrorHandler} of this.allViews) { detectChangesInViewIfRequired( _lView, diff --git a/packages/core/testing/src/component_fixture.ts b/packages/core/testing/src/component_fixture.ts index f46839b44976d..0d56b8947e9c4 100644 --- a/packages/core/testing/src/component_fixture.ts +++ b/packages/core/testing/src/component_fixture.ts @@ -23,9 +23,10 @@ import { ɵEffectScheduler as EffectScheduler, ɵgetDeferBlocks as getDeferBlocks, ɵNoopNgZone as NoopNgZone, + ɵZONELESS_ENABLED as ZONELESS_ENABLED, ɵPendingTasks as PendingTasks, } from '@angular/core'; -import {Subject, Subscription} from 'rxjs'; +import {Subscription} from 'rxjs'; import {DeferBlockFixture} from './defer'; import {ComponentFixtureAutoDetect, ComponentFixtureNoNgZone} from './test_bed_common'; @@ -85,6 +86,7 @@ export abstract class ComponentFixture { /** @internal */ protected abstract _autoDetect: boolean; private readonly scheduler = inject(ɵChangeDetectionScheduler, {optional: true}); + private readonly zonelessEnabled = inject(ZONELESS_ENABLED); // TODO(atscott): Remove this from public API ngZone = this._noZoneOptionIsSet ? null : this._ngZone; @@ -104,6 +106,7 @@ export abstract class ComponentFixture { if (this._autoDetect) { this._testAppRef.externalTestViews.add(this.componentRef.hostView); this.scheduler?.notify(ɵNotificationSource.ViewAttached); + this.scheduler?.notify(ɵNotificationSource.MarkAncestorsForTraversal); } this.componentRef.hostView.onDestroy(() => { this._testAppRef.externalTestViews.delete(this.componentRef.hostView); @@ -127,7 +130,22 @@ export abstract class ComponentFixture { * * Also runs detectChanges once so that any existing change is detected. */ - abstract autoDetectChanges(autoDetect?: boolean): void; + autoDetectChanges(autoDetect = true): void { + if (this._noZoneOptionIsSet && !this.zonelessEnabled) { + throw new Error('Cannot call autoDetectChanges when ComponentFixtureNoNgZone is set.'); + } + + if (autoDetect !== this._autoDetect) { + if (autoDetect) { + this._testAppRef.externalTestViews.add(this.componentRef.hostView); + } else { + this._testAppRef.externalTestViews.delete(this.componentRef.hostView); + } + } + + this._autoDetect = autoDetect; + this.detectChanges(); + } /** * Return whether the fixture is currently stable or has async tasks that have not been completed @@ -213,13 +231,6 @@ export class ScheduledComponentFixture extends ComponentFixture { /** @internal */ protected override _autoDetect = inject(ComponentFixtureAutoDetect, {optional: true}) ?? true; - override initialize(): void { - super.initialize(); - if (this._autoDetect) { - this._appRef.attachView(this.componentRef.hostView); - } - } - override detectChanges(checkNoChanges = true): void { if (!checkNoChanges) { throw new Error( @@ -231,19 +242,6 @@ export class ScheduledComponentFixture extends ComponentFixture { this._appRef.tick(); this._effectRunner.flush(); } - - override autoDetectChanges(autoDetect = true): void { - if (!autoDetect) { - throw new Error( - 'Cannot disable autoDetect after it has been enabled when using the zoneless scheduler. ' + - 'To disable autoDetect, add `{provide: ComponentFixtureAutoDetect, useValue: false}` to the TestBed providers.', - ); - } else if (!this._autoDetect) { - this._autoDetect = autoDetect; - this._appRef.attachView(this.componentRef.hostView); - } - this.detectChanges(); - } } interface TestAppRef { @@ -259,13 +257,7 @@ export class PseudoApplicationComponentFixture extends ComponentFixture { override _autoDetect = inject(ComponentFixtureAutoDetect, {optional: true}) ?? false; override initialize(): void { - if (this._autoDetect) { - this._testAppRef.externalTestViews.add(this.componentRef.hostView); - } - this.componentRef.hostView.onDestroy(() => { - this._testAppRef.externalTestViews.delete(this.componentRef.hostView); - }); - + super.initialize(); // Create subscriptions outside the NgZone so that the callbacks run outside // of NgZone. this._ngZone.runOutsideAngular(() => { @@ -294,23 +286,6 @@ export class PseudoApplicationComponentFixture extends ComponentFixture { this._effectRunner.flush(); } - override autoDetectChanges(autoDetect = true): void { - if (this._noZoneOptionIsSet) { - throw new Error('Cannot call autoDetectChanges when ComponentFixtureNoNgZone is set.'); - } - - if (autoDetect !== this._autoDetect) { - if (autoDetect) { - this._testAppRef.externalTestViews.add(this.componentRef.hostView); - } else { - this._testAppRef.externalTestViews.delete(this.componentRef.hostView); - } - } - - this._autoDetect = autoDetect; - this.detectChanges(); - } - override destroy(): void { this._subscriptions.unsubscribe(); super.destroy(); From 4e594e175dfddfe06b52702fd88fe5c002379c18 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 15 Aug 2024 11:42:00 -0700 Subject: [PATCH 10/36] refactor(core): Move ngZone subscription to base implementation (#57416) This commit moves the ngZone onError subscription to the base fixture implementation. While this subscription isn't necessary for zoneless, it does no harm because the observable never emits. PR Close #57416 --- .../core/testing/src/component_fixture.ts | 45 ++++++++----------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/packages/core/testing/src/component_fixture.ts b/packages/core/testing/src/component_fixture.ts index 0d56b8947e9c4..28072d44d78ef 100644 --- a/packages/core/testing/src/component_fixture.ts +++ b/packages/core/testing/src/component_fixture.ts @@ -32,6 +32,10 @@ import {DeferBlockFixture} from './defer'; import {ComponentFixtureAutoDetect, ComponentFixtureNoNgZone} from './test_bed_common'; import {TestBedApplicationErrorHandler} from './application_error_handler'; +interface TestAppRef { + externalTestViews: Set; +} + /** * Fixture for debugging and testing a component. * @@ -79,14 +83,14 @@ export abstract class ComponentFixture { // behavior. /** @internal */ protected readonly _appRef = inject(ApplicationRef); - /** @internal */ - protected readonly _testAppRef = this._appRef as unknown as TestAppRef; + private readonly _testAppRef = this._appRef as unknown as TestAppRef; private readonly pendingTasks = inject(PendingTasks); private readonly appErrorHandler = inject(TestBedApplicationErrorHandler); /** @internal */ protected abstract _autoDetect: boolean; private readonly scheduler = inject(ɵChangeDetectionScheduler, {optional: true}); private readonly zonelessEnabled = inject(ZONELESS_ENABLED); + private subscriptions = new Subscription(); // TODO(atscott): Remove this from public API ngZone = this._noZoneOptionIsSet ? null : this._ngZone; @@ -111,6 +115,17 @@ export abstract class ComponentFixture { this.componentRef.hostView.onDestroy(() => { this._testAppRef.externalTestViews.delete(this.componentRef.hostView); }); + // Create subscriptions outside the NgZone so that the callbacks run outside + // of NgZone. + this._ngZone.runOutsideAngular(() => { + this.subscriptions.add( + this._ngZone.onError.subscribe({ + next: (error: any) => { + throw error; + }, + }), + ); + }); } /** @@ -213,6 +228,7 @@ export abstract class ComponentFixture { * Trigger component destruction. */ destroy(): void { + this.subscriptions.unsubscribe(); this._testAppRef.externalTestViews.delete(this.componentRef.hostView); if (!this._isDestroyed) { this.componentRef.destroy(); @@ -244,33 +260,13 @@ export class ScheduledComponentFixture extends ComponentFixture { } } -interface TestAppRef { - externalTestViews: Set; -} - /** * ComponentFixture behavior that attempts to act as a "mini application". */ export class PseudoApplicationComponentFixture extends ComponentFixture { - private _subscriptions = new Subscription(); /** @internal */ override _autoDetect = inject(ComponentFixtureAutoDetect, {optional: true}) ?? false; - override initialize(): void { - super.initialize(); - // Create subscriptions outside the NgZone so that the callbacks run outside - // of NgZone. - this._ngZone.runOutsideAngular(() => { - this._subscriptions.add( - this._ngZone.onError.subscribe({ - next: (error: any) => { - throw error; - }, - }), - ); - }); - } - override detectChanges(checkNoChanges = true): void { this._effectRunner.flush(); // Run the change detection inside the NgZone so that any async tasks as part of the change @@ -285,9 +281,4 @@ export class PseudoApplicationComponentFixture extends ComponentFixture { // dirty in response to input signals changing. this._effectRunner.flush(); } - - override destroy(): void { - this._subscriptions.unsubscribe(); - super.destroy(); - } } From 455cd1edc3ce4f4123ccbc24b6149c1721c584dd Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 15 Aug 2024 11:59:56 -0700 Subject: [PATCH 11/36] refactor(core): Merge implementations of ComponentFixture (#57416) This commit removes the abstract base class and two separate implementations of `ComponentFixture` for zone vs zoneless. Now that the behaviors have gotten close enough to the same, the diverged concrete implementations serve less value. Instead, the different behaviors can be easily handled in if/else branches. The difference is now limited to the default for `autoDetect` and how `detectChanges` functions. PR Close #57416 --- goldens/public-api/core/testing/index.api.md | 6 +- .../core/testing/src/component_fixture.ts | 91 +++++++------------ packages/core/testing/src/test_bed.ts | 16 +--- .../core/testing/src/test_bed_compiler.ts | 1 - 4 files changed, 39 insertions(+), 75 deletions(-) diff --git a/goldens/public-api/core/testing/index.api.md b/goldens/public-api/core/testing/index.api.md index efe115f889ae4..1695d9f083889 100644 --- a/goldens/public-api/core/testing/index.api.md +++ b/goldens/public-api/core/testing/index.api.md @@ -28,9 +28,9 @@ import { ɵDeferBlockDetails } from '@angular/core'; export const __core_private_testing_placeholder__ = ""; // @public -export abstract class ComponentFixture { +export class ComponentFixture { constructor(componentRef: ComponentRef); - abstract autoDetectChanges(autoDetect?: boolean): void; + autoDetectChanges(autoDetect?: boolean): void; changeDetectorRef: ChangeDetectorRef; checkNoChanges(): void; componentInstance: T; @@ -38,7 +38,7 @@ export abstract class ComponentFixture { componentRef: ComponentRef; debugElement: DebugElement; destroy(): void; - abstract detectChanges(checkNoChanges?: boolean): void; + detectChanges(checkNoChanges?: boolean): void; elementRef: ElementRef; getDeferBlocks(): Promise; isStable(): boolean; diff --git a/packages/core/testing/src/component_fixture.ts b/packages/core/testing/src/component_fixture.ts index 28072d44d78ef..48fb80bce798c 100644 --- a/packages/core/testing/src/component_fixture.ts +++ b/packages/core/testing/src/component_fixture.ts @@ -41,7 +41,7 @@ interface TestAppRef { * * @publicApi */ -export abstract class ComponentFixture { +export class ComponentFixture { /** * The DebugElement associated with the root element of this component. */ @@ -86,10 +86,12 @@ export abstract class ComponentFixture { private readonly _testAppRef = this._appRef as unknown as TestAppRef; private readonly pendingTasks = inject(PendingTasks); private readonly appErrorHandler = inject(TestBedApplicationErrorHandler); - /** @internal */ - protected abstract _autoDetect: boolean; - private readonly scheduler = inject(ɵChangeDetectionScheduler, {optional: true}); private readonly zonelessEnabled = inject(ZONELESS_ENABLED); + private readonly scheduler = inject(ɵChangeDetectionScheduler); + private readonly autoDetectDefault = this.zonelessEnabled ? true : false; + private autoDetect = + inject(ComponentFixtureAutoDetect, {optional: true}) ?? this.autoDetectDefault; + private subscriptions = new Subscription(); // TODO(atscott): Remove this from public API @@ -103,11 +105,8 @@ export abstract class ComponentFixture { this.componentInstance = componentRef.instance; this.nativeElement = this.elementRef.nativeElement; this.componentRef = componentRef; - } - /** @internal */ - initialize(): void { - if (this._autoDetect) { + if (this.autoDetect) { this._testAppRef.externalTestViews.add(this.componentRef.hostView); this.scheduler?.notify(ɵNotificationSource.ViewAttached); this.scheduler?.notify(ɵNotificationSource.MarkAncestorsForTraversal); @@ -131,7 +130,31 @@ export abstract class ComponentFixture { /** * Trigger a change detection cycle for the component. */ - abstract detectChanges(checkNoChanges?: boolean): void; + detectChanges(checkNoChanges = true): void { + if (this.zonelessEnabled && !checkNoChanges) { + throw new Error( + 'Cannot disable `checkNoChanges` in this configuration. ' + + 'Use `fixture.componentRef.hostView.changeDetectorRef.detectChanges()` instead.', + ); + } + + this._effectRunner.flush(); + if (this.zonelessEnabled) { + this._appRef.tick(); + } else { + // Run the change detection inside the NgZone so that any async tasks as part of the change + // detection are captured by the zone and can be waited for in isStable. + // Run any effects that were created/dirtied during change detection. Such effects might become + // dirty in response to input signals changing. + this._ngZone.run(() => { + this.changeDetectorRef.detectChanges(); + if (checkNoChanges) { + this.checkNoChanges(); + } + }); + } + this._effectRunner.flush(); + } /** * Do a change detection run to make sure there were no changes. @@ -150,7 +173,7 @@ export abstract class ComponentFixture { throw new Error('Cannot call autoDetectChanges when ComponentFixtureNoNgZone is set.'); } - if (autoDetect !== this._autoDetect) { + if (autoDetect !== this.autoDetect) { if (autoDetect) { this._testAppRef.externalTestViews.add(this.componentRef.hostView); } else { @@ -158,7 +181,7 @@ export abstract class ComponentFixture { } } - this._autoDetect = autoDetect; + this.autoDetect = autoDetect; this.detectChanges(); } @@ -236,49 +259,3 @@ export abstract class ComponentFixture { } } } - -/** - * ComponentFixture behavior that actually attaches the component to the application to ensure - * behaviors between fixture and application do not diverge. `detectChanges` is disabled by default - * (instead, tests should wait for the scheduler to detect changes), `whenStable` is directly the - * `ApplicationRef.isStable`, and `autoDetectChanges` cannot be disabled. - */ -export class ScheduledComponentFixture extends ComponentFixture { - /** @internal */ - protected override _autoDetect = inject(ComponentFixtureAutoDetect, {optional: true}) ?? true; - - override detectChanges(checkNoChanges = true): void { - if (!checkNoChanges) { - throw new Error( - 'Cannot disable `checkNoChanges` in this configuration. ' + - 'Use `fixture.componentRef.hostView.changeDetectorRef.detectChanges()` instead.', - ); - } - this._effectRunner.flush(); - this._appRef.tick(); - this._effectRunner.flush(); - } -} - -/** - * ComponentFixture behavior that attempts to act as a "mini application". - */ -export class PseudoApplicationComponentFixture extends ComponentFixture { - /** @internal */ - override _autoDetect = inject(ComponentFixtureAutoDetect, {optional: true}) ?? false; - - override detectChanges(checkNoChanges = true): void { - this._effectRunner.flush(); - // Run the change detection inside the NgZone so that any async tasks as part of the change - // detection are captured by the zone and can be waited for in isStable. - this._ngZone.run(() => { - this.changeDetectorRef.detectChanges(); - if (checkNoChanges) { - this.checkNoChanges(); - } - }); - // Run any effects that were created/dirtied during change detection. Such effects might become - // dirty in response to input signals changing. - this._effectRunner.flush(); - } -} diff --git a/packages/core/testing/src/test_bed.ts b/packages/core/testing/src/test_bed.ts index 0341377c08703..3b5a0abd14930 100644 --- a/packages/core/testing/src/test_bed.ts +++ b/packages/core/testing/src/test_bed.ts @@ -39,14 +39,9 @@ import { ɵsetUnknownElementStrictMode as setUnknownElementStrictMode, ɵsetUnknownPropertyStrictMode as setUnknownPropertyStrictMode, ɵstringify as stringify, - ɵZONELESS_ENABLED as ZONELESS_ENABLED, } from '@angular/core'; -import { - ComponentFixture, - PseudoApplicationComponentFixture, - ScheduledComponentFixture, -} from './component_fixture'; +import {ComponentFixture} from './component_fixture'; import {MetadataOverride} from './metadata_override'; import { ComponentFixtureNoNgZone, @@ -699,14 +694,7 @@ export class TestBedImpl implements TestBed { `#${rootElId}`, this.testModuleRef, ) as ComponentRef; - return this.runInInjectionContext(() => { - const isZoneless = this.inject(ZONELESS_ENABLED); - const fixture = isZoneless - ? new ScheduledComponentFixture(componentRef) - : new PseudoApplicationComponentFixture(componentRef); - fixture.initialize(); - return fixture; - }); + return this.runInInjectionContext(() => new ComponentFixture(componentRef)); }; const noNgZone = this.inject(ComponentFixtureNoNgZone, false); const ngZone = noNgZone ? null : this.inject(NgZone, null); diff --git a/packages/core/testing/src/test_bed_compiler.ts b/packages/core/testing/src/test_bed_compiler.ts index 297465799f2bc..9928768396265 100644 --- a/packages/core/testing/src/test_bed_compiler.ts +++ b/packages/core/testing/src/test_bed_compiler.ts @@ -51,7 +51,6 @@ import { ɵNG_INJ_DEF as NG_INJ_DEF, ɵNG_MOD_DEF as NG_MOD_DEF, ɵNG_PIPE_DEF as NG_PIPE_DEF, - ɵZONELESS_ENABLED as ZONELESS_ENABLED, ɵNgModuleFactory as R3NgModuleFactory, ɵNgModuleTransitiveScopes as NgModuleTransitiveScopes, ɵNgModuleType as NgModuleType, From 6a7c5ae9583991be89836c09d6cada3bb91f3e01 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Thu, 15 Aug 2024 13:06:48 -0700 Subject: [PATCH 12/36] refactor(core): allow disabling checkNoChanges with zoneless (#57416) Disabling `checkNoChanges` in `ComponentFixture.detectChanges` was an error for the zoneless fixture since it was not yet working. This now allows checkNoChanges to be disabled. This option isn't really used/shouldn't be used by anyone except the FW so marked as a refactor. PR Close #57416 --- packages/core/test/component_fixture_spec.ts | 18 +++++++++ .../core/testing/src/component_fixture.ts | 39 ++++++++++--------- 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/packages/core/test/component_fixture_spec.ts b/packages/core/test/component_fixture_spec.ts index 805835cc5a7b2..cc20cca2139c5 100644 --- a/packages/core/test/component_fixture_spec.ts +++ b/packages/core/test/component_fixture_spec.ts @@ -563,4 +563,22 @@ describe('ComponentFixture with zoneless', () => { const fixture = TestBed.createComponent(App); await expectAsync(fixture.whenStable()).toBeRejected(); }); + + it('can disable checkNoChanges', () => { + @Component({ + template: '{{thing}}', + standalone: true, + }) + class App { + thing = 1; + ngAfterViewChecked() { + ++this.thing; + } + } + + const fixture = TestBed.createComponent(App); + expect(() => fixture.detectChanges(false /*checkNoChanges*/)).not.toThrow(); + // still throws if checkNoChanges is not disabled + expect(() => fixture.detectChanges()).toThrowError(/ExpressionChanged/); + }); }); diff --git a/packages/core/testing/src/component_fixture.ts b/packages/core/testing/src/component_fixture.ts index 48fb80bce798c..c9122f6ca359a 100644 --- a/packages/core/testing/src/component_fixture.ts +++ b/packages/core/testing/src/component_fixture.ts @@ -34,6 +34,7 @@ import {TestBedApplicationErrorHandler} from './application_error_handler'; interface TestAppRef { externalTestViews: Set; + skipCheckNoChangesForExternalTestViews: Set; } /** @@ -131,27 +132,27 @@ export class ComponentFixture { * Trigger a change detection cycle for the component. */ detectChanges(checkNoChanges = true): void { - if (this.zonelessEnabled && !checkNoChanges) { - throw new Error( - 'Cannot disable `checkNoChanges` in this configuration. ' + - 'Use `fixture.componentRef.hostView.changeDetectorRef.detectChanges()` instead.', - ); - } - this._effectRunner.flush(); - if (this.zonelessEnabled) { - this._appRef.tick(); - } else { - // Run the change detection inside the NgZone so that any async tasks as part of the change - // detection are captured by the zone and can be waited for in isStable. - // Run any effects that were created/dirtied during change detection. Such effects might become - // dirty in response to input signals changing. - this._ngZone.run(() => { - this.changeDetectorRef.detectChanges(); - if (checkNoChanges) { + const originalCheckNoChanges = this.componentRef.changeDetectorRef.checkNoChanges; + try { + if (!checkNoChanges) { + this.componentRef.changeDetectorRef.checkNoChanges = () => {}; + } + + if (this.zonelessEnabled) { + this._appRef.tick(); + } else { + // Run the change detection inside the NgZone so that any async tasks as part of the change + // detection are captured by the zone and can be waited for in isStable. + // Run any effects that were created/dirtied during change detection. Such effects might become + // dirty in response to input signals changing. + this._ngZone.run(() => { + this.changeDetectorRef.detectChanges(); this.checkNoChanges(); - } - }); + }); + } + } finally { + this.componentRef.changeDetectorRef.checkNoChanges = originalCheckNoChanges; } this._effectRunner.flush(); } From 0300dd2e18f064f2f57f7371e0dc5c01218b5019 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Mon, 26 Aug 2024 11:25:16 -0700 Subject: [PATCH 13/36] fix(core): Fix fixture.detectChanges with autoDetect disabled and zoneless (#57416) When disabling autodetect (not recommeneded) with zoneless, `fixture.detectChanges` would previously not refresh the fixture's component. PR Close #57416 --- packages/core/test/component_fixture_spec.ts | 16 ++++++++++++++++ packages/core/testing/src/component_fixture.ts | 9 ++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/core/test/component_fixture_spec.ts b/packages/core/test/component_fixture_spec.ts index cc20cca2139c5..f7f206c78dba2 100644 --- a/packages/core/test/component_fixture_spec.ts +++ b/packages/core/test/component_fixture_spec.ts @@ -581,4 +581,20 @@ describe('ComponentFixture with zoneless', () => { // still throws if checkNoChanges is not disabled expect(() => fixture.detectChanges()).toThrowError(/ExpressionChanged/); }); + + it('runs change detection when autoDetect is false', () => { + @Component({ + template: '{{thing()}}', + standalone: true, + }) + class App { + thing = signal(1); + } + + const fixture = TestBed.createComponent(App); + fixture.autoDetectChanges(false); + fixture.componentInstance.thing.set(2); + fixture.detectChanges(); + expect(fixture.nativeElement.innerText).toBe('2'); + }); }); diff --git a/packages/core/testing/src/component_fixture.ts b/packages/core/testing/src/component_fixture.ts index c9122f6ca359a..9e9767ef3dc17 100644 --- a/packages/core/testing/src/component_fixture.ts +++ b/packages/core/testing/src/component_fixture.ts @@ -140,7 +140,14 @@ export class ComponentFixture { } if (this.zonelessEnabled) { - this._appRef.tick(); + try { + this._testAppRef.externalTestViews.add(this.componentRef.hostView); + this._appRef.tick(); + } finally { + if (!this.autoDetect) { + this._testAppRef.externalTestViews.delete(this.componentRef.hostView); + } + } } else { // Run the change detection inside the NgZone so that any async tasks as part of the change // detection are captured by the zone and can be waited for in isStable. From dab722f9c899d775bb886a1d5beb5970f79c4cd8 Mon Sep 17 00:00:00 2001 From: Doug Parker Date: Mon, 17 Jun 2024 14:44:46 -0700 Subject: [PATCH 14/36] refactor(compiler): add `i18nPreserveWhitespaceForLegacyExtraction` (#56507) This configures whether or not to preserve whitespace content when extracting messages from Angular templates in the legacy (View Engine) extraction pipeline. This includes several bug fixes which unfortunately cannot be landed without changing message IDs in a breaking fashion and are necessary to properly trim whitespace. Instead these bug fixes are included only when the new flag is disabled. PR Close #56507 --- .../compiler-cli/compiler_options.api.md | 1 + .../annotations/component/src/handler.ts | 4 + .../annotations/component/src/resources.ts | 9 +- .../component/test/component_spec.ts | 1 + .../src/ngtsc/core/api/src/public_options.ts | 8 + .../src/ngtsc/core/src/compiler.ts | 1 + packages/compiler-cli/src/ngtsc/program.ts | 8 +- .../compiler-cli/src/transformers/i18n.ts | 8 +- packages/compiler/src/i18n/digest.ts | 25 +- .../compiler/src/i18n/extractor_merger.ts | 8 +- .../compiler/src/i18n/i18n_html_parser.ts | 2 +- packages/compiler/src/i18n/i18n_parser.ts | 42 +- packages/compiler/src/i18n/message_bundle.ts | 20 +- .../compiler/src/i18n/serializers/xliff2.ts | 2 +- packages/compiler/src/i18n/serializers/xmb.ts | 10 +- packages/compiler/src/i18n/serializers/xtb.ts | 2 +- .../src/ml_parser/html_whitespaces.ts | 145 +++++- .../compiler/src/render3/view/i18n/meta.ts | 75 ++- .../compiler/src/render3/view/template.ts | 31 +- .../test/i18n/extractor_merger_spec.ts | 5 + packages/compiler/test/i18n/i18n_ast_spec.ts | 1 + .../compiler/test/i18n/i18n_parser_spec.ts | 1 + .../test/i18n/integration_xmb_xtb_spec.ts | 4 +- .../test/i18n/serializers/xmb_spec.ts | 4 +- .../test/i18n/whitespace_sensitivity_spec.ts | 469 ++++++++++++++++++ .../test/ml_parser/html_whitespaces_spec.ts | 7 +- packages/compiler/test/render3/view/util.ts | 5 +- 27 files changed, 840 insertions(+), 58 deletions(-) create mode 100644 packages/compiler/test/i18n/whitespace_sensitivity_spec.ts diff --git a/goldens/public-api/compiler-cli/compiler_options.api.md b/goldens/public-api/compiler-cli/compiler_options.api.md index 8d61342f9b650..89cf36be0f375 100644 --- a/goldens/public-api/compiler-cli/compiler_options.api.md +++ b/goldens/public-api/compiler-cli/compiler_options.api.md @@ -38,6 +38,7 @@ export interface I18nOptions { i18nOutFile?: string; i18nOutFormat?: string; i18nOutLocale?: string; + i18nPreserveWhitespaceForLegacyExtraction?: boolean; i18nUseExternalIds?: boolean; } diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts index e0341a4c91a08..24b7c3b2456a1 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts @@ -251,6 +251,7 @@ export class ComponentDecoratorHandler private readonly enableLetSyntax: boolean, private readonly localCompilationExtraImportsTracker: LocalCompilationExtraImportsTracker | null, private readonly jitDeclarationRegistry: JitDeclarationRegistry, + private readonly i18nPreserveSignificantWhitespace: boolean, ) { this.extractTemplateOptions = { enableI18nLegacyMessageIdFormat: this.enableI18nLegacyMessageIdFormat, @@ -258,6 +259,7 @@ export class ComponentDecoratorHandler usePoisonedData: this.usePoisonedData, enableBlockSyntax: this.enableBlockSyntax, enableLetSyntax: this.enableLetSyntax, + preserveSignificantWhitespace: this.i18nPreserveSignificantWhitespace, }; } @@ -278,6 +280,7 @@ export class ComponentDecoratorHandler usePoisonedData: boolean; enableBlockSyntax: boolean; enableLetSyntax: boolean; + preserveSignificantWhitespace?: boolean; }; readonly precedence = HandlerPrecedence.PRIMARY; @@ -646,6 +649,7 @@ export class ComponentDecoratorHandler usePoisonedData: this.usePoisonedData, enableBlockSyntax: this.enableBlockSyntax, enableLetSyntax: this.enableLetSyntax, + preserveSignificantWhitespace: this.i18nPreserveSignificantWhitespace, }, this.compilationMode, ); diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/resources.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/resources.ts index 0ac502b37fdae..5b1875b78bfa6 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/resources.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/resources.ts @@ -135,6 +135,7 @@ export interface ExtractTemplateOptions { i18nNormalizeLineEndingsInICUs: boolean; enableBlockSyntax: boolean; enableLetSyntax: boolean; + preserveSignificantWhitespace?: boolean; } export function extractTemplate( @@ -277,15 +278,16 @@ function parseExtractedTemplate( const parsedTemplate = parseTemplate(sourceStr, sourceMapUrl ?? '', { ...commonParseOptions, preserveWhitespaces: template.preserveWhitespaces, + preserveSignificantWhitespace: options.preserveSignificantWhitespace, }); // Unfortunately, the primary parse of the template above may not contain accurate source map // information. If used directly, it would result in incorrect code locations in template // errors, etc. There are three main problems: // - // 1. `preserveWhitespaces: false` annihilates the correctness of template source mapping, as - // the whitespace transformation changes the contents of HTML text nodes before they're - // parsed into Angular expressions. + // 1. `preserveWhitespaces: false` or `preserveSignificantWhitespace: false` annihilates the + // correctness of template source mapping, as the whitespace transformation changes the + // contents of HTML text nodes before they're parsed into Angular expressions. // 2. `preserveLineEndings: false` causes growing misalignments in templates that use '\r\n' // line endings, by normalizing them to '\n'. // 3. By default, the template parser strips leading trivia characters (like spaces, tabs, and @@ -298,6 +300,7 @@ function parseExtractedTemplate( ...commonParseOptions, preserveWhitespaces: true, preserveLineEndings: true, + preserveSignificantWhitespace: true, leadingTriviaChars: [], }); diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/component/test/component_spec.ts index 389d13a2c9798..b2c8f4da5e437 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/test/component_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/test/component_spec.ts @@ -147,6 +147,7 @@ function setup( /* enableLetSyntax */ true, /* localCompilationExtraImportsTracker */ null, jitDeclarationRegistry, + /* i18nPreserveSignificantWhitespace */ true, ); return {reflectionHost, handler, resourceLoader, metaRegistry}; } diff --git a/packages/compiler-cli/src/ngtsc/core/api/src/public_options.ts b/packages/compiler-cli/src/ngtsc/core/api/src/public_options.ts index acd94044a60f5..8631e3bd83926 100644 --- a/packages/compiler-cli/src/ngtsc/core/api/src/public_options.ts +++ b/packages/compiler-cli/src/ngtsc/core/api/src/public_options.ts @@ -390,6 +390,14 @@ export interface I18nOptions { * The default is `false`, but this will be switched in a future major release. */ i18nNormalizeLineEndingsInICUs?: boolean; + + /** + * Whether or not to preserve whitespace when extracting messages with the legacy (View Engine) + * pipeline. + * + * Defaults to `true`. + */ + i18nPreserveWhitespaceForLegacyExtraction?: boolean; } /** diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 49a64d68b2abc..fb2575848a392 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -1446,6 +1446,7 @@ export class NgCompiler { this.enableLetSyntax, localCompilationExtraImportsTracker, jitDeclarationRegistry, + this.options.i18nPreserveWhitespaceForLegacyExtraction ?? true, ), // TODO(alxhub): understand why the cast here is necessary (something to do with `null` diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 81dfb6f006a50..ee250afdae60c 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -258,7 +258,13 @@ export class NgtscProgram implements api.Program { } private emitXi18n(): void { - const ctx = new MessageBundle(new HtmlParser(), [], {}, this.options.i18nOutLocale ?? null); + const ctx = new MessageBundle( + new HtmlParser(), + [], + {}, + this.options.i18nOutLocale ?? null, + this.options.i18nPreserveWhitespaceForLegacyExtraction, + ); this.compiler.xi18n(ctx); i18nExtract( this.options.i18nOutFormat ?? null, diff --git a/packages/compiler-cli/src/transformers/i18n.ts b/packages/compiler-cli/src/transformers/i18n.ts index cf2af710fd5a9..694f6747afb95 100644 --- a/packages/compiler-cli/src/transformers/i18n.ts +++ b/packages/compiler-cli/src/transformers/i18n.ts @@ -57,7 +57,13 @@ export function i18nSerialize( switch (format) { case 'xmb': - serializer = new Xmb(); + serializer = new Xmb( + // Whenever we disable whitespace preservation, we also want to stop preserving + // placeholders because they contain whitespace we want to drop too. Whitespace + // inside `{{ name }}` should be ignored for the same reasons as whitespace + // outside placeholders. + /* preservePlaceholders */ options.i18nPreserveWhitespaceForLegacyExtraction, + ); break; case 'xliff2': case 'xlf2': diff --git a/packages/compiler/src/i18n/digest.ts b/packages/compiler/src/i18n/digest.ts index 272df18771df1..55c62f6fe29af 100644 --- a/packages/compiler/src/i18n/digest.ts +++ b/packages/compiler/src/i18n/digest.ts @@ -32,15 +32,15 @@ export function computeDigest(message: i18n.Message): string { /** * Return the message id or compute it using the XLIFF2/XMB/$localize digest. */ -export function decimalDigest(message: i18n.Message): string { - return message.id || computeDecimalDigest(message); +export function decimalDigest(message: i18n.Message, preservePlaceholders: boolean): string { + return message.id || computeDecimalDigest(message, preservePlaceholders); } /** * Compute the message id using the XLIFF2/XMB/$localize digest. */ -export function computeDecimalDigest(message: i18n.Message): string { - const visitor = new _SerializerIgnoreIcuExpVisitor(); +export function computeDecimalDigest(message: i18n.Message, preservePlaceholders: boolean): string { + const visitor = new _SerializerIgnoreExpVisitor(preservePlaceholders); const parts = message.nodes.map((a) => a.visit(visitor, null)); return computeMsgId(parts.join(''), message.meaning); } @@ -100,12 +100,23 @@ export function serializeNodes(nodes: i18n.Node[]): string[] { /** * Serialize the i18n ast to something xml-like in order to generate an UID. * - * Ignore the ICU expressions so that message IDs stays identical if only the expression changes. + * Ignore the expressions so that message IDs stays identical if only the expression changes. * * @internal */ -class _SerializerIgnoreIcuExpVisitor extends _SerializerVisitor { - override visitIcu(icu: i18n.Icu, context: any): any { +class _SerializerIgnoreExpVisitor extends _SerializerVisitor { + constructor(private readonly preservePlaceholders: boolean) { + super(); + } + + override visitPlaceholder(ph: i18n.Placeholder, context: any): string { + // Do not take the expression into account when `preservePlaceholders` is disabled. + return this.preservePlaceholders + ? super.visitPlaceholder(ph, context) + : ``; + } + + override visitIcu(icu: i18n.Icu): string { let strCases = Object.keys(icu.cases).map((k: string) => `${k} {${icu.cases[k].visit(this)}}`); // Do not take the expression into account return `{${icu.type}, ${strCases.join(', ')}}`; diff --git a/packages/compiler/src/i18n/extractor_merger.ts b/packages/compiler/src/i18n/extractor_merger.ts index dcbf9a42f4b97..668b70da71a89 100644 --- a/packages/compiler/src/i18n/extractor_merger.ts +++ b/packages/compiler/src/i18n/extractor_merger.ts @@ -30,8 +30,9 @@ export function extractMessages( interpolationConfig: InterpolationConfig, implicitTags: string[], implicitAttrs: {[k: string]: string[]}, + preserveSignificantWhitespace: boolean, ): ExtractionResult { - const visitor = new _Visitor(implicitTags, implicitAttrs); + const visitor = new _Visitor(implicitTags, implicitAttrs, preserveSignificantWhitespace); return visitor.extract(nodes, interpolationConfig); } @@ -98,6 +99,7 @@ class _Visitor implements html.Visitor { constructor( private _implicitTags: string[], private _implicitAttrs: {[k: string]: string[]}, + private readonly _preserveSignificantWhitespace: boolean = true, ) {} /** @@ -347,6 +349,10 @@ class _Visitor implements html.Visitor { this._createI18nMessage = createI18nMessageFactory( interpolationConfig, DEFAULT_CONTAINER_BLOCKS, + // When dropping significant whitespace we need to retain whitespace tokens or + // else we won't be able to reuse source spans because empty tokens would be + // removed and cause a mismatch. + !this._preserveSignificantWhitespace /* retainEmptyTokens */, ); } diff --git a/packages/compiler/src/i18n/i18n_html_parser.ts b/packages/compiler/src/i18n/i18n_html_parser.ts index 974705e3c2a50..59ff488b3639b 100644 --- a/packages/compiler/src/i18n/i18n_html_parser.ts +++ b/packages/compiler/src/i18n/i18n_html_parser.ts @@ -79,7 +79,7 @@ function createSerializer(format?: string): Serializer { switch (format) { case 'xmb': - return new Xmb(); + return new Xmb(/* preservePlaceholders */ true); case 'xtb': return new Xtb(); case 'xliff2': diff --git a/packages/compiler/src/i18n/i18n_parser.ts b/packages/compiler/src/i18n/i18n_parser.ts index c409c8b5b7173..31f796be9b95a 100644 --- a/packages/compiler/src/i18n/i18n_parser.ts +++ b/packages/compiler/src/i18n/i18n_parser.ts @@ -37,8 +37,14 @@ export interface I18nMessageFactory { export function createI18nMessageFactory( interpolationConfig: InterpolationConfig, containerBlocks: Set, + retainEmptyTokens: boolean, ): I18nMessageFactory { - const visitor = new _I18nVisitor(_expParser, interpolationConfig, containerBlocks); + const visitor = new _I18nVisitor( + _expParser, + interpolationConfig, + containerBlocks, + retainEmptyTokens, + ); return (nodes, meaning, description, customId, visitNodeFn) => visitor.toI18nMessage(nodes, meaning, description, customId, visitNodeFn); } @@ -61,6 +67,7 @@ class _I18nVisitor implements html.Visitor { private _expressionParser: ExpressionParser, private _interpolationConfig: InterpolationConfig, private _containerBlocks: Set, + private readonly _retainEmptyTokens: boolean, ) {} public toI18nMessage( @@ -277,7 +284,16 @@ class _I18nVisitor implements html.Visitor { nodes.push(new i18n.Placeholder(expression, phName, token.sourceSpan)); break; default: - if (token.parts[0].length > 0) { + // Try to merge text tokens with previous tokens. We do this even for all tokens + // when `retainEmptyTokens == true` because whitespace tokens may have non-zero + // length, but will be trimmed by `WhitespaceVisitor` in one extraction pass and + // be considered "empty" there. Therefore a whitespace token with + // `retainEmptyTokens === true` should be treated like an empty token and either + // retained or merged into the previous node. Since extraction does two passes with + // different trimming behavior, the second pass needs to have identical node count + // to reuse source spans, so we need this check to get the same answer when both + // trimming and not trimming. + if (token.parts[0].length > 0 || this._retainEmptyTokens) { // This token is text or an encoded entity. // If it is following on from a previous text node then merge it into that node // Otherwise, if it is following an interpolation, then add a new node. @@ -293,7 +309,17 @@ class _I18nVisitor implements html.Visitor { } else { nodes.push(new i18n.Text(token.parts[0], token.sourceSpan)); } + } else { + // Retain empty tokens to avoid breaking dropping entire nodes such that source + // spans should not be reusable across multiple parses of a template. We *should* + // do this all the time, however we need to maintain backwards compatibility + // with existing message IDs so we can't do it by default and should only enable + // this when removing significant whitespace. + if (this._retainEmptyTokens) { + nodes.push(new i18n.Text(token.parts[0], token.sourceSpan)); + } } + break; } } @@ -359,7 +385,17 @@ function assertSingleContainerMessage(message: i18n.Message): void { */ function assertEquivalentNodes(previousNodes: i18n.Node[], nodes: i18n.Node[]): void { if (previousNodes.length !== nodes.length) { - throw new Error('The number of i18n message children changed between first and second pass.'); + throw new Error( + ` +The number of i18n message children changed between first and second pass. + +First pass (${previousNodes.length} tokens): +${previousNodes.map((node) => `"${node.sourceSpan.toString()}"`).join('\n')} + +Second pass (${nodes.length} tokens): +${nodes.map((node) => `"${node.sourceSpan.toString()}"`).join('\n')} + `.trim(), + ); } if (previousNodes.some((node, i) => nodes[i].constructor !== node.constructor)) { throw new Error( diff --git a/packages/compiler/src/i18n/message_bundle.ts b/packages/compiler/src/i18n/message_bundle.ts index 3009aec61be74..f3ed1faedc49e 100644 --- a/packages/compiler/src/i18n/message_bundle.ts +++ b/packages/compiler/src/i18n/message_bundle.ts @@ -6,8 +6,10 @@ * found in the LICENSE file at https://angular.io/license */ +import * as html from '../ml_parser/ast'; import {InterpolationConfig} from '../ml_parser/defaults'; import {HtmlParser} from '../ml_parser/html_parser'; +import {WhitespaceVisitor} from '../ml_parser/html_whitespaces'; import {ParseError} from '../parse_util'; import {extractMessages} from './extractor_merger'; @@ -25,14 +27,15 @@ export class MessageBundle { private _implicitTags: string[], private _implicitAttrs: {[k: string]: string[]}, private _locale: string | null = null, + private readonly _preserveWhitespace = true, ) {} updateFromTemplate( - html: string, + source: string, url: string, interpolationConfig: InterpolationConfig, ): ParseError[] { - const htmlParserResult = this._htmlParser.parse(html, url, { + const htmlParserResult = this._htmlParser.parse(source, url, { tokenizeExpansionForms: true, interpolationConfig, }); @@ -41,11 +44,22 @@ export class MessageBundle { return htmlParserResult.errors; } + // Trim unnecessary whitespace from extracted messages if requested. This + // makes the messages more durable to trivial whitespace changes without + // affected message IDs. + const rootNodes = this._preserveWhitespace + ? htmlParserResult.rootNodes + : html.visitAll( + new WhitespaceVisitor(/* preserveSignificantWhitespace */ false), + htmlParserResult.rootNodes, + ); + const i18nParserResult = extractMessages( - htmlParserResult.rootNodes, + rootNodes, interpolationConfig, this._implicitTags, this._implicitAttrs, + /* preserveSignificantWhitespace */ this._preserveWhitespace, ); if (i18nParserResult.errors.length) { diff --git a/packages/compiler/src/i18n/serializers/xliff2.ts b/packages/compiler/src/i18n/serializers/xliff2.ts index d74f3d7c65d3b..2837df8774166 100644 --- a/packages/compiler/src/i18n/serializers/xliff2.ts +++ b/packages/compiler/src/i18n/serializers/xliff2.ts @@ -128,7 +128,7 @@ export class Xliff2 extends Serializer { } override digest(message: i18n.Message): string { - return decimalDigest(message); + return decimalDigest(message, /* preservePlaceholders */ true); } } diff --git a/packages/compiler/src/i18n/serializers/xmb.ts b/packages/compiler/src/i18n/serializers/xmb.ts index 02ea6e82387a4..bedc073126d74 100644 --- a/packages/compiler/src/i18n/serializers/xmb.ts +++ b/packages/compiler/src/i18n/serializers/xmb.ts @@ -48,6 +48,10 @@ const _DOCTYPE = ` `; export class Xmb extends Serializer { + constructor(private readonly preservePlaceholders: boolean = true) { + super(); + } + override write(messages: i18n.Message[], locale: string | null): string { const exampleVisitor = new ExampleVisitor(); const visitor = new _Visitor(); @@ -104,7 +108,7 @@ export class Xmb extends Serializer { } override digest(message: i18n.Message): string { - return digest(message); + return digest(message, this.preservePlaceholders); } override createNameMapper(message: i18n.Message): PlaceholderMapper { @@ -202,8 +206,8 @@ class _Visitor implements i18n.Visitor { } } -export function digest(message: i18n.Message): string { - return decimalDigest(message); +export function digest(message: i18n.Message, preservePlaceholders: boolean): string { + return decimalDigest(message, preservePlaceholders); } // TC requires at least one non-empty example on placeholders diff --git a/packages/compiler/src/i18n/serializers/xtb.ts b/packages/compiler/src/i18n/serializers/xtb.ts index e2cedcd77a3a3..4158a5e5f2087 100644 --- a/packages/compiler/src/i18n/serializers/xtb.ts +++ b/packages/compiler/src/i18n/serializers/xtb.ts @@ -57,7 +57,7 @@ export class Xtb extends Serializer { } override digest(message: i18n.Message): string { - return digest(message); + return digest(message, /* preservePlaceholders */ true); } override createNameMapper(message: i18n.Message): PlaceholderMapper { diff --git a/packages/compiler/src/ml_parser/html_whitespaces.ts b/packages/compiler/src/ml_parser/html_whitespaces.ts index a4624cf69df3d..3e6af5dc9e244 100644 --- a/packages/compiler/src/ml_parser/html_whitespaces.ts +++ b/packages/compiler/src/ml_parser/html_whitespaces.ts @@ -9,7 +9,7 @@ import * as html from './ast'; import {NGSP_UNICODE} from './entities'; import {ParseTreeResult} from './parser'; -import {TextToken, TokenType} from './tokens'; +import {InterpolatedTextToken, TextToken, TokenType} from './tokens'; export const PRESERVE_WS_ATTR_NAME = 'ngPreserveWhitespaces'; @@ -48,13 +48,28 @@ export function replaceNgsp(value: string): string { * this visitor is not activated by default in Angular 5 and people need to explicitly opt-in for * whitespace removal. The default option for whitespace removal will be revisited in Angular 6 * and might be changed to "on" by default. + * + * If `originalNodeMap` is provided, the transformed nodes will be mapped back to their original + * inputs. Any output nodes not in the map were not transformed. This supports correlating and + * porting information between the trimmed nodes and original nodes (such as `i18n` properties) + * such that trimming whitespace does not does not drop required information from the node. */ export class WhitespaceVisitor implements html.Visitor { + // How many ICU expansions which are currently being visited. ICUs can be nested, so this + // tracks the current depth of nesting. If this depth is greater than 0, then this visitor is + // currently processing content inside an ICU expansion. + private icuExpansionDepth = 0; + + constructor( + private readonly preserveSignificantWhitespace: boolean, + private readonly originalNodeMap?: Map, + ) {} + visitElement(element: html.Element, context: any): any { if (SKIP_WS_TRIM_TAGS.has(element.name) || hasPreserveWhitespacesAttr(element.attrs)) { // don't descent into elements where we need to preserve whitespaces // but still visit all attributes to eliminate one used as a market to preserve WS - return new html.Element( + const newElement = new html.Element( element.name, html.visitAll(this, element.attrs), element.children, @@ -63,9 +78,11 @@ export class WhitespaceVisitor implements html.Visitor { element.endSourceSpan, element.i18n, ); + this.originalNodeMap?.set(newElement, element); + return newElement; } - return new html.Element( + const newElement = new html.Element( element.name, element.attrs, visitAllWithSiblings(this, element.children), @@ -74,6 +91,8 @@ export class WhitespaceVisitor implements html.Visitor { element.endSourceSpan, element.i18n, ); + this.originalNodeMap?.set(newElement, element); + return newElement; } visitAttribute(attribute: html.Attribute, context: any): any { @@ -85,14 +104,41 @@ export class WhitespaceVisitor implements html.Visitor { const hasExpansionSibling = context && (context.prev instanceof html.Expansion || context.next instanceof html.Expansion); + // Do not trim whitespace within ICU expansions when preserving significant whitespace. + // Historically, ICU whitespace was never trimmed and this is really a bug. However fixing it + // would change message IDs which we can't easily do. Instead we only trim ICU whitespace within + // ICU expansions when not preserving significant whitespace, which is the new behavior where it + // most matters. + const inIcuExpansion = this.icuExpansionDepth > 0; + if (inIcuExpansion && this.preserveSignificantWhitespace) return text; + if (isNotBlank || hasExpansionSibling) { // Process the whitespace in the tokens of this Text node const tokens = text.tokens.map((token) => token.type === TokenType.TEXT ? createWhitespaceProcessedTextToken(token) : token, ); - // Process the whitespace of the value of this Text node - const value = processWhitespace(text.value); - return new html.Text(value, text.sourceSpan, tokens, text.i18n); + + // Fully trim message when significant whitespace is not preserved. + if (!this.preserveSignificantWhitespace && tokens.length > 0) { + // The first token should only call `.trimStart()` and the last token + // should only call `.trimEnd()`, but there might be only one token which + // needs to call both. + const firstToken = tokens[0]!; + tokens.splice(0, 1, trimLeadingWhitespace(firstToken, context)); + + const lastToken = tokens[tokens.length - 1]; // Could be the same as the first token. + tokens.splice(tokens.length - 1, 1, trimTrailingWhitespace(lastToken, context)); + } + + // Process the whitespace of the value of this Text node. Also trim the leading/trailing + // whitespace when we don't need to preserve significant whitespace. + const processed = processWhitespace(text.value); + const value = this.preserveSignificantWhitespace + ? processed + : trimLeadingAndTrailingWhitespace(processed, context); + const result = new html.Text(value, text.sourceSpan, tokens, text.i18n); + this.originalNodeMap?.set(result, text); + return result; } return null; @@ -103,15 +149,40 @@ export class WhitespaceVisitor implements html.Visitor { } visitExpansion(expansion: html.Expansion, context: any): any { - return expansion; + this.icuExpansionDepth++; + let newExpansion: html.Expansion; + try { + newExpansion = new html.Expansion( + expansion.switchValue, + expansion.type, + visitAllWithSiblings(this, expansion.cases), + expansion.sourceSpan, + expansion.switchValueSourceSpan, + expansion.i18n, + ); + } finally { + this.icuExpansionDepth--; + } + + this.originalNodeMap?.set(newExpansion, expansion); + + return newExpansion; } visitExpansionCase(expansionCase: html.ExpansionCase, context: any): any { - return expansionCase; + const newExpansionCase = new html.ExpansionCase( + expansionCase.value, + visitAllWithSiblings(this, expansionCase.expression), + expansionCase.sourceSpan, + expansionCase.valueSourceSpan, + expansionCase.expSourceSpan, + ); + this.originalNodeMap?.set(newExpansionCase, expansionCase); + return newExpansionCase; } visitBlock(block: html.Block, context: any): any { - return new html.Block( + const newBlock = new html.Block( block.name, block.parameters, visitAllWithSiblings(this, block.children), @@ -120,6 +191,8 @@ export class WhitespaceVisitor implements html.Visitor { block.startSourceSpan, block.endSourceSpan, ); + this.originalNodeMap?.set(newBlock, block); + return newBlock; } visitBlockParameter(parameter: html.BlockParameter, context: any) { @@ -131,17 +204,67 @@ export class WhitespaceVisitor implements html.Visitor { } } +function trimLeadingWhitespace( + token: InterpolatedTextToken, + context: SiblingVisitorContext | null, +): InterpolatedTextToken { + if (token.type !== TokenType.TEXT) return token; + + const isFirstTokenInTag = !context?.prev; + if (!isFirstTokenInTag) return token; + + return transformTextToken(token, (text) => text.trimStart()); +} + +function trimTrailingWhitespace( + token: InterpolatedTextToken, + context: SiblingVisitorContext | null, +): InterpolatedTextToken { + if (token.type !== TokenType.TEXT) return token; + + const isLastTokenInTag = !context?.next; + if (!isLastTokenInTag) return token; + + return transformTextToken(token, (text) => text.trimEnd()); +} + +function trimLeadingAndTrailingWhitespace( + text: string, + context: SiblingVisitorContext | null, +): string { + const isFirstTokenInTag = !context?.prev; + const isLastTokenInTag = !context?.next; + + const maybeTrimmedStart = isFirstTokenInTag ? text.trimStart() : text; + const maybeTrimmed = isLastTokenInTag ? maybeTrimmedStart.trimEnd() : maybeTrimmedStart; + return maybeTrimmed; +} + function createWhitespaceProcessedTextToken({type, parts, sourceSpan}: TextToken): TextToken { return {type, parts: [processWhitespace(parts[0])], sourceSpan}; } +function transformTextToken( + {type, parts, sourceSpan}: TextToken, + transform: (parts: string) => string, +): TextToken { + // `TextToken` only ever has one part as defined in its type, so we just transform the first element. + return {type, parts: [transform(parts[0])], sourceSpan}; +} + function processWhitespace(text: string): string { return replaceNgsp(text).replace(WS_REPLACE_REGEXP, ' '); } -export function removeWhitespaces(htmlAstWithErrors: ParseTreeResult): ParseTreeResult { +export function removeWhitespaces( + htmlAstWithErrors: ParseTreeResult, + preserveSignificantWhitespace: boolean, +): ParseTreeResult { return new ParseTreeResult( - html.visitAll(new WhitespaceVisitor(), htmlAstWithErrors.rootNodes), + html.visitAll( + new WhitespaceVisitor(preserveSignificantWhitespace), + htmlAstWithErrors.rootNodes, + ), htmlAstWithErrors.errors, ); } diff --git a/packages/compiler/src/render3/view/i18n/meta.ts b/packages/compiler/src/render3/view/i18n/meta.ts index a40e7b98084c5..e1786d1668a24 100644 --- a/packages/compiler/src/render3/view/i18n/meta.ts +++ b/packages/compiler/src/render3/view/i18n/meta.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ +import {WhitespaceVisitor} from '../../../ml_parser/html_whitespaces'; import {computeDecimalDigest, computeDigest, decimalDigest} from '../../../i18n/digest'; import * as i18n from '../../../i18n/i18n_ast'; import {createI18nMessageFactory, VisitNodeFn} from '../../../i18n/i18n_parser'; @@ -30,18 +31,27 @@ export type I18nMeta = { meaning?: string; }; -const setI18nRefs: VisitNodeFn = (htmlNode, i18nNode) => { - if (htmlNode instanceof html.NodeWithI18n) { - if (i18nNode instanceof i18n.IcuPlaceholder && htmlNode.i18n instanceof i18n.Message) { - // This html node represents an ICU but this is a second processing pass, and the legacy id - // was computed in the previous pass and stored in the `i18n` property as a message. - // We are about to wipe out that property so capture the previous message to be reused when - // generating the message for this ICU later. See `_generateI18nMessage()`. - i18nNode.previousMessage = htmlNode.i18n; +const setI18nRefs = (originalNodeMap: Map): VisitNodeFn => { + return (trimmedNode, i18nNode) => { + // We need to set i18n properties on the original, untrimmed AST nodes. The i18n nodes needs to + // use the trimmed content for message IDs to make messages more stable to whitespace changes. + // But we don't want to actually trim the content, so we can't use the trimmed HTML AST for + // general code gen. Instead we map the trimmed HTML AST back to the original AST and then + // attach the i18n nodes so we get trimmed i18n nodes on the original (untrimmed) HTML AST. + const originalNode = originalNodeMap.get(trimmedNode) ?? trimmedNode; + + if (originalNode instanceof html.NodeWithI18n) { + if (i18nNode instanceof i18n.IcuPlaceholder && originalNode.i18n instanceof i18n.Message) { + // This html node represents an ICU but this is a second processing pass, and the legacy id + // was computed in the previous pass and stored in the `i18n` property as a message. + // We are about to wipe out that property so capture the previous message to be reused when + // generating the message for this ICU later. See `_generateI18nMessage()`. + i18nNode.previousMessage = originalNode.i18n; + } + originalNode.i18n = i18nNode; } - htmlNode.i18n = i18nNode; - } - return i18nNode; + return i18nNode; + }; }; /** @@ -59,6 +69,15 @@ export class I18nMetaVisitor implements html.Visitor { private keepI18nAttrs = false, private enableI18nLegacyMessageIdFormat = false, private containerBlocks: Set = DEFAULT_CONTAINER_BLOCKS, + private readonly preserveSignificantWhitespace: boolean = true, + + // When dropping significant whitespace we need to retain empty tokens or + // else we won't be able to reuse source spans because empty tokens would be + // removed and cause a mismatch. Unfortunately this still needs to be + // configurable and sometimes needs to be set independently in order to make + // sure the number of nodes don't change between parses, even when + // `preserveSignificantWhitespace` changes. + private readonly retainEmptyTokens: boolean = !preserveSignificantWhitespace, ) {} private _generateI18nMessage( @@ -70,6 +89,7 @@ export class I18nMetaVisitor implements html.Visitor { const createI18nMessage = createI18nMessageFactory( this.interpolationConfig, this.containerBlocks, + this.retainEmptyTokens, ); const message = createI18nMessage(nodes, meaning, description, customId, visitNodeFn); this._setMessageId(message, meta); @@ -94,7 +114,26 @@ export class I18nMetaVisitor implements html.Visitor { if (attr.name === I18N_ATTR) { // root 'i18n' node attribute const i18n = element.i18n || attr.value; - message = this._generateI18nMessage(element.children, i18n, setI18nRefs); + + // Generate a new AST with whitespace trimmed, but also generate a map + // to correlate each new node to its original so we can apply i18n + // information to the original node based on the trimmed content. + // + // `WhitespaceVisitor` removes *insignificant* whitespace as well as + // significant whitespace. Enabling this visitor should be conditional + // on `preserveWhitespace` rather than `preserveSignificantWhitespace`, + // however this would be a breaking change for existing behavior where + // `preserveWhitespace` was not respected correctly when generating + // message IDs. This is really a bug but one we need to keep to maintain + // backwards compatibility. + const originalNodeMap = new Map(); + const trimmedNodes = this.preserveSignificantWhitespace + ? element.children + : html.visitAll( + new WhitespaceVisitor(false /* preserveSignificantWhitespace */, originalNodeMap), + element.children, + ); + message = this._generateI18nMessage(trimmedNodes, i18n, setI18nRefs(originalNodeMap)); if (message.nodes.length === 0) { // Ignore the message if it is empty. message = undefined; @@ -216,7 +255,9 @@ export class I18nMetaVisitor implements html.Visitor { */ private _setMessageId(message: i18n.Message, meta: string | i18n.I18nMeta): void { if (!message.id) { - message.id = (meta instanceof i18n.Message && meta.id) || decimalDigest(message); + message.id = + (meta instanceof i18n.Message && meta.id) || + decimalDigest(message, /* preservePlaceholders */ this.preserveSignificantWhitespace); } } @@ -228,7 +269,13 @@ export class I18nMetaVisitor implements html.Visitor { */ private _setLegacyIds(message: i18n.Message, meta: string | i18n.I18nMeta): void { if (this.enableI18nLegacyMessageIdFormat) { - message.legacyIds = [computeDigest(message), computeDecimalDigest(message)]; + message.legacyIds = [ + computeDigest(message), + computeDecimalDigest( + message, + /* preservePlaceholders */ this.preserveSignificantWhitespace, + ), + ]; } else if (typeof meta !== 'string') { // This occurs if we are doing the 2nd pass after whitespace removal (see `parseTemplate()` in // `packages/compiler/src/render3/view/template.ts`). diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index adc255fb0bef7..555396acdec17 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -35,6 +35,10 @@ export interface ParseTemplateOptions { * Preserve original line endings instead of normalizing '\r\n' endings to '\n'. */ preserveLineEndings?: boolean; + /** + * Preserve whitespace significant to rendering. + */ + preserveSignificantWhitespace?: boolean; /** * How to parse interpolation markers. */ @@ -186,6 +190,12 @@ export function parseTemplate( let rootNodes: html.Node[] = parseResult.rootNodes; + // We need to use the same `retainEmptyTokens` value for both parses to avoid + // causing a mismatch when reusing source spans, even if the + // `preserveSignificantWhitespace` behavior is different between the two + // parses. + const retainEmptyTokens = !(options.preserveSignificantWhitespace ?? true); + // process i18n meta information (scan attributes, generate ids) // before we run whitespace removal process, because existing i18n // extraction process (ng extract-i18n) relies on a raw content to generate @@ -194,6 +204,9 @@ export function parseTemplate( interpolationConfig, /* keepI18nAttrs */ !preserveWhitespaces, enableI18nLegacyMessageIdFormat, + /* containerBlocks */ undefined, + options.preserveSignificantWhitespace, + retainEmptyTokens, ); const i18nMetaResult = i18nMetaVisitor.visitAllWithErrors(rootNodes); @@ -220,7 +233,14 @@ export function parseTemplate( rootNodes = i18nMetaResult.rootNodes; if (!preserveWhitespaces) { - rootNodes = html.visitAll(new WhitespaceVisitor(), rootNodes); + // Always preserve significant whitespace here because this is used to generate the `goog.getMsg` + // and `$localize` calls which should retain significant whitespace in order to render the + // correct output. We let this diverge from the message IDs generated earlier which might not + // have preserved significant whitespace. + rootNodes = html.visitAll( + new WhitespaceVisitor(/* preserveSignificantWhitespace */ true), + rootNodes, + ); // run i18n meta visitor again in case whitespaces are removed (because that might affect // generated i18n message content) and first pass indicated that i18n content is present in a @@ -228,7 +248,14 @@ export function parseTemplate( // mimic existing extraction process (ng extract-i18n) if (i18nMetaVisitor.hasI18nMeta) { rootNodes = html.visitAll( - new I18nMetaVisitor(interpolationConfig, /* keepI18nAttrs */ false), + new I18nMetaVisitor( + interpolationConfig, + /* keepI18nAttrs */ false, + /* enableI18nLegacyMessageIdFormat */ undefined, + /* containerBlocks */ undefined, + /* preserveSignificantWhitespace */ true, + retainEmptyTokens, + ), rootNodes, ); } diff --git a/packages/compiler/test/i18n/extractor_merger_spec.ts b/packages/compiler/test/i18n/extractor_merger_spec.ts index da0839475a055..ac385ad7735e3 100644 --- a/packages/compiler/test/i18n/extractor_merger_spec.ts +++ b/packages/compiler/test/i18n/extractor_merger_spec.ts @@ -525,6 +525,7 @@ describe('Merger', () => { DEFAULT_INTERPOLATION_CONFIG, [], {}, + /* preserveSignificantWhitespace */ true, ).messages; expect(messages.length).toEqual(1); @@ -601,6 +602,7 @@ describe('Merger', () => { DEFAULT_INTERPOLATION_CONFIG, [], {}, + /* preserveSignificantWhitespace */ true, ).messages; expect(messages.length).toEqual(1); @@ -668,6 +670,7 @@ function fakeTranslate( DEFAULT_INTERPOLATION_CONFIG, implicitTags, implicitAttrs, + /* preserveSignificantWhitespace */ true, ).messages; const i18nMsgMap: {[id: string]: i18n.Node[]} = {}; @@ -727,6 +730,7 @@ function extract( DEFAULT_INTERPOLATION_CONFIG, implicitTags, implicitAttrs, + /* preserveSignificantWhitespace */ true, ); if (result.errors.length > 0) { @@ -751,6 +755,7 @@ function extractErrors( DEFAULT_INTERPOLATION_CONFIG, implicitTags, implicitAttrs, + /* preserveSignificantWhitespace */ true, ).errors; return errors.map((e): [string, string] => [e.msg, e.span.toString()]); diff --git a/packages/compiler/test/i18n/i18n_ast_spec.ts b/packages/compiler/test/i18n/i18n_ast_spec.ts index a597e11c01e64..1f0741ea1d0c8 100644 --- a/packages/compiler/test/i18n/i18n_ast_spec.ts +++ b/packages/compiler/test/i18n/i18n_ast_spec.ts @@ -15,6 +15,7 @@ describe('Message', () => { const messageFactory = createI18nMessageFactory( DEFAULT_INTERPOLATION_CONFIG, DEFAULT_CONTAINER_BLOCKS, + /* retainEmptyTokens */ false, ); describe('messageText()', () => { it('should serialize simple text', () => { diff --git a/packages/compiler/test/i18n/i18n_parser_spec.ts b/packages/compiler/test/i18n/i18n_parser_spec.ts index df82fa26b75c5..398be5b967fd3 100644 --- a/packages/compiler/test/i18n/i18n_parser_spec.ts +++ b/packages/compiler/test/i18n/i18n_parser_spec.ts @@ -374,5 +374,6 @@ export function _extractMessages( DEFAULT_INTERPOLATION_CONFIG, implicitTags, implicitAttrs, + /* preserveSignificantWhitespace */ true, ).messages; } diff --git a/packages/compiler/test/i18n/integration_xmb_xtb_spec.ts b/packages/compiler/test/i18n/integration_xmb_xtb_spec.ts index ed5c3b342965c..1294eb892a785 100644 --- a/packages/compiler/test/i18n/integration_xmb_xtb_spec.ts +++ b/packages/compiler/test/i18n/integration_xmb_xtb_spec.ts @@ -24,7 +24,7 @@ xdescribe('i18n XMB/XTB integration spec', () => { beforeEach(waitForAsync(() => configureCompiler(XTB + LF_LINE_ENDING_XTB, 'xtb'))); it('should extract from templates', () => { - const serializer = new Xmb(); + const serializer = new Xmb(/* preservePlaceholders */ true); const serializedXmb = serializeTranslations(HTML, serializer); XMB.forEach((x) => { @@ -43,7 +43,7 @@ xdescribe('i18n XMB/XTB integration spec', () => { beforeEach(waitForAsync(() => configureCompiler(XTB + CRLF_LINE_ENDING_XTB, 'xtb'))); it('should extract from templates (with CRLF line endings)', () => { - const serializer = new Xmb(); + const serializer = new Xmb(/* preservePlaceholders */ true); const serializedXmb = serializeTranslations(HTML.replace(/\n/g, '\r\n'), serializer); XMB.forEach((x) => { diff --git a/packages/compiler/test/i18n/serializers/xmb_spec.ts b/packages/compiler/test/i18n/serializers/xmb_spec.ts index 6d3eb50a328c3..05da0f3c4595a 100644 --- a/packages/compiler/test/i18n/serializers/xmb_spec.ts +++ b/packages/compiler/test/i18n/serializers/xmb_spec.ts @@ -70,7 +70,7 @@ lines it('should throw when trying to load an xmb file', () => { expect(() => { - const serializer = new Xmb(); + const serializer = new Xmb(/* preservePlaceholders */ true); serializer.load(XMB, 'url'); }).toThrowError(/Unsupported/); }); @@ -78,7 +78,7 @@ lines function toXmb(html: string, url: string, locale: string | null = null): string { const catalog = new MessageBundle(new HtmlParser(), [], {}, locale); - const serializer = new Xmb(); + const serializer = new Xmb(/* preservePlaceholders */ true); catalog.updateFromTemplate(html, url, DEFAULT_INTERPOLATION_CONFIG); diff --git a/packages/compiler/test/i18n/whitespace_sensitivity_spec.ts b/packages/compiler/test/i18n/whitespace_sensitivity_spec.ts new file mode 100644 index 0000000000000..2ba1799a0fa43 --- /dev/null +++ b/packages/compiler/test/i18n/whitespace_sensitivity_spec.ts @@ -0,0 +1,469 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import * as i18n from '../../src/i18n/i18n_ast'; +import {MessageBundle} from '../../src/i18n/message_bundle'; +import {DEFAULT_INTERPOLATION_CONFIG} from '../../src/ml_parser/defaults'; +import {HtmlParser} from '../../src/ml_parser/html_parser'; +import {Xmb} from '../../src/i18n/serializers/xmb'; + +describe('i18nPreserveWhitespaceForLegacyExtraction', () => { + describe('ignores whitespace changes when disabled', () => { + it('from converting one-line messages to block messages', () => { + const initial = extractMessages( + ` +
Hello, World!
+
{{ abc }}
+
Start {{ abc }} End
+
{{ first }} middle {{ end }}
+ +
Before First Second After
+
+
Before After
+
{apples, plural, =1 {One apple.} =other {Many apples.}}
+
{apples, plural, =other {{bananas, plural, =other{Many apples and bananas.}}}}
+ +i18nPreserveWhitespaceForLegacyExtraction does not support changing ICU case text. +Test case is disabled by omitting the i18n attribute. +
{apples, plural, =1 {One apple.} =other {Many apples.}}
+ `.trim(), + false /* preserveWhitespace */, + ); + + const multiLine = extractMessages( + ` +
+ Hello, World! +
+
+ {{ abc }} +
+
+ Start {{ abc }} End +
+
+ {{ first }} middle {{ end }} +
+ +
+ Before + + First + Second + + After +
+
+ +
+
+ Before + + After +
+
{ + apples, plural, + =1 {One apple.} + =other {Many apples.} +}
+
{ + apples, plural, + =other {{bananas, plural, + =other{Many apples and bananas.} + }} +}
+ +i18nPreserveWhitespaceForLegacyExtraction does not support changing ICU case text. +Test case is disabled by omitting the i18n attribute. +
{ + apples, plural, + =1 { + One + apple. + } + =other { + Many + apples. + } +}
+ `.trim(), + false /* preserveWhitespace */, + ); + + expect(multiLine.length).toBe(10); + expect(multiLine.map(({id}) => id)).toEqual(initial.map(({id}) => id)); + }); + + it('from indenting a message', () => { + const initial = extractMessages( + ` +
+ Hello, World! +
+
+ {{ abc }} +
+
+ Start {{ abc }} End +
+
+ {{ first }} middle {{ end }} +
+ +
+ Before + Link + After +
+
+ +
+
+ Before After +
+
{ + apples, plural, + =1 {One apple.} + =other {Many apples.} +}
+
{ + apples, plural, + =other {{bananas, plural, + =other{Many apples and bananas.} + }} +}
+ +i18nPreserveWhitespaceForLegacyExtraction does not support indenting ICU case text. +Test case is disabled by omitting the i18n attribute. +
{ + apples, plural, + =1 { + One + apple. + } + =other { + Many + apples. + } +}
+ `.trim(), + false /* preserveWhitespace */, + ); + + const indented = extractMessages( + ` +
+
+ Hello, World! +
+
+ {{ abc }} +
+
+ Start {{ abc }} End +
+
+ {{ first }} middle {{ end }} +
+ +
+ Before + Link + After +
+
+ +
+
+ Before After +
+
{ + apples, plural, + =1 {One apple.} + =other {Many apples.} + }
+
{ + apples, plural, + =other {{bananas, plural, + =other{Many apples and bananas.} + }} + }
+ + i18nPreserveWhitespaceForLegacyExtraction does not support indenting ICU case text. + Test case is disabled by omitting the i18n attribute. +
{ + apples, plural, + =1 { + One + apple. + } + =other { + Many + apples. + } + }
+
+ `.trim(), + false /* preserveWhitespace */, + ); + + expect(indented.length).toBe(10); + expect(indented.map(({id}) => id)).toEqual(initial.map(({id}) => id)); + }); + + it('from adjusting line wrapping', () => { + const initial = extractMessages( + ` +
+ This is a long message which maybe + exceeds line length. +
+
+ {{ veryLongExpressionWhichMaybeExceedsLineLength | async }} +
+
+ This is a long {{ abc }} which maybe + exceeds line length. +
+
+ {{ first }} long message {{ end }} +
+ +
+ This is a + long message which + maybe exceeds line length. +
+
+ +
+
+ Before After +
+ +i18nPreserveWhitespaceForLegacyExtraction does not support line wrapping ICU case text. +Test case is disabled by omitting the i18n attribute. +
{ + apples, plural, =other {Very long text which maybe exceeds line length.} +}
+ `.trim(), + false /* preserveWhitespace */, + ); + + const adjusted = extractMessages( + ` +
+ This is a long message which + maybe exceeds line length. +
+
+ {{ + veryLongExpressionWhichMaybeExceedsLineLength + | async + }} +
+
+ This is a long {{ abc }} which + maybe exceeds line length. +
+
+ {{ first }} + long message + {{ end }} +
+ +
+ This is a long + + message + + which maybe exceeds line length. +
+
+ +
+
+ Before + + After +
+ +i18nPreserveWhitespaceForLegacyExtraction does not support line wrapping ICU case text. +Test case is disabled by omitting the i18n attribute. +
{ + apples, plural, =other { + Very long text which + maybe exceeds line length. + } +}
+ `.trim(), + false /* preserveWhitespace */, + ); + + expect(adjusted.length).toBe(8); + expect(adjusted.map(({id}) => id)).toEqual(initial.map(({id}) => id)); + }); + + it('from trimming significant whitespace', () => { + const initial = extractMessages( + ` +
Hello, World!
+
{{ abc }}
+
Start {{ abc }} End
+
{{ first }} middle {{ end }}
+
Foo
+ +
Before Link After
+
+
Before After
+ +i18nPreserveWhitespaceForLegacyExtraction does not support trimming ICU case text. +Test case is disabled by omitting the i18n attribute. +
{ + apples, plural, + =1 { One apple. } + =other { Many apples. } +}
+ `.trim(), + false /* preserveWhitespace */, + ); + + const trimmed = extractMessages( + ` +
Hello, World!
+
{{ abc }}
+
Start {{ abc }} End
+
{{ first }} middle {{ end }}
+ + +
Before Link After
+
+
Before After
+ +i18nPreserveWhitespaceForLegacyExtraction does not support trimming ICU case text. +Test case is disabled by omitting the i18n attribute. +
{ + apples, plural, + =1 {One apple.} + =other {Many apples.} +}
+ `.trim(), + false /* preserveWhitespace */, + ); + + expect(trimmed.length).toBe(9); + expect(trimmed).toEqual(initial); + }); + }); +}); + +interface AssertableMessage { + id: string; + text: string; +} + +/** + * Serializes a message for easy assertion that the meaningful text did not change. + * For example: This does not include expression source, because that is allowed to + * change without affecting message IDs or extracted placeholders. + */ +const debugSerializer = new (class implements i18n.Visitor { + visitText(text: i18n.Text): string { + return text.value; + } + + visitContainer(container: i18n.Container): string { + return `[${container.children.map((child) => child.visit(this)).join(', ')}]`; + } + + visitIcu(icu: i18n.Icu): string { + const strCases = Object.keys(icu.cases).map( + (k: string) => `${k} {${icu.cases[k].visit(this)}}`, + ); + return `{${icu.expression}, ${icu.type}, ${strCases.join(', ')}}`; + } + + visitTagPlaceholder(ph: i18n.TagPlaceholder): string { + return ph.isVoid + ? `` + : `${ph.children + .map((child) => child.visit(this)) + .join(', ')}`; + } + + visitPlaceholder(ph: i18n.Placeholder): string { + return ``; + } + + visitIcuPlaceholder(ph: i18n.IcuPlaceholder): string { + return ``; + } + + visitBlockPlaceholder(ph: i18n.BlockPlaceholder): string { + return `${ph.children + .map((child) => child.visit(this)) + .join(', ')}`; + } +})(); + +function extractMessages(source: string, preserveWhitespace: boolean): AssertableMessage[] { + const bundle = new MessageBundle( + new HtmlParser(), + [] /* implicitTags */, + {} /* implicitAttrs */, + undefined /* locale */, + preserveWhitespace, + ); + const errors = bundle.updateFromTemplate(source, 'url', DEFAULT_INTERPOLATION_CONFIG); + if (errors.length !== 0) { + throw new Error( + `Failed to parse template:\n${errors.map((err) => err.toString()).join('\n\n')}`, + ); + } + + const messages = bundle.getMessages(); + + const xmbSerializer = new Xmb(/* preservePlaceholders */ preserveWhitespace); + return messages.map((message) => ({ + id: xmbSerializer.digest(message), + text: message.nodes.map((node) => node.visit(debugSerializer)).join(''), + })); +} diff --git a/packages/compiler/test/ml_parser/html_whitespaces_spec.ts b/packages/compiler/test/ml_parser/html_whitespaces_spec.ts index a594ca8b2830f..8379e5c6f1d05 100644 --- a/packages/compiler/test/ml_parser/html_whitespaces_spec.ts +++ b/packages/compiler/test/ml_parser/html_whitespaces_spec.ts @@ -16,7 +16,12 @@ import {humanizeDom} from './ast_spec_utils'; describe('removeWhitespaces', () => { function parseAndRemoveWS(template: string, options?: TokenizeOptions): any[] { - return humanizeDom(removeWhitespaces(new HtmlParser().parse(template, 'TestComp', options))); + return humanizeDom( + removeWhitespaces( + new HtmlParser().parse(template, 'TestComp', options), + true /* preserveSignificantWhitespace */, + ), + ); } it('should remove blank text nodes', () => { diff --git a/packages/compiler/test/render3/view/util.ts b/packages/compiler/test/render3/view/util.ts index 020993116690c..9281b7144dacd 100644 --- a/packages/compiler/test/render3/view/util.ts +++ b/packages/compiler/test/render3/view/util.ts @@ -164,7 +164,10 @@ export function parseR3( let htmlNodes = processI18nMeta(parseResult).rootNodes; if (!options.preserveWhitespaces) { - htmlNodes = html.visitAll(new WhitespaceVisitor(), htmlNodes); + htmlNodes = html.visitAll( + new WhitespaceVisitor(true /* preserveSignificantWhitespace */), + htmlNodes, + ); } const expressionParser = new Parser(new Lexer()); From 5a0ff41f75fd6d58d73baf11315c012131d9ec82 Mon Sep 17 00:00:00 2001 From: Doug Parker Date: Thu, 18 Jul 2024 17:28:09 -0700 Subject: [PATCH 15/36] refactor(compiler): ensure context is always provided for `WhitespaceVisitor` (#56507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When disabling `i18nPreserveSignificantWhitespaceForLegacyExtraction` I was looking at a test case with ICU messages containing leading and trailing whitespace: ```angular
{apples, plural, =other {I have many apples.}}
``` This would historically generate two messages: ```javascript const MSG_TMP = goog.getMsg('{apples, plural, =other {I have many apples.}}'); const MSG_FOO = goog.getMsg(' {$ICU} ', { 'ICU': MSG_TMP }); ``` But I found that I was getting just one message: ```javascript const MSG_TMP = goog.getMsg(' {apples, plural, =other {I have many apples.}} '); ``` This is arguably an improvement, but changed the messages and message IDs, which isn't desirable with this option. I eventually traced this back to the `isIcu` initialization in [`i18n_parser.ts`](/packages/compiler/src/i18n/i18n_parser.ts): ```typescript const context: I18nMessageVisitorContext = { isIcu: nodes.length == 1 && nodes[0] instanceof html.Expansion, // ... }; ``` [`_I18nVisitor.prototype.visitExpansion`](/packages/compiler/src/i18n/i18n_parser.ts) uses this to decide whether or not to generate a sub-message for a given ICU expansion: ```typescript if (context.isIcu || context.icuDepth > 0) { // Returns an ICU node when: // - the message (vs a part of the message) is an ICU message, or // - the ICU message is nested. const expPh = context.placeholderRegistry.getUniquePlaceholder(`VAR_${icu.type}`); i18nIcu.expressionPlaceholder = expPh; context.placeholderToContent[expPh] = { text: icu.switchValue, sourceSpan: icu.switchValueSourceSpan, }; return context.visitNodeFn(icu, i18nIcu); } // Else returns a placeholder // ICU placeholders should not be replaced with their original content but with the their // translations. // TODO(vicb): add a html.Node -> i18n.Message cache to avoid having to re-create the msg const phName = context.placeholderRegistry.getPlaceholderName('ICU', icu.sourceSpan.toString()); context.placeholderToMessage[phName] = this.toI18nMessage([icu], '', '', '', undefined); const node = new i18n.IcuPlaceholder(i18nIcu, phName, icu.sourceSpan); return context.visitNodeFn(icu, node); ``` Note that `isIcu` is the key condition between these two cases and depends on whether or not the ICU expansion has any siblings. The introduction of `WhitespaceVisitor` to `I18nMetaVisitor` trims insignificant whitespace, including empty text nodes not adjacent to an ICU expansion (from [`WhitespaceVisitor.prototype.visitText`](/packages/compiler/src/ml_parser/html_whitespaces.ts)): ```typescript const isNotBlank = text.value.match(NO_WS_REGEXP); const hasExpansionSibling = context && (context.prev instanceof html.Expansion || context.next instanceof html.Expansion); if (isNotBlank || hasExpansionSibling) { // Transform node by trimming it... return trimmedNode; } return null; // Drop node which is empty and has no ICU expansion sibling. ``` `hasExpansionSibling` was intended to retain empty text nodes leading or trailing an ICU expansion, however `context` was `undefined`, so this check failed and the leading / trailing text nodes were dropped. This resulted in trimming the ICU text by dropping the leading / trailing whitespace nodes. Having only a single ICU expansion with no leading / trailing text nodes caused `_I18nVisitor` to initialize `isIcu` incorrectly and caused it to generate one message instead of two. `WhitespaceVisitor` is supposed to get this context from `visitAllWithSiblings`. So the fix here is to make sure `WhitespaceVisitor` is always visited via this function which provides the required context. I updated all usage sites to make sure this context is use consistently and implemented the `WhitespaceVisitor.prototype.visit` method to throw when the context is missing to make sure we don't encounter a similar mistake in the future. Unfortunately this broke one compliance test. Specifically the [`icu_logic/icu_only.js`](/home/douglasparker/Source/ng/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/icu_logic/icu_only.js) test which changed from generating: ```javascript function MyComponent_Template(rf, ctx) { if (rf & 1) { i0.ɵɵi18n(0, 0); } // ... } ``` To now generating: ```javascript function MyComponent_Template(rf, ctx) { if (rf & 1) { i0.ɵɵtext(0, " "); i0.ɵɵi18n(1, 0); i0.ɵɵtext(2, "\n"); } // ... } ``` This test uses the default value `preserveWhitespaces: false` (`i18nPreserveSignificantWhitespaceForLegacyExtraction` should not affect compiled JS output, we already retain significant whitespace there). So what this indicates to me is that ICU logic is already broken because it's not preserving significant whitespace in this case. My change is probably a bug fix, but one which would affect the compiled runtime, which is not in scope here. The root cause is because using `visitAllWithSiblings` everywhere means the context is retained correctly in this case and the whitespace is leading/trailing an ICU message, therefore it is retained per the logic of `WhitespaceVisitor.prototype.visitText` I mentioned eariler. To address this, I left one usage of `WhitespaceVisitor` using `html.visitAll` instead of `visitAllWithSiblings` to retain this bug. I has to lossen the assertion I put in `WhitespaceVisitor.prototype.visit` to make this possible, but it should still throw by default when misused, which is the important part. PR Close #56507 --- packages/compiler/src/i18n/message_bundle.ts | 5 ++--- .../src/ml_parser/html_whitespaces.ts | 19 ++++++++++++++++--- .../compiler/src/render3/view/i18n/meta.ts | 4 ++-- .../compiler/src/render3/view/template.ts | 13 ++++++++++++- packages/compiler/test/render3/view/util.ts | 4 ++-- 5 files changed, 34 insertions(+), 11 deletions(-) diff --git a/packages/compiler/src/i18n/message_bundle.ts b/packages/compiler/src/i18n/message_bundle.ts index f3ed1faedc49e..534eefb1dae24 100644 --- a/packages/compiler/src/i18n/message_bundle.ts +++ b/packages/compiler/src/i18n/message_bundle.ts @@ -6,10 +6,9 @@ * found in the LICENSE file at https://angular.io/license */ -import * as html from '../ml_parser/ast'; import {InterpolationConfig} from '../ml_parser/defaults'; import {HtmlParser} from '../ml_parser/html_parser'; -import {WhitespaceVisitor} from '../ml_parser/html_whitespaces'; +import {WhitespaceVisitor, visitAllWithSiblings} from '../ml_parser/html_whitespaces'; import {ParseError} from '../parse_util'; import {extractMessages} from './extractor_merger'; @@ -49,7 +48,7 @@ export class MessageBundle { // affected message IDs. const rootNodes = this._preserveWhitespace ? htmlParserResult.rootNodes - : html.visitAll( + : visitAllWithSiblings( new WhitespaceVisitor(/* preserveSignificantWhitespace */ false), htmlParserResult.rootNodes, ); diff --git a/packages/compiler/src/ml_parser/html_whitespaces.ts b/packages/compiler/src/ml_parser/html_whitespaces.ts index 3e6af5dc9e244..6813aff8d4b0f 100644 --- a/packages/compiler/src/ml_parser/html_whitespaces.ts +++ b/packages/compiler/src/ml_parser/html_whitespaces.ts @@ -63,6 +63,7 @@ export class WhitespaceVisitor implements html.Visitor { constructor( private readonly preserveSignificantWhitespace: boolean, private readonly originalNodeMap?: Map, + private readonly requireContext = true, ) {} visitElement(element: html.Element, context: any): any { @@ -71,7 +72,7 @@ export class WhitespaceVisitor implements html.Visitor { // but still visit all attributes to eliminate one used as a market to preserve WS const newElement = new html.Element( element.name, - html.visitAll(this, element.attrs), + visitAllWithSiblings(this, element.attrs), element.children, element.sourceSpan, element.startSourceSpan, @@ -202,6 +203,18 @@ export class WhitespaceVisitor implements html.Visitor { visitLetDeclaration(decl: html.LetDeclaration, context: any) { return decl; } + + visit(_node: html.Node, context: any) { + // `visitAllWithSiblings` provides context necessary for ICU messages to be handled correctly. + // Prefer that over calling `html.visitAll` directly on this visitor. + if (this.requireContext && !context) { + throw new Error( + `WhitespaceVisitor requires context. Visit via \`visitAllWithSiblings\` to get this context.`, + ); + } + + return false; + } } function trimLeadingWhitespace( @@ -261,7 +274,7 @@ export function removeWhitespaces( preserveSignificantWhitespace: boolean, ): ParseTreeResult { return new ParseTreeResult( - html.visitAll( + visitAllWithSiblings( new WhitespaceVisitor(preserveSignificantWhitespace), htmlAstWithErrors.rootNodes, ), @@ -274,7 +287,7 @@ interface SiblingVisitorContext { next: html.Node | undefined; } -function visitAllWithSiblings(visitor: WhitespaceVisitor, nodes: html.Node[]): any[] { +export function visitAllWithSiblings(visitor: WhitespaceVisitor, nodes: html.Node[]): any[] { const result: any[] = []; nodes.forEach((ast, i) => { diff --git a/packages/compiler/src/render3/view/i18n/meta.ts b/packages/compiler/src/render3/view/i18n/meta.ts index e1786d1668a24..1c55eb1217595 100644 --- a/packages/compiler/src/render3/view/i18n/meta.ts +++ b/packages/compiler/src/render3/view/i18n/meta.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {WhitespaceVisitor} from '../../../ml_parser/html_whitespaces'; +import {WhitespaceVisitor, visitAllWithSiblings} from '../../../ml_parser/html_whitespaces'; import {computeDecimalDigest, computeDigest, decimalDigest} from '../../../i18n/digest'; import * as i18n from '../../../i18n/i18n_ast'; import {createI18nMessageFactory, VisitNodeFn} from '../../../i18n/i18n_parser'; @@ -129,7 +129,7 @@ export class I18nMetaVisitor implements html.Visitor { const originalNodeMap = new Map(); const trimmedNodes = this.preserveSignificantWhitespace ? element.children - : html.visitAll( + : visitAllWithSiblings( new WhitespaceVisitor(false /* preserveSignificantWhitespace */, originalNodeMap), element.children, ); diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 555396acdec17..6fb920b10a39e 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -237,8 +237,19 @@ export function parseTemplate( // and `$localize` calls which should retain significant whitespace in order to render the // correct output. We let this diverge from the message IDs generated earlier which might not // have preserved significant whitespace. + // + // This should use `visitAllWithSiblings` to set `WhitespaceVisitor` context correctly, however + // there is an existing bug where significant whitespace is not properly retained in the JS + // output of leading/trailing whitespace for ICU messages due to the existing lack of context\ + // in `WhitespaceVisitor`. Using `visitAllWithSiblings` here would fix that bug and retain the + // whitespace, however it would also change the runtime representation which we don't want to do + // right now. rootNodes = html.visitAll( - new WhitespaceVisitor(/* preserveSignificantWhitespace */ true), + new WhitespaceVisitor( + /* preserveSignificantWhitespace */ true, + /* originalNodeMap */ undefined, + /* requireContext */ false, + ), rootNodes, ); diff --git a/packages/compiler/test/render3/view/util.ts b/packages/compiler/test/render3/view/util.ts index 9281b7144dacd..041f084d6e5dd 100644 --- a/packages/compiler/test/render3/view/util.ts +++ b/packages/compiler/test/render3/view/util.ts @@ -12,7 +12,7 @@ import {Parser} from '../../../src/expression_parser/parser'; import * as html from '../../../src/ml_parser/ast'; import {DEFAULT_INTERPOLATION_CONFIG, InterpolationConfig} from '../../../src/ml_parser/defaults'; import {HtmlParser} from '../../../src/ml_parser/html_parser'; -import {WhitespaceVisitor} from '../../../src/ml_parser/html_whitespaces'; +import {WhitespaceVisitor, visitAllWithSiblings} from '../../../src/ml_parser/html_whitespaces'; import {ParseTreeResult} from '../../../src/ml_parser/parser'; import * as a from '../../../src/render3/r3_ast'; import {htmlAstToRender3Ast, Render3ParseResult} from '../../../src/render3/r3_template_transform'; @@ -164,7 +164,7 @@ export function parseR3( let htmlNodes = processI18nMeta(parseResult).rootNodes; if (!options.preserveWhitespaces) { - htmlNodes = html.visitAll( + htmlNodes = visitAllWithSiblings( new WhitespaceVisitor(true /* preserveSignificantWhitespace */), htmlNodes, ); From 50f08e6c4bf1caeeb08d3505ce7fabd466b9c76b Mon Sep 17 00:00:00 2001 From: Alex Castle Date: Wed, 21 Aug 2024 15:47:41 -0700 Subject: [PATCH 16/36] feat(common): automatically use sizes auto in NgOptimizedImage (#57479) Prepend 'auto' to value of the `sizes` prop generated by NgOptimizedImage, when image has a responsive srcset and is lazy-loaded. PR Close #57479 --- adev/src/content/guide/image-optimization.md | 2 + .../ng_optimized_image/ng_optimized_image.ts | 15 +++- .../directives/ng_optimized_image_spec.ts | 73 ++++++++++++++++++- 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/adev/src/content/guide/image-optimization.md b/adev/src/content/guide/image-optimization.md index 44c82a78cdacd..2d80daa07775a 100644 --- a/adev/src/content/guide/image-optimization.md +++ b/adev/src/content/guide/image-optimization.md @@ -248,6 +248,8 @@ If you haven't used `sizes` before, a good place to start is to set it based on If you find that the above does not cover your desired image behavior, see the documentation on [advanced sizes values](#advanced-sizes-values). +Note that `NgOptimizedImage` automatically prepends `"auto"` to the provided `sizes` value. This is an optimization that increases the accuracy of srcset selection on browsers which support `sizes="auto"`, and is ignored by browsers which do not. + By default, the responsive breakpoints are: `[16, 32, 48, 64, 96, 128, 256, 384, 640, 750, 828, 1080, 1200, 1920, 2048, 3840]` diff --git a/packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts b/packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts index d40f950b0337a..38dc8ba52d3e7 100644 --- a/packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts +++ b/packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts @@ -492,8 +492,21 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy { const rewrittenSrcset = this.updateSrcAndSrcset(); if (this.sizes) { - this.setHostAttribute('sizes', this.sizes); + if (this.getLoadingBehavior() === 'lazy') { + this.setHostAttribute('sizes', 'auto, ' + this.sizes); + } else { + this.setHostAttribute('sizes', this.sizes); + } + } else { + if ( + this.ngSrcset && + VALID_WIDTH_DESCRIPTOR_SRCSET.test(this.ngSrcset) && + this.getLoadingBehavior() === 'lazy' + ) { + this.setHostAttribute('sizes', 'auto, 100vw'); + } } + if (this.isServer && this.priority) { this.preloadLinkCreator.createPreloadLinkTag( this.renderer, diff --git a/packages/common/test/directives/ng_optimized_image_spec.ts b/packages/common/test/directives/ng_optimized_image_spec.ts index 6eb03caacc6cb..9181ed28a2119 100644 --- a/packages/common/test/directives/ng_optimized_image_spec.ts +++ b/packages/common/test/directives/ng_optimized_image_spec.ts @@ -1027,7 +1027,7 @@ describe('Image directive', () => { it('should add default sizes value in fill mode', () => { setupTestingModule(); - const template = ''; + const template = ''; const fixture = createTestComponent(template); fixture.detectChanges(); @@ -1035,10 +1035,21 @@ describe('Image directive', () => { const img = nativeElement.querySelector('img')!; expect(img.getAttribute('sizes')).toBe('100vw'); }); + it('should add auto sizes to default in fill mode when lazy', () => { + setupTestingModule(); + + const template = ''; + + const fixture = createTestComponent(template); + fixture.detectChanges(); + const nativeElement = fixture.nativeElement as HTMLElement; + const img = nativeElement.querySelector('img')!; + expect(img.getAttribute('sizes')).toBe('auto, 100vw'); + }); it('should not overwrite sizes value in fill mode', () => { setupTestingModule(); - const template = ''; + const template = ''; const fixture = createTestComponent(template); fixture.detectChanges(); @@ -1046,6 +1057,17 @@ describe('Image directive', () => { const img = nativeElement.querySelector('img')!; expect(img.getAttribute('sizes')).toBe('50vw'); }); + it('should prepend "auto" to sizes in fill mode when lazy', () => { + setupTestingModule(); + + const template = ''; + + const fixture = createTestComponent(template); + fixture.detectChanges(); + const nativeElement = fixture.nativeElement as HTMLElement; + const img = nativeElement.querySelector('img')!; + expect(img.getAttribute('sizes')).toBe('auto, 50vw'); + }); it('should cause responsive srcset to be generated in fill mode', () => { setupTestingModule(); @@ -2022,6 +2044,34 @@ describe('Image directive', () => { `${IMG_BASE_URL}/img.png?w=300 3x`, ); }); + it('should automatically set a default sizes attribute when ngSrcset is used with a responsive srcset and is lazy', () => { + setupTestingModule({imageLoader}); + + const template = ` + + `; + const fixture = createTestComponent(template); + fixture.detectChanges(); + + const nativeElement = fixture.nativeElement as HTMLElement; + const img = nativeElement.querySelector('img')!; + expect(img.src).toBe(`${IMG_BASE_URL}/img.png`); + expect(img.sizes).toBe(`auto, 100vw`); + }); + it('should not automatically set a default sizes attribute when ngSrcset is used with a responsive srcset and is not lazy', () => { + setupTestingModule({imageLoader}); + + const template = ` + + `; + const fixture = createTestComponent(template); + fixture.detectChanges(); + + const nativeElement = fixture.nativeElement as HTMLElement; + const img = nativeElement.querySelector('img')!; + expect(img.src).toBe(`${IMG_BASE_URL}/img.png`); + expect(img.sizes).toBe(''); + }); }); describe('sizes attribute', () => { @@ -2030,7 +2080,7 @@ describe('Image directive', () => { const template = ''; + 'sizes="(max-width: 768px) 100vw, (max-width: 1200px) 50vw, 33vw" priority>'; const fixture = createTestComponent(template); fixture.detectChanges(); @@ -2042,6 +2092,23 @@ describe('Image directive', () => { ); }); + it('should prepend sizes="auto" to a lazy-loaded image', () => { + setupTestingModule(); + + const template = + ''; + const fixture = createTestComponent(template); + fixture.detectChanges(); + + const nativeElement = fixture.nativeElement as HTMLElement; + const img = nativeElement.querySelector('img')!; + + expect(img.getAttribute('sizes')).toBe( + 'auto, (max-width: 768px) 100vw, (max-width: 1200px) 50vw, 33vw', + ); + }); + it('should throw if a complex `sizes` is used', () => { setupTestingModule(); From 7b1e5be20b99c88246c6be78a4dcd64eb55cee1a Mon Sep 17 00:00:00 2001 From: Chintan Kavathia Date: Thu, 22 Aug 2024 11:16:44 +0530 Subject: [PATCH 17/36] fix(core): fallback to default ng-content with empty projectable nodes. (#57480) BREAKING CHANGE: Render default fallback with empty `projectableNodes`. When passing an empty array to `projectableNodes` in the `createComponent` API, the default fallback content of the `ng-content` will be rendered if present. To prevent rendering the default content, pass `document.createTextNode('')` as a `projectableNode`. For example: ```ts // The first ng-content will render the default fallback content if present createComponent(MyComponent. { projectableNodes: [[], [secondNode]] }); // To prevent projecting the default fallback content: createComponent(MyComponent. { projectableNodes: [[document.createTextNode('')], [secondNode]] }); ``` Fixes #57471 PR Close #57480 --- packages/core/src/render3/component_ref.ts | 2 +- packages/core/test/acceptance/content_spec.ts | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/packages/core/src/render3/component_ref.ts b/packages/core/src/render3/component_ref.ts index d67271a7b4a60..01b07325f1bb9 100644 --- a/packages/core/src/render3/component_ref.ts +++ b/packages/core/src/render3/component_ref.ts @@ -670,7 +670,7 @@ function projectNodes( // complex checks down the line. // We also normalize the length of the passed in projectable nodes (to match the number of // slots defined by a component). - projection.push(nodesforSlot != null ? Array.from(nodesforSlot) : null); + projection.push(nodesforSlot != null && nodesforSlot.length ? Array.from(nodesforSlot) : null); } } diff --git a/packages/core/test/acceptance/content_spec.ts b/packages/core/test/acceptance/content_spec.ts index 59fb8effb9b6e..93f800508aeed 100644 --- a/packages/core/test/acceptance/content_spec.ts +++ b/packages/core/test/acceptance/content_spec.ts @@ -1670,6 +1670,32 @@ describe('projection', () => { expect(getElementHtml(hostElement)).toContain('

override

|Two fallback|Three fallback'); }); + it('should render the content through projectableNodes along with fallback', () => { + @Component({ + standalone: true, + template: + `One fallback|` + + `Two fallback|Three fallback`, + }) + class Projection {} + + const hostElement = document.createElement('div'); + const environmentInjector = TestBed.inject(EnvironmentInjector); + const paragraph = document.createElement('p'); + paragraph.textContent = 'override'; + const secondParagraph = document.createElement('p'); + secondParagraph.textContent = 'override'; + const projectableNodes = [[paragraph], [], [secondParagraph]]; + const componentRef = createComponent(Projection, { + hostElement, + environmentInjector, + projectableNodes, + }); + componentRef.changeDetectorRef.detectChanges(); + + expect(getElementHtml(hostElement)).toContain('

override

|Two fallback|

override

'); + }); + it('should render fallback content when ng-content is inside an ng-template', () => { @Component({ selector: 'projection', From d73f2e91a22e77f5879e164c40a5d2eb6fb70333 Mon Sep 17 00:00:00 2001 From: Matthieu Riegler Date: Mon, 26 Aug 2024 18:21:57 +0200 Subject: [PATCH 18/36] refactor(animations): Add loading strategy for the Async Animations (#57493) In some cases apps need to schedule a bit later the loading of the animation module. This private token will allow to investigate which other strategy could be useful. PR Close #57493 --- .../async/src/async_animation_renderer.ts | 25 ++++++++- .../animations/async/src/private_export.ts | 5 +- .../animations/async/src/providers.ts | 1 + .../async/test/animation_renderer_spec.ts | 53 ++++++++++++++++++- 4 files changed, 80 insertions(+), 4 deletions(-) diff --git a/packages/platform-browser/animations/async/src/async_animation_renderer.ts b/packages/platform-browser/animations/async/src/async_animation_renderer.ts index 7834918520d56..8531f79e11554 100644 --- a/packages/platform-browser/animations/async/src/async_animation_renderer.ts +++ b/packages/platform-browser/animations/async/src/async_animation_renderer.ts @@ -24,6 +24,8 @@ import { ɵChangeDetectionScheduler as ChangeDetectionScheduler, ɵNotificationSource as NotificationSource, ɵRuntimeError as RuntimeError, + Injector, + InjectionToken, } from '@angular/core'; import {ɵRuntimeErrorCode as RuntimeErrorCode} from '@angular/platform-browser'; @@ -33,6 +35,9 @@ const ANIMATION_PREFIX = '@'; export class AsyncAnimationRendererFactory implements OnDestroy, RendererFactory2 { private _rendererFactoryPromise: Promise | null = null; private readonly scheduler = inject(ChangeDetectionScheduler, {optional: true}); + private readonly loadingSchedulerFn = inject(ɵASYNC_ANIMATION_LOADING_SCHEDULER_FN, { + optional: true, + }); private _engine?: AnimationEngine; /** @@ -68,9 +73,16 @@ export class AsyncAnimationRendererFactory implements OnDestroy, RendererFactory // Note on the `.then(m => m)` part below: Closure compiler optimizations in g3 require // `.then` to be present for a dynamic import (or an import should be `await`ed) to detect // the set of imported symbols. - const moduleImpl = this.moduleImpl ?? import('@angular/animations/browser').then((m) => m); + const loadFn = () => this.moduleImpl ?? import('@angular/animations/browser').then((m) => m); - return moduleImpl + let moduleImplPromise: typeof this.moduleImpl; + if (this.loadingSchedulerFn) { + moduleImplPromise = this.loadingSchedulerFn(loadFn); + } else { + moduleImplPromise = loadFn(); + } + + return moduleImplPromise .catch((e) => { throw new RuntimeError( RuntimeErrorCode.ANIMATION_RENDERER_ASYNC_LOADING_FAILURE, @@ -280,3 +292,12 @@ export class DynamicDelegationRenderer implements Renderer2 { return this.replay !== null && propOrEventName.startsWith(ANIMATION_PREFIX); } } + +/** + * Provides a custom scheduler function for the async loading of the animation package. + * + * Private token for investigation purposes + */ +export const ɵASYNC_ANIMATION_LOADING_SCHEDULER_FN = new InjectionToken<(loadFn: () => T) => T>( + ngDevMode ? 'async_animation_loading_scheduler_fn' : '', +); diff --git a/packages/platform-browser/animations/async/src/private_export.ts b/packages/platform-browser/animations/async/src/private_export.ts index c1a4e49c8ffaa..21b0d80b9bb6a 100644 --- a/packages/platform-browser/animations/async/src/private_export.ts +++ b/packages/platform-browser/animations/async/src/private_export.ts @@ -6,4 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -export {AsyncAnimationRendererFactory as ɵAsyncAnimationRendererFactory} from './async_animation_renderer'; +export { + AsyncAnimationRendererFactory as ɵAsyncAnimationRendererFactory, + ɵASYNC_ANIMATION_LOADING_SCHEDULER_FN, +} from './async_animation_renderer'; diff --git a/packages/platform-browser/animations/async/src/providers.ts b/packages/platform-browser/animations/async/src/providers.ts index 78f85c765e676..15bf2561428fb 100644 --- a/packages/platform-browser/animations/async/src/providers.ts +++ b/packages/platform-browser/animations/async/src/providers.ts @@ -14,6 +14,7 @@ import { NgZone, RendererFactory2, ɵperformanceMarkFeature as performanceMarkFeature, + InjectionToken, } from '@angular/core'; import {ɵDomRendererFactory2 as DomRendererFactory2} from '@angular/platform-browser'; diff --git a/packages/platform-browser/animations/async/test/animation_renderer_spec.ts b/packages/platform-browser/animations/async/test/animation_renderer_spec.ts index d0cd8a14690dc..217a2eda6effb 100644 --- a/packages/platform-browser/animations/async/test/animation_renderer_spec.ts +++ b/packages/platform-browser/animations/async/test/animation_renderer_spec.ts @@ -21,14 +21,17 @@ import { } from '@angular/animations/browser'; import {DOCUMENT} from '@angular/common'; import { + afterNextRender, ANIMATION_MODULE_TYPE, Component, + inject, Injectable, + Injector, NgZone, RendererFactory2, RendererType2, + runInInjectionContext, ViewChild, - ɵChangeDetectionScheduler as ChangeDetectionScheduler, } from '@angular/core'; import {TestBed} from '@angular/core/testing'; import {ɵDomRendererFactory2 as DomRendererFactory2} from '@angular/platform-browser'; @@ -38,7 +41,9 @@ import {el} from '@angular/platform-browser/testing/src/browser_util'; import { AsyncAnimationRendererFactory, DynamicDelegationRenderer, + ɵASYNC_ANIMATION_LOADING_SCHEDULER_FN, } from '../src/async_animation_renderer'; +import {provideAnimationsAsync} from '../public_api'; type AnimationBrowserModule = typeof import('@angular/animations/browser'); @@ -393,6 +398,52 @@ type AnimationBrowserModule = typeof import('@angular/animations/browser'); expect(engine.players.length).toEqual(1); }); }); + + describe('custom scheduling', () => { + it('should be able to use a custom loading scheduler', async () => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [ + provideAnimationsAsync(), + { + provide: ɵASYNC_ANIMATION_LOADING_SCHEDULER_FN, + useFactory: () => { + const injector = inject(Injector); + return (loadFn: () => T) => { + return new Promise((res) => { + runInInjectionContext(injector, () => afterNextRender(() => res(loadFn()))); + }); + }; + }, + }, + ], + }); + const renderer = await makeRenderer(); + expect(renderer).toBeInstanceOf(DynamicDelegationRenderer); + expect(renderer['delegate']).toBeInstanceOf(AnimationRenderer); + }); + + it('should handle scheduling error', async () => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [ + provideAnimationsAsync(), + { + provide: ɵASYNC_ANIMATION_LOADING_SCHEDULER_FN, + useValue: () => { + throw new Error('SchedulingError'); + }, + }, + ], + }); + + try { + await makeRenderer(); + } catch (err) { + expect((err as Error).message).toBe('SchedulingError'); + } + }); + }); }); })(); From 5fed53a57a780794b53ae0bbe19fac3130ca36d1 Mon Sep 17 00:00:00 2001 From: Angular Robot Date: Sun, 25 Aug 2024 11:03:54 +0000 Subject: [PATCH 19/36] build: update dependency jsdom to v25 (#57514) See associated pull request for more information. PR Close #57514 --- adev/shared-docs/package.json | 2 +- package.json | 2 +- yarn.lock | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/adev/shared-docs/package.json b/adev/shared-docs/package.json index 6c862bc9869d4..9fe505ed05bb6 100644 --- a/adev/shared-docs/package.json +++ b/adev/shared-docs/package.json @@ -19,7 +19,7 @@ "fast-glob": "~3.3.2", "fflate": "^0.8.2", "html-entities": "~2.5.2", - "jsdom": "~24.1.0", + "jsdom": "~25.0.0", "marked": "~14.0.0", "mermaid": "^11.0.0", "shiki": "^1.10.3" diff --git a/package.json b/package.json index 18868c4c69e8d..b9365801cfd59 100644 --- a/package.json +++ b/package.json @@ -206,7 +206,7 @@ "gulp-conventional-changelog": "^5.0.0", "html-entities": "^2.5.2", "husky": "9.1.4", - "jsdom": "^24.0.0", + "jsdom": "^25.0.0", "karma-coverage": "^2.2.1", "karma-jasmine-html-reporter": "^2.1.0", "karma-sauce-launcher": "^4.3.6", diff --git a/yarn.lock b/yarn.lock index 7d318b37cd0de..2a528ae51ff7c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -11567,10 +11567,10 @@ jsbn@~0.1.0: resolved "https://registry.yarnpkg.com/jsbn/-/jsbn-0.1.1.tgz#a5e654c2e5a2deb5f201d96cefbca80c0ef2f513" integrity sha512-UVU9dibq2JcFWxQPA6KCqj5O42VOmAY3zQUfEKxU0KpTGXwNoCjkX1e13eHNvw/xPynt6pU0rZ1htjWTNTSXsg== -jsdom@^24.0.0: - version "24.1.1" - resolved "https://registry.yarnpkg.com/jsdom/-/jsdom-24.1.1.tgz#f41df8f4f3b2fbfa7e1bdc5df62c9804fd14a9d0" - integrity sha512-5O1wWV99Jhq4DV7rCLIoZ/UIhyQeDR7wHVyZAHAshbrvZsLs+Xzz7gtwnlJTJDjleiTKh54F4dXrX70vJQTyJQ== +jsdom@^25.0.0: + version "25.0.0" + resolved "https://registry.yarnpkg.com/jsdom/-/jsdom-25.0.0.tgz#d1612b4ddab85af56821b2f731e15faae135f4e1" + integrity sha512-OhoFVT59T7aEq75TVw9xxEfkXgacpqAhQaYgP9y/fDqWQCMB/b1H66RfmPm/MaeaAIU9nDwMOVTlPN51+ao6CQ== dependencies: cssstyle "^4.0.1" data-urls "^5.0.0" From 99e40574cb9729a64b8dde3ebc1dbe32b79f52f5 Mon Sep 17 00:00:00 2001 From: AleksanderBodurri Date: Sun, 25 Aug 2024 20:09:43 -0400 Subject: [PATCH 20/36] fix(devtools): ignore DOM Nodes from other frames when performing render tree detection (#57518) Previously, if an application had DOM Nodes injected into it from other frames, DevTools would fail to parse component trees with the render tree strategy properly because of an instanceof Node check that the framework performs. Now we check for instanceof Node before even calling framework debug APIs on DOM nodes so that we can skip nodes that come from other frames entirely. PR Close #57518 --- .../src/lib/directive-forest/render-tree.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/devtools/projects/ng-devtools-backend/src/lib/directive-forest/render-tree.ts b/devtools/projects/ng-devtools-backend/src/lib/directive-forest/render-tree.ts index 754c2c321e7de..5f5c84ae17167 100644 --- a/devtools/projects/ng-devtools-backend/src/lib/directive-forest/render-tree.ts +++ b/devtools/projects/ng-devtools-backend/src/lib/directive-forest/render-tree.ts @@ -19,6 +19,11 @@ const extractViewTree = ( getComponent: (element: Element) => {} | null, getDirectives: (node: Node) => {}[], ): ComponentTreeNode[] => { + // Ignore DOM Node if it came from a different frame. Use instanceof Node to check this. + if (!(domNode instanceof Node)) { + return result; + } + const directives = getDirectives(domNode); if (!directives.length && !(domNode instanceof Element)) { return result; From 6f5b435a6932fde69d6783921fc84a9dd7670c90 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 26 Aug 2024 15:04:13 +0000 Subject: [PATCH 21/36] refactor(migrations): initial migration logic for converting to signal queries (#57525) Adds initial migration logic to convert decorator query declarations to signal queries. We will re-use more part of the signal input migration in follow-ups, to properly migrate, and e.g. even handle control flow PR Close #57525 --- .../signal-migration/src/BUILD.bazel | 2 +- .../src/utils/remove_from_union.ts | 6 +- .../signal-queries-migration/BUILD.bazel | 41 +++++ .../convert_query_property.ts | 133 +++++++++++++++ .../identify_queries.ts | 119 +++++++++++++ .../migration.spec.ts | 122 ++++++++++++++ .../signal-queries-migration/migration.ts | 158 ++++++++++++++++++ .../reference_tracking.ts | 30 ++++ .../core/schematics/utils/tsurge/BUILD.bazel | 3 + .../schematics/utils/tsurge/testing/diff.ts | 34 ++++ .../schematics/utils/tsurge/testing/index.ts | 5 +- 11 files changed, 647 insertions(+), 6 deletions(-) create mode 100644 packages/core/schematics/migrations/signal-queries-migration/BUILD.bazel create mode 100644 packages/core/schematics/migrations/signal-queries-migration/convert_query_property.ts create mode 100644 packages/core/schematics/migrations/signal-queries-migration/identify_queries.ts create mode 100644 packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts create mode 100644 packages/core/schematics/migrations/signal-queries-migration/migration.ts create mode 100644 packages/core/schematics/migrations/signal-queries-migration/reference_tracking.ts create mode 100644 packages/core/schematics/utils/tsurge/testing/diff.ts diff --git a/packages/core/schematics/migrations/signal-migration/src/BUILD.bazel b/packages/core/schematics/migrations/signal-migration/src/BUILD.bazel index 3f69ccbd43bf7..43f06da29d2b0 100644 --- a/packages/core/schematics/migrations/signal-migration/src/BUILD.bazel +++ b/packages/core/schematics/migrations/signal-migration/src/BUILD.bazel @@ -7,7 +7,7 @@ ts_library( exclude = ["test/**"], ), visibility = [ - "//packages/core/schematics/migrations/signal-migration/src/batch:__pkg__", + "//packages/core/schematics/migrations/signal-queries-migration:__pkg__", "//packages/language-service:__subpackages__", ], deps = [ diff --git a/packages/core/schematics/migrations/signal-migration/src/utils/remove_from_union.ts b/packages/core/schematics/migrations/signal-migration/src/utils/remove_from_union.ts index 43c8df19f2623..0d4765b0e0651 100644 --- a/packages/core/schematics/migrations/signal-migration/src/utils/remove_from_union.ts +++ b/packages/core/schematics/migrations/signal-migration/src/utils/remove_from_union.ts @@ -11,10 +11,14 @@ import ts from 'typescript'; export function removeFromUnionIfPossible( union: ts.UnionTypeNode, filter: (v: ts.TypeNode) => boolean, -): ts.UnionTypeNode { +): ts.TypeNode { const filtered = union.types.filter(filter); if (filtered.length === union.types.length) { return union; } + // If there is only item at this point, avoid the union structure. + if (filtered.length === 1) { + return filtered[0]; + } return ts.factory.updateUnionTypeNode(union, ts.factory.createNodeArray(filtered)); } diff --git a/packages/core/schematics/migrations/signal-queries-migration/BUILD.bazel b/packages/core/schematics/migrations/signal-queries-migration/BUILD.bazel new file mode 100644 index 0000000000000..067d34a58fec6 --- /dev/null +++ b/packages/core/schematics/migrations/signal-queries-migration/BUILD.bazel @@ -0,0 +1,41 @@ +load("//tools:defaults.bzl", "jasmine_node_test", "ts_library") + +ts_library( + name = "migration", + srcs = glob( + ["**/*.ts"], + exclude = ["*.spec.ts"], + ), + deps = [ + "//packages/compiler", + "//packages/compiler-cli", + "//packages/compiler-cli/private", + "//packages/compiler-cli/src/ngtsc/annotations", + "//packages/compiler-cli/src/ngtsc/annotations/directive", + "//packages/compiler-cli/src/ngtsc/reflection", + "//packages/core/schematics/migrations/signal-migration/src", + "//packages/core/schematics/utils/tsurge", + "@npm//@types/node", + "@npm//typescript", + ], +) + +ts_library( + name = "test_lib", + testonly = True, + srcs = glob( + ["**/*.spec.ts"], + ), + deps = [ + ":migration", + "//packages/compiler-cli", + "//packages/compiler-cli/src/ngtsc/file_system/testing", + "//packages/core/schematics/utils/tsurge", + ], +) + +jasmine_node_test( + name = "test", + srcs = [":test_lib"], + env = {"FORCE_COLOR": "3"}, +) diff --git a/packages/core/schematics/migrations/signal-queries-migration/convert_query_property.ts b/packages/core/schematics/migrations/signal-queries-migration/convert_query_property.ts new file mode 100644 index 0000000000000..5af12d3fcb997 --- /dev/null +++ b/packages/core/schematics/migrations/signal-queries-migration/convert_query_property.ts @@ -0,0 +1,133 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import ts from 'typescript'; +import {ExtractedQuery} from './identify_queries'; +import {Replacement, TextUpdate} from '../../utils/tsurge'; +import {absoluteFromSourceFile} from '../../../../compiler-cli'; +import {ImportManager} from '../../../../compiler-cli/private/migrations'; +import assert from 'assert'; +import {WrappedNodeExpr} from '@angular/compiler'; +import {removeFromUnionIfPossible} from '../signal-migration/src/utils/remove_from_union'; + +const printer = ts.createPrinter(); + +/** + * A few notes on changes: + * + * @ViewChild() + * --> static is gone! + * --> read stays + * + * @ViewChildren() + * --> emitDistinctChangesOnly is gone! + * --> read stays + * + * @ContentChild() + * --> descendants stays + * --> read stays + * --> static is gone! + * + * @ContentChildren() + * --> descendants stays + * --> read stays + * --> emitDistinctChangesOnly is gone! + */ +export function computeReplacementsToMigrateQuery( + node: ts.PropertyDeclaration, + metadata: ExtractedQuery, + importManager: ImportManager, +): Replacement[] { + const sf = node.getSourceFile(); + let newQueryFn = importManager.addImport({ + requestedFile: sf, + exportModuleSpecifier: '@angular/core', + exportSymbolName: metadata.kind, + }); + + // The default value for descendants is `true`, except for `ContentChildren`. + const defaultDescendants = metadata.kind !== 'contentChildren'; + const optionProperties: ts.PropertyAssignment[] = []; + const args: ts.Expression[] = [ + metadata.args[0], // Locator. + ]; + let type = node.type; + + if (metadata.queryInfo.read !== null) { + assert(metadata.queryInfo.read instanceof WrappedNodeExpr); + optionProperties.push( + ts.factory.createPropertyAssignment('read', metadata.queryInfo.read.node), + ); + } + if (metadata.queryInfo.descendants !== defaultDescendants) { + optionProperties.push( + ts.factory.createPropertyAssignment( + 'descendants', + metadata.queryInfo.descendants ? ts.factory.createTrue() : ts.factory.createFalse(), + ), + ); + } + + if (optionProperties.length > 0) { + args.push(ts.factory.createObjectLiteralExpression(optionProperties)); + } + + // TODO: Can we consult, based on references and non-null assertions? + const isIndicatedAsRequired = node.exclamationToken !== undefined; + + // If the query is required already via some indicators, and this is a "single" + // query, use the available `.required` method. + if (isIndicatedAsRequired && metadata.queryInfo.first) { + newQueryFn = ts.factory.createPropertyAccessExpression(newQueryFn, 'required'); + } + + // If this query is still nullable (i.e. not required), attempt to remove + // explicit `undefined` types if possible. + if (!isIndicatedAsRequired && type !== undefined && ts.isUnionTypeNode(type)) { + type = removeFromUnionIfPossible(type, (v) => v.kind !== ts.SyntaxKind.UndefinedKeyword); + } + + const locatorType = Array.isArray(metadata.queryInfo.predicate) + ? null + : metadata.queryInfo.predicate.expression; + const readType = metadata.queryInfo.read ?? locatorType; + + // If the type and the read type are matching, we can rely on the TS generic + // signature rather than repeating e.g. `viewChild