From 9e82559de4e99a1aedf645a05b01fc08d3f4b1b1 Mon Sep 17 00:00:00 2001 From: Krzysztof Platis Date: Tue, 8 Oct 2024 00:19:15 +0200 Subject: [PATCH 01/15] fix(platform-server): destroy `PlatformRef` when error happens during the `bootstrap()` phase (#58112) The `bootstrap()` phase might fail e.g. due to an rejected promise in some `APP_INIIALIZER`. If `PlatformRef` is not destroyed, then the main app's injector is not destroyed and therefore `ngOnDestroy` hooks of singleton services is not called on the end (failure) of SSR. This could lead to possible memory leaks in custom SSR apps, if their singleton services' `ngOnDestroy` hooks contained an important teardown logic (e.g. unsubscribing from RxJS observable). Note: I needed to fix by the way another thing too: now we destroy `moduleRef` when `platformInjector` is destroyed - by setting a `PLATFORM_DESTROY_LISTENER` fixes #58111 PR Close #58112 --- packages/core/src/platform/bootstrap.ts | 20 ++- packages/core/src/platform/platform_ref.ts | 6 +- packages/platform-server/src/utils.ts | 49 +++--- .../platform-server/test/integration_spec.ts | 150 ++++++++++++++++++ 4 files changed, 198 insertions(+), 27 deletions(-) diff --git a/packages/core/src/platform/bootstrap.ts b/packages/core/src/platform/bootstrap.ts index 88ef785f23068..c9d802754770d 100644 --- a/packages/core/src/platform/bootstrap.ts +++ b/packages/core/src/platform/bootstrap.ts @@ -26,21 +26,24 @@ import {Injector} from '../di'; import {InternalNgModuleRef, NgModuleRef} from '../linker/ng_module_factory'; import {stringify} from '../util/stringify'; -export interface ModuleBootstrapConfig { +export interface BootstrapConfig { + platformInjector: Injector; +} + +export interface ModuleBootstrapConfig extends BootstrapConfig { moduleRef: InternalNgModuleRef; allPlatformModules: NgModuleRef[]; } -export interface ApplicationBootstrapConfig { +export interface ApplicationBootstrapConfig extends BootstrapConfig { r3Injector: R3Injector; - platformInjector: Injector; rootComponent: Type | undefined; } function isApplicationBootstrapConfig( config: ApplicationBootstrapConfig | ModuleBootstrapConfig, ): config is ApplicationBootstrapConfig { - return !!(config as ApplicationBootstrapConfig).platformInjector; + return !(config as ModuleBootstrapConfig).moduleRef; } export function bootstrap( @@ -91,9 +94,9 @@ export function bootstrap( }); }); + // If the whole platform is destroyed, invoke the `destroy` method + // for all bootstrapped applications as well. if (isApplicationBootstrapConfig(config)) { - // If the whole platform is destroyed, invoke the `destroy` method - // for all bootstrapped applications as well. const destroyListener = () => envInjector.destroy(); const onPlatformDestroyListeners = config.platformInjector.get(PLATFORM_DESTROY_LISTENERS); onPlatformDestroyListeners.add(destroyListener); @@ -103,9 +106,14 @@ export function bootstrap( onPlatformDestroyListeners.delete(destroyListener); }); } else { + const destroyListener = () => config.moduleRef.destroy(); + const onPlatformDestroyListeners = config.platformInjector.get(PLATFORM_DESTROY_LISTENERS); + onPlatformDestroyListeners.add(destroyListener); + config.moduleRef.onDestroy(() => { remove(config.allPlatformModules, config.moduleRef); onErrorSubscription.unsubscribe(); + onPlatformDestroyListeners.delete(destroyListener); }); } diff --git a/packages/core/src/platform/platform_ref.ts b/packages/core/src/platform/platform_ref.ts index 600d00cf84c85..69d4eeef20354 100644 --- a/packages/core/src/platform/platform_ref.ts +++ b/packages/core/src/platform/platform_ref.ts @@ -79,7 +79,11 @@ export class PlatformRef { allAppProviders, ); - return bootstrap({moduleRef, allPlatformModules: this._modules}); + return bootstrap({ + moduleRef, + allPlatformModules: this._modules, + platformInjector: this.injector, + }); } /** diff --git a/packages/platform-server/src/utils.ts b/packages/platform-server/src/utils.ts index 060ecdaf4b3d7..9b6642c2eb881 100644 --- a/packages/platform-server/src/utils.ts +++ b/packages/platform-server/src/utils.ts @@ -210,18 +210,21 @@ async function _render(platformRef: PlatformRef, applicationRef: ApplicationRef) } appendServerContextInfo(applicationRef); - const output = platformState.renderToString(); - // Destroy the application in a macrotask, this allows pending promises to be settled and errors - // to be surfaced to the users. - await new Promise((resolve) => { + return platformState.renderToString(); +} + +/** + * Destroy the application in a macrotask, this allows pending promises to be settled and errors + * to be surfaced to the users. + */ +function asyncDestroyPlatform(platformRef: PlatformRef): Promise { + return new Promise((resolve) => { setTimeout(() => { platformRef.destroy(); resolve(); }, 0); }); - - return output; } /** @@ -264,9 +267,13 @@ export async function renderModule( ): Promise { const {document, url, extraProviders: platformProviders} = options; const platformRef = createServerPlatform({document, url, platformProviders}); - const moduleRef = await platformRef.bootstrapModule(moduleType); - const applicationRef = moduleRef.injector.get(ApplicationRef); - return _render(platformRef, applicationRef); + try { + const moduleRef = await platformRef.bootstrapModule(moduleType); + const applicationRef = moduleRef.injector.get(ApplicationRef); + return await _render(platformRef, applicationRef); + } finally { + await asyncDestroyPlatform(platformRef); + } } /** @@ -299,15 +306,17 @@ export async function renderApplication( startMeasuring(renderAppLabel); const platformRef = createServerPlatform(options); - - startMeasuring(bootstrapLabel); - const applicationRef = await bootstrap(); - stopMeasuring(bootstrapLabel); - - startMeasuring(_renderLabel); - const rendered = await _render(platformRef, applicationRef); - stopMeasuring(_renderLabel); - - stopMeasuring(renderAppLabel); - return rendered; + try { + startMeasuring(bootstrapLabel); + const applicationRef = await bootstrap(); + stopMeasuring(bootstrapLabel); + + startMeasuring(_renderLabel); + const rendered = await _render(platformRef, applicationRef); + stopMeasuring(_renderLabel); + return rendered; + } finally { + await asyncDestroyPlatform(platformRef); + stopMeasuring(renderAppLabel); + } } diff --git a/packages/platform-server/test/integration_spec.ts b/packages/platform-server/test/integration_spec.ts index dcfead07f56d4..b2facece402c5 100644 --- a/packages/platform-server/test/integration_spec.ts +++ b/packages/platform-server/test/integration_spec.ts @@ -42,6 +42,9 @@ import { ViewEncapsulation, ɵPendingTasks as PendingTasks, ɵwhenStable as whenStable, + APP_INITIALIZER, + inject, + getPlatform, } from '@angular/core'; import {SSR_CONTENT_INTEGRITY_MARKER} from '@angular/core/src/hydration/utils'; import {TestBed} from '@angular/core/testing'; @@ -1076,6 +1079,153 @@ class HiddenModule {} ); }, ); + + it( + `should call onOnDestroy of a service after a successful render` + + `(standalone: ${isStandalone})`, + async () => { + let wasServiceNgOnDestroyCalled = false; + + @Injectable({providedIn: 'root'}) + class DestroyableService { + ngOnDestroy() { + wasServiceNgOnDestroyCalled = true; + } + } + + const SuccessfulAppInitializerProviders = [ + { + provide: APP_INITIALIZER, + useFactory: () => { + inject(DestroyableService); + return () => Promise.resolve(); // Success in APP_INITIALIZER + }, + multi: true, + }, + ]; + + @NgModule({ + providers: SuccessfulAppInitializerProviders, + imports: [MyServerAppModule, ServerModule], + bootstrap: [MyServerApp], + }) + class ServerSuccessfulAppInitializerModule {} + + const ServerSuccessfulAppInitializerAppStandalone = getStandaloneBootstrapFn( + createMyServerApp(true), + SuccessfulAppInitializerProviders, + ); + + const options = {document: doc}; + const bootstrap = isStandalone + ? renderApplication(ServerSuccessfulAppInitializerAppStandalone, options) + : renderModule(ServerSuccessfulAppInitializerModule, options); + await bootstrap; + + expect(getPlatform()).withContext('PlatformRef should be destroyed').toBeNull(); + expect(wasServiceNgOnDestroyCalled) + .withContext('DestroyableService.ngOnDestroy() should be called') + .toBeTrue(); + }, + ); + + it( + `should call onOnDestroy of a service after some APP_INITIALIZER fails ` + + `(standalone: ${isStandalone})`, + async () => { + let wasServiceNgOnDestroyCalled = false; + + @Injectable({providedIn: 'root'}) + class DestroyableService { + ngOnDestroy() { + wasServiceNgOnDestroyCalled = true; + } + } + + const FailingAppInitializerProviders = [ + { + provide: APP_INITIALIZER, + useFactory: () => { + inject(DestroyableService); + return () => Promise.reject('Error in APP_INITIALIZER'); + }, + multi: true, + }, + ]; + + @NgModule({ + providers: FailingAppInitializerProviders, + imports: [MyServerAppModule, ServerModule], + bootstrap: [MyServerApp], + }) + class ServerFailingAppInitializerModule {} + + const ServerFailingAppInitializerAppStandalone = getStandaloneBootstrapFn( + createMyServerApp(true), + FailingAppInitializerProviders, + ); + + const options = {document: doc}; + const bootstrap = isStandalone + ? renderApplication(ServerFailingAppInitializerAppStandalone, options) + : renderModule(ServerFailingAppInitializerModule, options); + await expectAsync(bootstrap).toBeRejectedWith('Error in APP_INITIALIZER'); + + expect(getPlatform()).withContext('PlatformRef should be destroyed').toBeNull(); + expect(wasServiceNgOnDestroyCalled) + .withContext('DestroyableService.ngOnDestroy() should be called') + .toBeTrue(); + }, + ); + + it( + `should call onOnDestroy of a service after an error happens in a root component's constructor ` + + `(standalone: ${isStandalone})`, + async () => { + let wasServiceNgOnDestroyCalled = false; + + @Injectable({providedIn: 'root'}) + class DestroyableService { + ngOnDestroy() { + wasServiceNgOnDestroyCalled = true; + } + } + + @Component({ + standalone: isStandalone, + selector: 'app', + template: `Works!`, + }) + class MyServerFailingConstructorApp { + constructor() { + inject(DestroyableService); + throw 'Error in constructor of the root component'; + } + } + + @NgModule({ + declarations: [MyServerFailingConstructorApp], + imports: [MyServerAppModule, ServerModule], + bootstrap: [MyServerFailingConstructorApp], + }) + class MyServerFailingConstructorAppModule {} + + const MyServerFailingConstructorAppStandalone = getStandaloneBootstrapFn( + MyServerFailingConstructorApp, + ); + const options = {document: doc}; + const bootstrap = isStandalone + ? renderApplication(MyServerFailingConstructorAppStandalone, options) + : renderModule(MyServerFailingConstructorAppModule, options); + await expectAsync(bootstrap).toBeRejectedWith( + 'Error in constructor of the root component', + ); + expect(getPlatform()).withContext('PlatformRef should be destroyed').toBeNull(); + expect(wasServiceNgOnDestroyCalled) + .withContext('DestroyableService.ngOnDestroy() should be called') + .toBeTrue(); + }, + ); }); }); From e00775a0c9e0153188e5377b623742ca76f5a499 Mon Sep 17 00:00:00 2001 From: cexbrayat Date: Sat, 5 Oct 2024 11:21:34 +0200 Subject: [PATCH 02/15] docs: mention autoDetectChanges parameter default value (#58092) It was unclear whether the parameter was necessary, as its default value was not mentioned in the jdsoc. PR Close #58092 --- packages/core/testing/src/component_fixture.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/core/testing/src/component_fixture.ts b/packages/core/testing/src/component_fixture.ts index 350ced7d16155..7a01006c37408 100644 --- a/packages/core/testing/src/component_fixture.ts +++ b/packages/core/testing/src/component_fixture.ts @@ -176,6 +176,8 @@ export class ComponentFixture { * Set whether the fixture should autodetect changes. * * Also runs detectChanges once so that any existing change is detected. + * + * @param autoDetect Whether to autodetect changes. By default, `true`. */ autoDetectChanges(autoDetect = true): void { if (this._noZoneOptionIsSet && !this.zonelessEnabled) { From c1aa411cf13259d991c8f224a2bafc3e9763fe8d Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 9 Oct 2024 14:13:52 +0000 Subject: [PATCH 03/15] fix(migrations): properly resolve tsconfig paths on windows (#58137) The Angular CLI devkit and Tsurge, as well as TypeScript only deal with Posix paths. We also normalize paths into posix paths, and try to implement a devkit compatible virtual file system for the compiler-cli. This commit fixes an issue where we accidentally resolved `/` to the system root on Windows. e.g. `C:/`. This broke the posix and devkit paths throughout tsconfig parsing. This commit fixes this. Fixes #58132. PR Close #58137 --- .../tsurge/helpers/angular_devkit/devkit_filesystem.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/schematics/utils/tsurge/helpers/angular_devkit/devkit_filesystem.ts b/packages/core/schematics/utils/tsurge/helpers/angular_devkit/devkit_filesystem.ts index 31b9c58205efe..00b97fd384ca0 100644 --- a/packages/core/schematics/utils/tsurge/helpers/angular_devkit/devkit_filesystem.ts +++ b/packages/core/schematics/utils/tsurge/helpers/angular_devkit/devkit_filesystem.ts @@ -23,7 +23,7 @@ import { PathSegment, PathString, } from '@angular/compiler-cli/src/ngtsc/file_system'; -import * as path from 'node:path'; +import * as posixPath from 'node:path/posix'; /** * Angular compiler file system implementation that leverages an @@ -64,7 +64,7 @@ export class DevkitMigrationFilesystem implements FileSystem { } basename(filePath: string, extension?: string): PathSegment { - return path.basename(filePath, extension) as PathSegment; + return posixPath.basename(filePath, extension) as PathSegment; } normalize(path: T): T { @@ -76,7 +76,7 @@ export class DevkitMigrationFilesystem implements FileSystem { // In dev-kit, the NodeJS working directory should never be // considered, so `/` is the last resort over `cwd`. return this.normalize( - path.resolve(normalize('/'), ...normalizedPaths), + posixPath.resolve(normalize('/'), ...normalizedPaths), ) as string as AbsoluteFsPath; } From 2d1131491a479a1c79b45179b97e559976c48e0a Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 9 Oct 2024 14:18:26 +0000 Subject: [PATCH 04/15] refactor(migrations): improve statistic tracking of signal input migration (#58137) We should skip tracking information for inputs outside migration scope. PR Close #58137 --- .../migrations/signal-migration/src/migration.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/core/schematics/migrations/signal-migration/src/migration.ts b/packages/core/schematics/migrations/signal-migration/src/migration.ts index ad7fb8816bd29..5cc7b8a6b72c6 100644 --- a/packages/core/schematics/migrations/signal-migration/src/migration.ts +++ b/packages/core/schematics/migrations/signal-migration/src/migration.ts @@ -193,12 +193,24 @@ export class SignalInputMigration extends TsurgeComplexMigration< for (const [id, input] of Object.entries(globalMetadata.knownInputs)) { fullCompilationInputs++; - if (input.seenAsSourceInput) { - sourceInputs++; + + const isConsideredSourceInput = + input.seenAsSourceInput && + input.memberIncompatibility !== InputIncompatibilityReason.OutsideOfMigrationScope && + input.memberIncompatibility !== InputIncompatibilityReason.SkippedViaConfigFilter; + + // We won't track incompatibilities to inputs that aren't considered source inputs. + // Tracking their statistics wouldn't provide any value. + if (!isConsideredSourceInput) { + continue; } + + sourceInputs++; + if (input.memberIncompatibility !== null || input.owningClassIncompatibility !== null) { incompatibleInputs++; } + if (input.memberIncompatibility !== null) { const reasonName = InputIncompatibilityReason[input.memberIncompatibility]; const key = `input-field-incompatibility-${reasonName}` as const; From 66cca864895d58342376bfb35cdc8b903255b98a Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 9 Oct 2024 15:20:41 +0000 Subject: [PATCH 05/15] refactor(migrations): allow reuse of input incompatibility categorization in query migration (#58139) This commit moves the incompatibility categorization into a more common place, and renames it from Input incompatibilities to "field incompatibilities". This construct can then be used in the queries migration as well to give insight into why certain fields weren't migrated. PR Close #58139 --- .../src/batch/merge_unit_data.ts | 16 +-- .../signal-migration/src/batch/unit_data.ts | 6 +- .../signal-migration/src/best_effort_mode.ts | 13 +- .../src/convert-input/prepare_and_check.ts | 15 ++- .../migrations/signal-migration/src/index.ts | 14 +- .../src/input_detection/directive_info.ts | 9 +- .../src/input_detection/incompatibility.ts | 67 --------- .../input_detection/incompatibility_human.ts | 118 ---------------- .../src/input_detection/known_inputs.ts | 20 +-- .../signal-migration/src/migration.ts | 12 +- .../src/passes/1_identify_inputs.ts | 4 +- .../common_incompatible_patterns.ts | 4 +- .../problematic_patterns/incompatibility.ts | 65 +++++++++ .../incompatibility_human.ts | 127 ++++++++++++++++++ .../problematic_field_registry.ts | 10 +- .../src/pattern_advisors/spy_on_pattern.ts | 4 +- .../signal-migration/src/phase_analysis.ts | 8 +- .../src/utils/incompatibility_todos.ts | 25 ++-- .../signal-migration/test/golden.txt | 2 +- .../apply_input_refactoring.ts | 16 ++- 20 files changed, 286 insertions(+), 269 deletions(-) delete mode 100644 packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility.ts delete mode 100644 packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human.ts create mode 100644 packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility.ts create mode 100644 packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility_human.ts diff --git a/packages/core/schematics/migrations/signal-migration/src/batch/merge_unit_data.ts b/packages/core/schematics/migrations/signal-migration/src/batch/merge_unit_data.ts index 2df0cdf79cffe..f2b5da47f5dec 100644 --- a/packages/core/schematics/migrations/signal-migration/src/batch/merge_unit_data.ts +++ b/packages/core/schematics/migrations/signal-migration/src/batch/merge_unit_data.ts @@ -7,9 +7,9 @@ */ import { - InputIncompatibilityReason, - pickInputIncompatibility, -} from '../input_detection/incompatibility'; + FieldIncompatibilityReason, + pickFieldIncompatibility, +} from '../passes/problematic_patterns/incompatibility'; import {GraphNode, topologicalSort} from '../utils/inheritance_sort'; import {CompilationUnitData} from './unit_data'; @@ -58,7 +58,7 @@ export function mergeCompilationUnitData( } else { // Input might not be incompatible in one target, but others might invalidate it. // merge the incompatibility state. - existing.memberIncompatibility = pickInputIncompatibility( + existing.memberIncompatibility = pickFieldIncompatibility( {reason: info.memberIncompatibility, context: null}, {reason: existing.memberIncompatibility, context: null}, ).reason; @@ -100,8 +100,8 @@ export function mergeCompilationUnitData( // If parent is incompatible and not migrated, then this input // cannot be migrated either. Try propagating parent incompatibility then. if (isNodeIncompatible(parent.data)) { - node.data.info.memberIncompatibility = pickInputIncompatibility( - {reason: InputIncompatibilityReason.ParentIsIncompatible, context: null}, + node.data.info.memberIncompatibility = pickFieldIncompatibility( + {reason: FieldIncompatibilityReason.ParentIsIncompatible, context: null}, existingMemberIncompatibility, ).reason; break; @@ -118,8 +118,8 @@ export function mergeCompilationUnitData( ? {reason: info.memberIncompatibility, context: null} : null; - info.memberIncompatibility = pickInputIncompatibility( - {reason: InputIncompatibilityReason.OutsideOfMigrationScope, context: null}, + info.memberIncompatibility = pickFieldIncompatibility( + {reason: FieldIncompatibilityReason.OutsideOfMigrationScope, context: null}, existingMemberIncompatibility, ).reason; } diff --git a/packages/core/schematics/migrations/signal-migration/src/batch/unit_data.ts b/packages/core/schematics/migrations/signal-migration/src/batch/unit_data.ts index 2f64c28e7fb9b..9515bf6b5ae6f 100644 --- a/packages/core/schematics/migrations/signal-migration/src/batch/unit_data.ts +++ b/packages/core/schematics/migrations/signal-migration/src/batch/unit_data.ts @@ -8,8 +8,8 @@ import { ClassIncompatibilityReason, - InputIncompatibilityReason, -} from '../input_detection/incompatibility'; + FieldIncompatibilityReason, +} from '../passes/problematic_patterns/incompatibility'; import {ClassFieldUniqueKey} from '../passes/reference_resolution/known_fields'; /** @@ -24,7 +24,7 @@ export interface CompilationUnitData { // Use `string` here so that it's a usable index key. [inputIdKey: string]: { owningClassIncompatibility: ClassIncompatibilityReason | null; - memberIncompatibility: InputIncompatibilityReason | null; + memberIncompatibility: FieldIncompatibilityReason | null; seenAsSourceInput: boolean; extendsFrom: ClassFieldUniqueKey | null; }; diff --git a/packages/core/schematics/migrations/signal-migration/src/best_effort_mode.ts b/packages/core/schematics/migrations/signal-migration/src/best_effort_mode.ts index 483b7dfd3b764..5bd22b058070e 100644 --- a/packages/core/schematics/migrations/signal-migration/src/best_effort_mode.ts +++ b/packages/core/schematics/migrations/signal-migration/src/best_effort_mode.ts @@ -6,19 +6,20 @@ * found in the LICENSE file at https://angular.dev/license */ -import {InputIncompatibilityReason} from './input_detection/incompatibility'; +import {FieldIncompatibilityReason} from './passes/problematic_patterns/incompatibility'; import {KnownInputs} from './input_detection/known_inputs'; /** Input reasons that cannot be ignored. */ -export const nonIgnorableInputIncompatibilities: InputIncompatibilityReason[] = [ +export const nonIgnorableInputIncompatibilities: FieldIncompatibilityReason[] = [ // Outside of scope inputs should not be migrated. E.g. references to inputs in `node_modules/`. - InputIncompatibilityReason.OutsideOfMigrationScope, + FieldIncompatibilityReason.OutsideOfMigrationScope, // Explicitly filtered inputs cannot be skipped via best effort mode. - InputIncompatibilityReason.SkippedViaConfigFilter, + FieldIncompatibilityReason.SkippedViaConfigFilter, // There is no good output for accessor inputs. - InputIncompatibilityReason.Accessor, + FieldIncompatibilityReason.Accessor, // There is no good output for such inputs. We can't perform "conversion". - InputIncompatibilityReason.RequiredInputButNoGoodExplicitTypeExtractable, + FieldIncompatibilityReason.SignalInput__RequiredButNoGoodExplicitTypeExtractable, + FieldIncompatibilityReason.SignalInput__QuestionMarkButNoGoodExplicitTypeExtractable, ]; /** Filters ignorable input incompatibilities when best effort mode is enabled. */ diff --git a/packages/core/schematics/migrations/signal-migration/src/convert-input/prepare_and_check.ts b/packages/core/schematics/migrations/signal-migration/src/convert-input/prepare_and_check.ts index e242d7f1abdf8..9097f4aed114d 100644 --- a/packages/core/schematics/migrations/signal-migration/src/convert-input/prepare_and_check.ts +++ b/packages/core/schematics/migrations/signal-migration/src/convert-input/prepare_and_check.ts @@ -9,9 +9,9 @@ import ts from 'typescript'; import {ExtractedInput} from '../input_detection/input_decorator'; import { - InputIncompatibilityReason, - InputMemberIncompatibility, -} from '../input_detection/incompatibility'; + FieldIncompatibility, + FieldIncompatibilityReason, +} from '../passes/problematic_patterns/incompatibility'; import {InputNode} from '../input_detection/input_node'; import {Decorator} from '@angular/compiler-cli/src/ngtsc/reflection'; import assert from 'assert'; @@ -45,12 +45,12 @@ export function prepareAndCheckForConversion( metadata: ExtractedInput, checker: ts.TypeChecker, options: NgCompilerOptions, -): InputMemberIncompatibility | ConvertInputPreparation { +): FieldIncompatibility | ConvertInputPreparation { // Accessor inputs cannot be migrated right now. if (ts.isAccessor(node)) { return { context: node, - reason: InputIncompatibilityReason.Accessor, + reason: FieldIncompatibilityReason.Accessor, }; } @@ -103,7 +103,8 @@ export function prepareAndCheckForConversion( if (typeToAdd === undefined) { return { context: node, - reason: InputIncompatibilityReason.InputWithQuestionMarkButNoGoodExplicitTypeExtractable, + reason: + FieldIncompatibilityReason.SignalInput__QuestionMarkButNoGoodExplicitTypeExtractable, }; } @@ -154,7 +155,7 @@ export function prepareAndCheckForConversion( // the generated type might depend on imports that we cannot add here (nor want). return { context: node, - reason: InputIncompatibilityReason.RequiredInputButNoGoodExplicitTypeExtractable, + reason: FieldIncompatibilityReason.SignalInput__RequiredButNoGoodExplicitTypeExtractable, }; } } diff --git a/packages/core/schematics/migrations/signal-migration/src/index.ts b/packages/core/schematics/migrations/signal-migration/src/index.ts index 5bcf1cc2d8645..3b599f3cf8b6b 100644 --- a/packages/core/schematics/migrations/signal-migration/src/index.ts +++ b/packages/core/schematics/migrations/signal-migration/src/index.ts @@ -6,10 +6,6 @@ * found in the LICENSE file at https://angular.dev/license */ -export { - getMessageForClassIncompatibility, - getMessageForInputIncompatibility, -} from './input_detection/incompatibility_human'; export {type KnownInputInfo, KnownInputs} from './input_detection/known_inputs'; export { type InputNameNode, @@ -21,4 +17,12 @@ export {type InputDescriptor, getInputDescriptor, isInputDescriptor} from './uti export {SignalInputMigration} from './migration'; export {type MigrationConfig} from './migration_config'; export {nonIgnorableInputIncompatibilities} from './best_effort_mode'; -export {InputIncompatibilityReason} from './input_detection/incompatibility'; +export { + type FieldIncompatibility, + FieldIncompatibilityReason, + ClassIncompatibilityReason, +} from './passes/problematic_patterns/incompatibility'; +export { + getMessageForClassIncompatibility, + getMessageForFieldIncompatibility, +} from './passes/problematic_patterns/incompatibility_human'; diff --git a/packages/core/schematics/migrations/signal-migration/src/input_detection/directive_info.ts b/packages/core/schematics/migrations/signal-migration/src/input_detection/directive_info.ts index 1d52380ab1ba5..bc4d6b994fbb8 100644 --- a/packages/core/schematics/migrations/signal-migration/src/input_detection/directive_info.ts +++ b/packages/core/schematics/migrations/signal-migration/src/input_detection/directive_info.ts @@ -9,7 +9,10 @@ import ts from 'typescript'; import {ExtractedInput} from './input_decorator'; import {InputDescriptor} from '../utils/input_id'; -import {ClassIncompatibilityReason, InputMemberIncompatibility} from './incompatibility'; +import { + ClassIncompatibilityReason, + FieldIncompatibility, +} from '../passes/problematic_patterns/incompatibility'; import {ClassFieldUniqueKey} from '../passes/reference_resolution/known_fields'; /** @@ -26,7 +29,7 @@ export class DirectiveInfo { >(); /** Map of input IDs and their incompatibilities. */ - memberIncompatibility = new Map(); + memberIncompatibility = new Map(); /** * Whether the whole class is incompatible. @@ -61,7 +64,7 @@ export class DirectiveInfo { /** Get incompatibility of the given member, if it's incompatible for migration. */ getInputMemberIncompatibility( input: InputDescriptor, - ): ClassIncompatibilityReason | InputMemberIncompatibility | null { + ): ClassIncompatibilityReason | FieldIncompatibility | null { return this.memberIncompatibility.get(input.key) ?? this.incompatible ?? null; } } diff --git a/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility.ts b/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility.ts deleted file mode 100644 index 61764da4a837e..0000000000000 --- a/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility.ts +++ /dev/null @@ -1,67 +0,0 @@ -/** - * @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.dev/license - */ - -import ts from 'typescript'; - -/** - * Reasons why an input cannot be migrated. - * - * Higher values of incompatibility reasons indicate a more significant - * incompatibility reason. Lower ones may be overridden by higher ones. - * */ -export enum InputIncompatibilityReason { - OverriddenByDerivedClass = 1, - RedeclaredViaDerivedClassInputsArray = 2, - TypeConflictWithBaseClass = 3, - ParentIsIncompatible = 4, - SpyOnThatOverwritesField = 5, - PotentiallyNarrowedInTemplateButNoSupportYet = 6, - RequiredInputButNoGoodExplicitTypeExtractable = 7, - InputWithQuestionMarkButNoGoodExplicitTypeExtractable = 8, - WriteAssignment = 9, - Accessor = 10, - OutsideOfMigrationScope = 11, - SkippedViaConfigFilter = 12, -} - -/** Reasons why a whole class and its inputs cannot be migrated. */ -export enum ClassIncompatibilityReason { - ClassManuallyInstantiated, - InputOwningClassReferencedInClassProperty, -} - -/** Description of an input incompatibility and some helpful debug context. */ -export interface InputMemberIncompatibility { - reason: InputIncompatibilityReason; - context: ts.Node | null; -} - -/** Whether the given value refers to an input member incompatibility. */ -export function isInputMemberIncompatibility(value: unknown): value is InputMemberIncompatibility { - return ( - (value as Partial).reason !== undefined && - (value as Partial).context !== undefined && - InputIncompatibilityReason.hasOwnProperty( - (value as Partial).reason!, - ) - ); -} - -/** Picks the more significant input compatibility. */ -export function pickInputIncompatibility( - a: InputMemberIncompatibility, - b: InputMemberIncompatibility | null, -): InputMemberIncompatibility { - if (b === null) { - return a; - } - if (a.reason < b.reason) { - return b; - } - return a; -} diff --git a/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human.ts b/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human.ts deleted file mode 100644 index 794f636b169a8..0000000000000 --- a/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human.ts +++ /dev/null @@ -1,118 +0,0 @@ -/** - * @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.dev/license - */ - -import {ClassIncompatibilityReason, InputIncompatibilityReason} from './incompatibility'; - -/** - * Gets human-readable message information for the given input incompatibility. - * This text will be used by the language service, or CLI-based migration. - */ -export function getMessageForInputIncompatibility(reason: InputIncompatibilityReason): { - short: string; - extra: string; -} { - switch (reason) { - case InputIncompatibilityReason.Accessor: - return { - short: 'Accessor inputs cannot be migrated as they are too complex.', - extra: - 'The migration potentially requires usage of `effect` or `computed`, but ' + - 'the intent is unclear. The migration cannot safely migrate.', - }; - case InputIncompatibilityReason.OverriddenByDerivedClass: - return { - short: 'The input cannot be migrated because the field is overridden by a subclass.', - extra: 'The field in the subclass is not an input, so migrating would break your build.', - }; - case InputIncompatibilityReason.ParentIsIncompatible: - return { - short: 'This input is inherited from a superclass, but the parent cannot be migrated.', - extra: 'Migrating this input would cause your build to fail.', - }; - case InputIncompatibilityReason.PotentiallyNarrowedInTemplateButNoSupportYet: - return { - short: - 'This input is used in a control flow expression (e.g. `@if` or `*ngIf`) and ' + - 'migrating would break narrowing currently.', - extra: `In the future, Angular intends to support narrowing of signals.`, - }; - case InputIncompatibilityReason.RedeclaredViaDerivedClassInputsArray: - return { - short: 'The input is overridden by a subclass that cannot be migrated.', - extra: - 'The subclass re-declares this input via the `inputs` array in @Directive/@Component. ' + - 'Migrating this input would break your build because the subclass input cannot be migrated.', - }; - case InputIncompatibilityReason.RequiredInputButNoGoodExplicitTypeExtractable: - return { - short: `Input is required, but the migration cannot determine a good type for the input.`, - extra: 'Consider adding an explicit type to make the migration possible.', - }; - case InputIncompatibilityReason.InputWithQuestionMarkButNoGoodExplicitTypeExtractable: - return { - short: `Input is marked with a question mark. Migration could not determine a good type for the input.`, - extra: - 'The migration needs to be able to resolve a type, so that it can include `undefined` in your type. ' + - 'Consider adding an explicit type to make the migration possible.', - }; - case InputIncompatibilityReason.SkippedViaConfigFilter: - return { - short: `This input is not part of the current migration scope.`, - extra: 'Skipped via migration config.', - }; - case InputIncompatibilityReason.SpyOnThatOverwritesField: - return { - short: 'A jasmine `spyOn` call spies on this input. This breaks with signal inputs.', - extra: `Migration cannot safely migrate as "spyOn" writes to the input. Signal inputs are readonly.`, - }; - case InputIncompatibilityReason.TypeConflictWithBaseClass: - return { - short: - 'This input overrides a field from a superclass, while the superclass field is not migrated.', - extra: 'Migrating the input would break your build because of a type conflict then.', - }; - case InputIncompatibilityReason.WriteAssignment: - return { - short: 'Your application code writes to the input. This prevents migration.', - extra: 'Signal inputs are readonly, so migrating would break your build.', - }; - case InputIncompatibilityReason.OutsideOfMigrationScope: - return { - short: 'This input is not part of any source files in your project.', - extra: 'The migration excludes inputs if no source file declaring the input was seen.', - }; - } -} - -/** - * Gets human-readable message information for the given input class incompatibility. - * This text will be used by the language service, or CLI-based migration. - */ -export function getMessageForClassIncompatibility(reason: ClassIncompatibilityReason): { - short: string; - extra: string; -} { - switch (reason) { - case ClassIncompatibilityReason.InputOwningClassReferencedInClassProperty: - return { - short: 'Class of this input is referenced in the signature of another class.', - extra: - 'The other class is likely typed to expect a non-migrated field, so ' + - 'migration is skipped to not break your build.', - }; - case ClassIncompatibilityReason.ClassManuallyInstantiated: - return { - short: - 'Class of this input is manually instantiated. ' + - 'This is discouraged and prevents migration', - extra: - 'Signal inputs require a DI injection context. Manually instantiating ' + - 'breaks this requirement in some cases, so the migration is skipped.', - }; - } -} diff --git a/packages/core/schematics/migrations/signal-migration/src/input_detection/known_inputs.ts b/packages/core/schematics/migrations/signal-migration/src/input_detection/known_inputs.ts index 33ec709f4fe75..335d259592215 100644 --- a/packages/core/schematics/migrations/signal-migration/src/input_detection/known_inputs.ts +++ b/packages/core/schematics/migrations/signal-migration/src/input_detection/known_inputs.ts @@ -13,11 +13,11 @@ import {InputNode} from './input_node'; import {DirectiveInfo} from './directive_info'; import { ClassIncompatibilityReason, - InputIncompatibilityReason, - InputMemberIncompatibility, - isInputMemberIncompatibility, - pickInputIncompatibility, -} from './incompatibility'; + FieldIncompatibility, + FieldIncompatibilityReason, + isFieldIncompatibility, + pickFieldIncompatibility, +} from '../passes/problematic_patterns/incompatibility'; import {ClassFieldUniqueKey, KnownFields} from '../passes/reference_resolution/known_fields'; import {attemptRetrieveInputFromSymbol} from './nodes_to_input'; import {ProgramInfo, projectFile, ProjectFile} from '../../../../utils/tsurge'; @@ -122,7 +122,7 @@ export class KnownInputs } /** Marks the given input as incompatible for migration. */ - markFieldIncompatible(input: InputDescriptor, incompatibility: InputMemberIncompatibility) { + markFieldIncompatible(input: InputDescriptor, incompatibility: FieldIncompatibility) { if (!this.knownInputIds.has(input.key)) { throw new Error(`Input cannot be marked as incompatible because it's not registered.`); } @@ -131,8 +131,8 @@ export class KnownInputs const existingIncompatibility = inputInfo.container.getInputMemberIncompatibility(input); // Ensure an existing more significant incompatibility is not overridden. - if (existingIncompatibility !== null && isInputMemberIncompatibility(existingIncompatibility)) { - incompatibility = pickInputIncompatibility(existingIncompatibility, incompatibility); + if (existingIncompatibility !== null && isFieldIncompatibility(existingIncompatibility)) { + incompatibility = pickFieldIncompatibility(existingIncompatibility, incompatibility); } this.knownInputIds @@ -169,14 +169,14 @@ export class KnownInputs captureUnknownDerivedField(field: InputDescriptor): void { this.markFieldIncompatible(field, { context: null, - reason: InputIncompatibilityReason.OverriddenByDerivedClass, + reason: FieldIncompatibilityReason.OverriddenByDerivedClass, }); } captureUnknownParentField(field: InputDescriptor): void { this.markFieldIncompatible(field, { context: null, - reason: InputIncompatibilityReason.TypeConflictWithBaseClass, + reason: FieldIncompatibilityReason.TypeConflictWithBaseClass, }); } } diff --git a/packages/core/schematics/migrations/signal-migration/src/migration.ts b/packages/core/schematics/migrations/signal-migration/src/migration.ts index 5cc7b8a6b72c6..e4fb740ae0b6f 100644 --- a/packages/core/schematics/migrations/signal-migration/src/migration.ts +++ b/packages/core/schematics/migrations/signal-migration/src/migration.ts @@ -26,8 +26,8 @@ import {filterIncompatibilitiesForBestEffortMode} from './best_effort_mode'; import assert from 'assert'; import { ClassIncompatibilityReason, - InputIncompatibilityReason, -} from './input_detection/incompatibility'; + FieldIncompatibilityReason, +} from './passes/problematic_patterns/incompatibility'; import {isInputDescriptor} from './utils/input_id'; import {MigrationConfig} from './migration_config'; import {ClassFieldUniqueKey} from './passes/reference_resolution/known_fields'; @@ -196,8 +196,8 @@ export class SignalInputMigration extends TsurgeComplexMigration< const isConsideredSourceInput = input.seenAsSourceInput && - input.memberIncompatibility !== InputIncompatibilityReason.OutsideOfMigrationScope && - input.memberIncompatibility !== InputIncompatibilityReason.SkippedViaConfigFilter; + input.memberIncompatibility !== FieldIncompatibilityReason.OutsideOfMigrationScope && + input.memberIncompatibility !== FieldIncompatibilityReason.SkippedViaConfigFilter; // We won't track incompatibilities to inputs that aren't considered source inputs. // Tracking their statistics wouldn't provide any value. @@ -212,7 +212,7 @@ export class SignalInputMigration extends TsurgeComplexMigration< } if (input.memberIncompatibility !== null) { - const reasonName = InputIncompatibilityReason[input.memberIncompatibility]; + const reasonName = FieldIncompatibilityReason[input.memberIncompatibility]; const key = `input-field-incompatibility-${reasonName}` as const; fieldIncompatibleCounts[key] ??= 0; fieldIncompatibleCounts[key]++; @@ -258,7 +258,7 @@ function filterInputsViaConfig( skippedInputs.add(input.descriptor.key); knownInputs.markFieldIncompatible(input.descriptor, { context: null, - reason: InputIncompatibilityReason.SkippedViaConfigFilter, + reason: FieldIncompatibilityReason.SkippedViaConfigFilter, }); } } diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/1_identify_inputs.ts b/packages/core/schematics/migrations/signal-migration/src/passes/1_identify_inputs.ts index 64524c0efd9fd..5a3d1fe39d878 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/1_identify_inputs.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/1_identify_inputs.ts @@ -19,7 +19,7 @@ import {MigrationHost} from '../migration_host'; import {MigrationResult} from '../result'; import {getInputDescriptor} from '../utils/input_id'; import {prepareAndCheckForConversion} from '../convert-input/prepare_and_check'; -import {isInputMemberIncompatibility} from '../input_detection/incompatibility'; +import {isFieldIncompatibility} from './problematic_patterns/incompatibility'; /** * Phase where we iterate through all source files of the program (including `.d.ts`) @@ -62,7 +62,7 @@ export function pass1__IdentifySourceFileAndDeclarationInputs( host.compilerOptions, ); - if (isInputMemberIncompatibility(conversionPreparation)) { + if (isFieldIncompatibility(conversionPreparation)) { knownDecoratorInputs.markFieldIncompatible(inputDescr, conversionPreparation); result.sourceInputs.set(inputDescr, null); } else { diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/common_incompatible_patterns.ts b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/common_incompatible_patterns.ts index 4a5cba2337031..706f3043aad00 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/common_incompatible_patterns.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/common_incompatible_patterns.ts @@ -9,7 +9,7 @@ import {unwrapExpression} from '@angular/compiler-cli/src/ngtsc/annotations/common'; import assert from 'assert'; import ts from 'typescript'; -import {ClassIncompatibilityReason} from '../../input_detection/incompatibility'; +import {ClassIncompatibilityReason} from './incompatibility'; import {SpyOnFieldPattern} from '../../pattern_advisors/spy_on_pattern'; import {getMemberName} from '../../utils/class_member_names'; import {GroupedTsAstVisitor} from '../../utils/grouped_ts_ast_visitor'; @@ -112,7 +112,7 @@ export function checkIncompatiblePatterns( fields.markClassIncompatible( inputClassSymbolsToClass.get(newTarget)!, - ClassIncompatibilityReason.InputOwningClassReferencedInClassProperty, + ClassIncompatibilityReason.OwningClassReferencedInClassProperty, ); } } diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility.ts b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility.ts new file mode 100644 index 0000000000000..f9d117a10daf4 --- /dev/null +++ b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility.ts @@ -0,0 +1,65 @@ +/** + * @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.dev/license + */ + +import ts from 'typescript'; + +/** + * Reasons why a field cannot be migrated. + * + * Higher values of incompatibility reasons indicate a more significant + * incompatibility reason. Lower ones may be overridden by higher ones. + * */ +export enum FieldIncompatibilityReason { + OverriddenByDerivedClass = 1, + RedeclaredViaDerivedClassInputsArray = 2, + TypeConflictWithBaseClass = 3, + ParentIsIncompatible = 4, + SpyOnThatOverwritesField = 5, + PotentiallyNarrowedInTemplateButNoSupportYet = 6, + SignalInput__RequiredButNoGoodExplicitTypeExtractable = 7, + SignalInput__QuestionMarkButNoGoodExplicitTypeExtractable = 8, + WriteAssignment = 9, + Accessor = 10, + OutsideOfMigrationScope = 11, + SkippedViaConfigFilter = 12, +} + +/** Reasons why a whole class and its fields cannot be migrated. */ +export enum ClassIncompatibilityReason { + ClassManuallyInstantiated, + OwningClassReferencedInClassProperty, +} + +/** Description of a field incompatibility and some helpful debug context. */ +export interface FieldIncompatibility { + reason: FieldIncompatibilityReason; + context: ts.Node | null; +} + +/** Whether the given value refers to an field incompatibility. */ +export function isFieldIncompatibility(value: unknown): value is FieldIncompatibility { + return ( + (value as Partial).reason !== undefined && + (value as Partial).context !== undefined && + FieldIncompatibilityReason.hasOwnProperty((value as Partial).reason!) + ); +} + +/** Picks the more significant field compatibility. */ +export function pickFieldIncompatibility( + a: FieldIncompatibility, + b: FieldIncompatibility | null, +): FieldIncompatibility { + if (b === null) { + return a; + } + if (a.reason < b.reason) { + return b; + } + return a; +} diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility_human.ts b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility_human.ts new file mode 100644 index 0000000000000..bd1e6e8cfda34 --- /dev/null +++ b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility_human.ts @@ -0,0 +1,127 @@ +/** + * @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.dev/license + */ + +import {ClassIncompatibilityReason, FieldIncompatibilityReason} from './incompatibility'; + +/** + * Gets human-readable message information for the given field incompatibility. + * This text will be used by the language service, or CLI-based migration. + */ +export function getMessageForFieldIncompatibility( + reason: FieldIncompatibilityReason, + fieldName: {single: string; plural: string}, +): { + short: string; + extra: string; +} { + switch (reason) { + case FieldIncompatibilityReason.Accessor: + return { + short: `Accessor ${fieldName.plural} cannot be migrated as they are too complex.`, + extra: + 'The migration potentially requires usage of `effect` or `computed`, but ' + + 'the intent is unclear. The migration cannot safely migrate.', + }; + case FieldIncompatibilityReason.OverriddenByDerivedClass: + return { + short: `The ${fieldName.single} cannot be migrated because the field is overridden by a subclass.`, + extra: 'The field in the subclass is not a signal, so migrating would break your build.', + }; + case FieldIncompatibilityReason.ParentIsIncompatible: + return { + short: `This ${fieldName.single} is inherited from a superclass, but the parent cannot be migrated.`, + extra: 'Migrating this field would cause your build to fail.', + }; + case FieldIncompatibilityReason.PotentiallyNarrowedInTemplateButNoSupportYet: + return { + short: + `This ${fieldName.single} is used in a control flow expression (e.g. \`@if\` or \`*ngIf\`) and ` + + 'migrating would break narrowing currently.', + extra: `In the future, Angular intends to support narrowing of signals.`, + }; + case FieldIncompatibilityReason.RedeclaredViaDerivedClassInputsArray: + return { + short: `The ${fieldName.single} is overridden by a subclass that cannot be migrated.`, + extra: + `The subclass overrides this ${fieldName.single} via the \`inputs\` array in @Directive/@Component. ` + + 'Migrating the field would break your build because the subclass field cannot be a signal.', + }; + case FieldIncompatibilityReason.SignalInput__RequiredButNoGoodExplicitTypeExtractable: + return { + short: `Input is required, but the migration cannot determine a good type for the input.`, + extra: 'Consider adding an explicit type to make the migration possible.', + }; + case FieldIncompatibilityReason.SignalInput__QuestionMarkButNoGoodExplicitTypeExtractable: + return { + short: `Input is marked with a question mark. Migration could not determine a good type for the input.`, + extra: + 'The migration needs to be able to resolve a type, so that it can include `undefined` in your type. ' + + 'Consider adding an explicit type to make the migration possible.', + }; + case FieldIncompatibilityReason.SkippedViaConfigFilter: + return { + short: `This ${fieldName.single} is not part of the current migration scope.`, + extra: 'Skipped via migration config.', + }; + case FieldIncompatibilityReason.SpyOnThatOverwritesField: + return { + short: 'A jasmine `spyOn` call spies on this field. This breaks with signals.', + extra: + `Migration cannot safely migrate as "spyOn" writes to the ${fieldName.single}. ` + + `Signal ${fieldName.plural} are readonly.`, + }; + case FieldIncompatibilityReason.TypeConflictWithBaseClass: + return { + short: + `This ${fieldName.single} overrides a field from a superclass, while the superclass ` + + `field is not migrated.`, + extra: 'Migrating the field would break your build because of a type conflict.', + }; + case FieldIncompatibilityReason.WriteAssignment: + return { + short: `Your application code writes to the ${fieldName.single}. This prevents migration.`, + extra: `Signal ${fieldName.plural} are readonly, so migrating would break your build.`, + }; + case FieldIncompatibilityReason.OutsideOfMigrationScope: + return { + short: `This ${fieldName.single} is not part of any source files in your project.`, + extra: `The migration excludes ${fieldName.plural} if no source file declaring them was seen.`, + }; + } +} + +/** + * Gets human-readable message information for the given class incompatibility. + * This text will be used by the language service, or CLI-based migration. + */ +export function getMessageForClassIncompatibility( + reason: ClassIncompatibilityReason, + fieldName: {single: string; plural: string}, +): { + short: string; + extra: string; +} { + switch (reason) { + case ClassIncompatibilityReason.OwningClassReferencedInClassProperty: + return { + short: `Class of this ${fieldName.single} is referenced in the signature of another class.`, + extra: + 'The other class is likely typed to expect a non-migrated field, so ' + + 'migration is skipped to not break your build.', + }; + case ClassIncompatibilityReason.ClassManuallyInstantiated: + return { + short: + `Class of this ${fieldName.single} is manually instantiated. ` + + 'This is discouraged and prevents migration', + extra: + `Signal ${fieldName.plural} require a DI injection context. Manually instantiating ` + + 'breaks this requirement in some cases, so the migration is skipped.', + }; + } +} diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/problematic_field_registry.ts b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/problematic_field_registry.ts index dfac6777cc9f5..f27f0fd595051 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/problematic_field_registry.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/problematic_field_registry.ts @@ -8,10 +8,7 @@ import ts from 'typescript'; import {ClassFieldDescriptor} from '../reference_resolution/known_fields'; -import { - ClassIncompatibilityReason, - InputIncompatibilityReason, -} from '../../input_detection/incompatibility'; +import {ClassIncompatibilityReason, FieldIncompatibility} from './incompatibility'; /** * Interface describing a registry for identifying and checking @@ -28,9 +25,6 @@ import { */ export interface ProblematicFieldRegistry { isFieldIncompatible(descriptor: D): boolean; - markFieldIncompatible( - field: D, - info: {reason: InputIncompatibilityReason; context: ts.Node | null}, - ): void; + markFieldIncompatible(field: D, info: FieldIncompatibility): void; markClassIncompatible(node: ts.ClassDeclaration, reason: ClassIncompatibilityReason): void; } diff --git a/packages/core/schematics/migrations/signal-migration/src/pattern_advisors/spy_on_pattern.ts b/packages/core/schematics/migrations/signal-migration/src/pattern_advisors/spy_on_pattern.ts index 7bdc898ecfe2b..10e5468fb9caa 100644 --- a/packages/core/schematics/migrations/signal-migration/src/pattern_advisors/spy_on_pattern.ts +++ b/packages/core/schematics/migrations/signal-migration/src/pattern_advisors/spy_on_pattern.ts @@ -9,7 +9,7 @@ import ts from 'typescript'; import {ClassFieldDescriptor, KnownFields} from '../passes/reference_resolution/known_fields'; import {ProblematicFieldRegistry} from '../passes/problematic_patterns/problematic_field_registry'; -import {InputIncompatibilityReason} from '../input_detection/incompatibility'; +import {FieldIncompatibilityReason} from '../passes/problematic_patterns/incompatibility'; /** * Detects `spyOn(dirInstance, 'myInput')` calls that likely modify @@ -43,7 +43,7 @@ export class SpyOnFieldPattern { } this.fields.markFieldIncompatible(fieldTarget, { - reason: InputIncompatibilityReason.SpyOnThatOverwritesField, + reason: FieldIncompatibilityReason.SpyOnThatOverwritesField, context: node, }); } diff --git a/packages/core/schematics/migrations/signal-migration/src/phase_analysis.ts b/packages/core/schematics/migrations/signal-migration/src/phase_analysis.ts index 375978fdb7f78..f77f6bf25717b 100644 --- a/packages/core/schematics/migrations/signal-migration/src/phase_analysis.ts +++ b/packages/core/schematics/migrations/signal-migration/src/phase_analysis.ts @@ -21,7 +21,7 @@ import { isTemplateReference, isTsReference, } from './passes/reference_resolution/reference_kinds'; -import {InputIncompatibilityReason} from './input_detection/incompatibility'; +import {FieldIncompatibilityReason} from './passes/problematic_patterns/incompatibility'; /** * Executes the analysis phase of the migration. @@ -108,14 +108,14 @@ export function executeAnalysisPhase( for (const reference of result.references) { if (isTsReference(reference) && reference.from.isWrite) { knownInputs.markFieldIncompatible(reference.target, { - reason: InputIncompatibilityReason.WriteAssignment, + reason: FieldIncompatibilityReason.WriteAssignment, context: reference.from.node, }); } if (isTemplateReference(reference) || isHostBindingReference(reference)) { if (reference.from.isWrite) { knownInputs.markFieldIncompatible(reference.target, { - reason: InputIncompatibilityReason.WriteAssignment, + reason: FieldIncompatibilityReason.WriteAssignment, // No TS node context available for template or host bindings. context: null, }); @@ -127,7 +127,7 @@ export function executeAnalysisPhase( if (isTemplateReference(reference)) { if (reference.from.isLikelyPartOfNarrowing) { knownInputs.markFieldIncompatible(reference.target, { - reason: InputIncompatibilityReason.PotentiallyNarrowedInTemplateButNoSupportYet, + reason: FieldIncompatibilityReason.PotentiallyNarrowedInTemplateButNoSupportYet, context: null, }); } diff --git a/packages/core/schematics/migrations/signal-migration/src/utils/incompatibility_todos.ts b/packages/core/schematics/migrations/signal-migration/src/utils/incompatibility_todos.ts index 8c710e4686051..b7b1f0ac64893 100644 --- a/packages/core/schematics/migrations/signal-migration/src/utils/incompatibility_todos.ts +++ b/packages/core/schematics/migrations/signal-migration/src/utils/incompatibility_todos.ts @@ -8,17 +8,17 @@ import {KnownInputInfo} from '../input_detection/known_inputs'; import {ProgramInfo, Replacement} from '../../../../utils/tsurge'; -import { - InputIncompatibilityReason, - isInputMemberIncompatibility, -} from '../input_detection/incompatibility'; import { getMessageForClassIncompatibility, - getMessageForInputIncompatibility, -} from '../input_detection/incompatibility_human'; + getMessageForFieldIncompatibility, +} from '../passes/problematic_patterns/incompatibility_human'; import {insertPrecedingLine} from '../../../../utils/tsurge/helpers/ast/insert_preceding_line'; import {InputNode} from '../input_detection/input_node'; import {cutStringToLineLimit} from '../../../../utils/tsurge/helpers/string_manipulation/cut_string_line_length'; +import { + FieldIncompatibilityReason, + isFieldIncompatibility, +} from '../passes/problematic_patterns/incompatibility'; /** * Inserts a TODO for the incompatibility blocking the given node @@ -37,16 +37,17 @@ export function insertTodoForIncompatibility( // If an input is skipped via config filter or outside migration scope, do not // insert TODOs, as this could results in lots of unnecessary comments. if ( - isInputMemberIncompatibility(incompatibility) && - (incompatibility.reason === InputIncompatibilityReason.SkippedViaConfigFilter || - incompatibility.reason === InputIncompatibilityReason.OutsideOfMigrationScope) + isFieldIncompatibility(incompatibility) && + (incompatibility.reason === FieldIncompatibilityReason.SkippedViaConfigFilter || + incompatibility.reason === FieldIncompatibilityReason.OutsideOfMigrationScope) ) { return []; } - const message = isInputMemberIncompatibility(incompatibility) - ? getMessageForInputIncompatibility(incompatibility.reason).short - : getMessageForClassIncompatibility(incompatibility).short; + const message = isFieldIncompatibility(incompatibility) + ? getMessageForFieldIncompatibility(incompatibility.reason, {single: 'input', plural: 'inputs'}) + .short + : getMessageForClassIncompatibility(incompatibility, {single: 'input', plural: 'inputs'}).short; const lines = cutStringToLineLimit(message, 70); return [ diff --git a/packages/core/schematics/migrations/signal-migration/test/golden.txt b/packages/core/schematics/migrations/signal-migration/test/golden.txt index 0afbbfb0a9266..e20cdfffb79db 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden.txt @@ -1134,7 +1134,7 @@ import {Input} from '@angular/core'; class MyComp { // TODO: Skipped for migration because: - // A jasmine `spyOn` call spies on this input. This breaks with signal inputs. + // A jasmine `spyOn` call spies on this field. This breaks with signals. @Input() myInput = () => {}; } diff --git a/packages/language-service/src/refactorings/convert_to_signal_input/apply_input_refactoring.ts b/packages/language-service/src/refactorings/convert_to_signal_input/apply_input_refactoring.ts index 2db86f95c8b0f..e18b466b33970 100644 --- a/packages/language-service/src/refactorings/convert_to_signal_input/apply_input_refactoring.ts +++ b/packages/language-service/src/refactorings/convert_to_signal_input/apply_input_refactoring.ts @@ -12,8 +12,8 @@ import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core'; import ts from 'typescript'; import { getMessageForClassIncompatibility, - getMessageForInputIncompatibility, - InputIncompatibilityReason, + getMessageForFieldIncompatibility, + FieldIncompatibilityReason, KnownInputInfo, MigrationConfig, nonIgnorableInputIncompatibilities, @@ -72,7 +72,7 @@ export async function applySignalInputRefactoring( } const incompatibilityMessages = new Map(); - const incompatibilityReasons = new Set(); + const incompatibilityReasons = new Set(); for (const incompatibleInput of targetInputs.filter((i) => i.isIncompatible())) { const {container, descriptor} = incompatibleInput; @@ -80,14 +80,20 @@ export async function applySignalInputRefactoring( const classIncompatibility = container.incompatible; if (memberIncompatibility !== undefined) { - const {short, extra} = getMessageForInputIncompatibility(memberIncompatibility.reason); + const {short, extra} = getMessageForFieldIncompatibility(memberIncompatibility.reason, { + single: 'input', + plural: 'inputs', + }); incompatibilityMessages.set(descriptor.node.name.text, `${short}\n${extra}`); incompatibilityReasons.add(memberIncompatibility.reason); continue; } if (classIncompatibility !== null) { - const {short, extra} = getMessageForClassIncompatibility(classIncompatibility); + const {short, extra} = getMessageForClassIncompatibility(classIncompatibility, { + single: 'input', + plural: 'inputs', + }); incompatibilityMessages.set(descriptor.node.name.text, `${short}\n${extra}`); continue; } From 95bee1536208b994c7ac4399ac79af1c9ac9e61c Mon Sep 17 00:00:00 2001 From: Julien Saguet Date: Wed, 2 Aug 2023 00:55:18 +0200 Subject: [PATCH 06/15] refactor(service-worker): remove backward compatibility code (#51246) Remove backward compatibility code from service worker package. Fixes #43403 PR Close #51246 --- .../service-worker/worker/src/app-version.ts | 10 -- packages/service-worker/worker/src/driver.ts | 29 ----- .../service-worker/worker/test/happy_spec.ts | 117 +----------------- 3 files changed, 1 insertion(+), 155 deletions(-) diff --git a/packages/service-worker/worker/src/app-version.ts b/packages/service-worker/worker/src/app-version.ts index ef2db4bde8ca2..7f3352470cfc4 100644 --- a/packages/service-worker/worker/src/app-version.ts +++ b/packages/service-worker/worker/src/app-version.ts @@ -15,12 +15,6 @@ import {DebugHandler} from './debug'; import {IdleScheduler} from './idle'; import {Manifest} from './manifest'; -const BACKWARDS_COMPATIBILITY_NAVIGATION_URLS = [ - {positive: true, regex: '^/.*$'}, - {positive: false, regex: '^/.*\\.[^/]*$'}, - {positive: false, regex: '^/.*__'}, -]; - /** * A specific version of the application, identified by a unique manifest * as determined by its hash. @@ -115,10 +109,6 @@ export class AppVersion implements UpdateSource { new DataGroup(scope, adapter, config, database, debugHandler, `${config.version}:data`), ); - // This keeps backwards compatibility with app versions without navigation urls. - // Fix: https://github.com/angular/angular/issues/27209 - manifest.navigationUrls = manifest.navigationUrls || BACKWARDS_COMPATIBILITY_NAVIGATION_URLS; - // Create `include`/`exclude` RegExps for the `navigationUrls` declared in the manifest. const includeUrls = manifest.navigationUrls.filter((spec) => spec.positive); const excludeUrls = manifest.navigationUrls.filter((spec) => !spec.positive); diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts index c463fe9eeebf7..84a1577594350 100644 --- a/packages/service-worker/worker/src/driver.ts +++ b/packages/service-worker/worker/src/driver.ts @@ -163,20 +163,6 @@ export class Driver implements Debuggable, UpdateSource { // As above, it's safe to take over from existing clients immediately, since the new SW // version will continue to serve the old application. await this.scope.clients.claim(); - - // Once all clients have been taken over, we can delete caches used by old versions of - // `@angular/service-worker`, which are no longer needed. This can happen in the background. - this.idle.schedule('activate: cleanup-old-sw-caches', async () => { - try { - await this.cleanupOldSwCaches(); - } catch (err) { - // Nothing to do - cleanup failed. Just log it. - this.debugger.log( - err as Error, - 'cleanupOldSwCaches @ activate: cleanup-old-sw-caches', - ); - } - }); })(), ); @@ -1038,21 +1024,6 @@ export class Driver implements Debuggable, UpdateSource { } } - /** - * Delete caches that were used by older versions of `@angular/service-worker` to avoid running - * into storage quota limitations imposed by browsers. - * (Since at this point the SW has claimed all clients, it is safe to remove those caches.) - */ - async cleanupOldSwCaches(): Promise { - // This is an exceptional case, where we need to interact with caches that would not be - // generated by this ServiceWorker (but by old versions of it). Use the native `CacheStorage` - // directly. - const caches = this.adapter.caches.original; - const cacheNames = await caches.keys(); - const oldSwCacheNames = cacheNames.filter((name) => /^ngsw:(?!\/)/.test(name)); - await Promise.all(oldSwCacheNames.map((name) => caches.delete(name))); - } - /** * Determine if a specific version of the given resource is cached anywhere within the SW, * and fetch it if so. diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index 269b9681fd659..5fc6f2d05b57b 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -9,7 +9,7 @@ import {processNavigationUrls} from '../../config/src/generator'; import {CacheDatabase} from '../src/db-cache'; import {Driver, DriverReadyState} from '../src/driver'; -import {AssetGroupConfig, DataGroupConfig, Manifest} from '../src/manifest'; +import {Manifest} from '../src/manifest'; import {sha1} from '../src/sha1'; import {clearAllCaches, MockCache} from '../testing/cache'; import {MockWindowClient} from '../testing/clients'; @@ -115,24 +115,6 @@ import {envIsSupported} from '../testing/utils'; hashTable: tmpHashTableForFs(brokenFs, {'/bar.txt': true}), }; - // Manifest without navigation urls to test backward compatibility with - // versions < 6.0.0. - interface ManifestV5 { - configVersion: number; - appData?: {[key: string]: string}; - index: string; - assetGroups?: AssetGroupConfig[]; - dataGroups?: DataGroupConfig[]; - hashTable: {[url: string]: string}; - } - - // To simulate versions < 6.0.0 - const manifestOld: ManifestV5 = { - configVersion: 1, - index: '/foo.txt', - hashTable: tmpHashTableForFs(dist), - }; - const manifest: Manifest = { configVersion: 1, timestamp: 1234567890123, @@ -293,36 +275,6 @@ import {envIsSupported} from '../testing/utils'; expect(claimSpy).toHaveBeenCalledTimes(1); }); - it('cleans up old `@angular/service-worker` caches, after activation', async () => { - const claimSpy = spyOn(scope.clients, 'claim'); - const cleanupOldSwCachesSpy = spyOn(driver, 'cleanupOldSwCaches'); - - // Automatically advance time to trigger idle tasks as they are added. - scope.autoAdvanceTime = true; - await scope.startup(true); - await scope.resolveSelfMessages(); - scope.autoAdvanceTime = false; - - expect(cleanupOldSwCachesSpy).toHaveBeenCalledTimes(1); - expect(claimSpy).toHaveBeenCalledBefore(cleanupOldSwCachesSpy); - }); - - it('does not blow up if cleaning up old `@angular/service-worker` caches fails', async () => { - spyOn(driver, 'cleanupOldSwCaches').and.callFake(() => Promise.reject('Ooops')); - - // Automatically advance time to trigger idle tasks as they are added. - scope.autoAdvanceTime = true; - await scope.startup(true); - await scope.resolveSelfMessages(); - scope.autoAdvanceTime = false; - - server.clearRequests(); - - expect(driver.state).toBe(DriverReadyState.NORMAL); - expect(await makeRequest(scope, '/foo.txt')).toBe('this is foo'); - server.assertNoOtherRequests(); - }); - it('initializes prefetched content correctly, after activation', async () => { // Automatically advance time to trigger idle tasks as they are added. scope.autoAdvanceTime = true; @@ -2029,53 +1981,6 @@ import {envIsSupported} from '../testing/utils'; }); }); - describe('cleanupOldSwCaches()', () => { - it('should delete the correct caches', async () => { - const oldSwCacheNames = [ - // Example cache names from the beta versions of `@angular/service-worker`. - 'ngsw:active', - 'ngsw:staged', - 'ngsw:manifest:a1b2c3:super:duper', - // Example cache names from the beta versions of `@angular/service-worker`. - 'ngsw:a1b2c3:assets:foo', - 'ngsw:db:a1b2c3:assets:bar', - ]; - const otherCacheNames = [ - 'ngsuu:active', - 'not:ngsw:active', - 'NgSw:StAgEd', - 'ngsw:/:db:control', - 'ngsw:/foo/:active', - 'ngsw:/bar/:staged', - ]; - const allCacheNames = oldSwCacheNames.concat(otherCacheNames); - - await Promise.all(allCacheNames.map((name) => scope.caches.original.open(name))); - expect(await scope.caches.original.keys()).toEqual( - jasmine.arrayWithExactContents(allCacheNames), - ); - - await driver.cleanupOldSwCaches(); - expect(await scope.caches.original.keys()).toEqual( - jasmine.arrayWithExactContents(otherCacheNames), - ); - }); - - it('should delete other caches even if deleting one of them fails', async () => { - const oldSwCacheNames = ['ngsw:active', 'ngsw:staged', 'ngsw:manifest:a1b2c3:super:duper']; - const deleteSpy = spyOn(scope.caches.original, 'delete').and.callFake((cacheName: string) => - Promise.reject(`Failed to delete cache '${cacheName}'.`), - ); - - await Promise.all(oldSwCacheNames.map((name) => scope.caches.original.open(name))); - const error = await driver.cleanupOldSwCaches().catch((err) => err); - - expect(error).toBe("Failed to delete cache 'ngsw:active'."); - expect(deleteSpy).toHaveBeenCalledTimes(3); - oldSwCacheNames.forEach((name) => expect(deleteSpy).toHaveBeenCalledWith(name)); - }); - }); - describe('bugs', () => { it('does not crash with bad index hash', async () => { scope = new SwTestHarnessBuilder().withServerState(brokenServer).build(); @@ -2610,26 +2515,6 @@ import {envIsSupported} from '../testing/utils'; expect(driver.state).toEqual(DriverReadyState.EXISTING_CLIENTS_ONLY); }); }); - - describe('backwards compatibility with v5', () => { - beforeEach(() => { - const serverV5 = new MockServerStateBuilder() - .withStaticFiles(dist) - .withManifest(manifestOld) - .build(); - - scope = new SwTestHarnessBuilder().withServerState(serverV5).build(); - driver = new Driver(scope, scope, new CacheDatabase(scope)); - }); - - // Test this bug: https://github.com/angular/angular/issues/27209 - it('fills previous versions of manifests with default navigation urls for backwards compatibility', async () => { - expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo'); - await driver.initialized; - scope.updateServerState(serverUpdate); - expect(await driver.checkForUpdate()).toEqual(true); - }); - }); }); describe('navigationRequestStrategy', () => { From 1479af978cd2bbe4ee9f1ca9682684b8e5135fa7 Mon Sep 17 00:00:00 2001 From: Youssef El Houti Date: Tue, 5 Dec 2023 11:24:31 +0100 Subject: [PATCH 07/15] feat(service-worker): finish implementation of refreshAhead feature (#53356) Copy and document the refreshAhead option that allows to refresh cache entries before they expire. This allows to mark cached entries as stale while still retruning them until maxAge in case of service outage. Closes #46729 PR Close #53356 --- .../content/ecosystem/service-workers/config.md | 17 +++++++++++++++++ .../service-worker/config/index.api.md | 1 + packages/service-worker/config/schema.json | 4 ++++ packages/service-worker/config/src/generator.ts | 2 ++ packages/service-worker/config/src/in.ts | 1 + .../config/test/generator_spec.ts | 9 +++++++++ 6 files changed, 34 insertions(+) diff --git a/adev/src/content/ecosystem/service-workers/config.md b/adev/src/content/ecosystem/service-workers/config.md index da5239f6ad7dd..ea752174c25ef 100644 --- a/adev/src/content/ecosystem/service-workers/config.md +++ b/adev/src/content/ecosystem/service-workers/config.md @@ -197,6 +197,7 @@ export interface DataGroup { maxSize: number; maxAge: string; timeout?: string; + refreshAhead?: string; strategy?: 'freshness' | 'performance'; }; cacheQueryOptions?: { @@ -270,6 +271,22 @@ The network timeout is how long the Angular service worker waits for the network For example, the string `5s30u` translates to five seconds and 30 milliseconds of network timeout. + +##### `refreshAhead` + +This duration string specifies the time ahead of the expiration of a cached resource when the Angular service worker should proactively attempt to refresh the resource from the network. +The `refreshAhead` duration is an optional configuration that determines how much time before the expiration of a cached response the service worker should initiate a request to refresh the resource from the network. + +| Suffixes | Details | +|:--- |:--- | +| `d` | Days | +| `h` | Hours | +| `m` | Minutes | +| `s` | Seconds | +| `u` | Milliseconds | + +For example, the string `1h30m` translates to one hour and 30 minutes ahead of the expiration time. + ##### `strategy` The Angular service worker can use either of two caching strategies for data resources. diff --git a/goldens/public-api/service-worker/config/index.api.md b/goldens/public-api/service-worker/config/index.api.md index bd5a35187d1d2..79429bdfbd907 100644 --- a/goldens/public-api/service-worker/config/index.api.md +++ b/goldens/public-api/service-worker/config/index.api.md @@ -44,6 +44,7 @@ export interface DataGroup { maxSize: number; maxAge: Duration; timeout?: Duration; + refreshAhead?: Duration; strategy?: 'freshness' | 'performance'; cacheOpaqueResponses?: boolean; }; diff --git a/packages/service-worker/config/schema.json b/packages/service-worker/config/schema.json index 4b02f9772baf3..fb1ab0cf368af 100644 --- a/packages/service-worker/config/schema.json +++ b/packages/service-worker/config/schema.json @@ -119,6 +119,10 @@ "type": "string", "description": "This duration string specifies the network timeout. The network timeout is how long the Angular service worker will wait for the network to respond before using a cached response, if configured to do so. 'timeout' is a duration string, using the following unit suffixes: d= days, h= hours, m= minutes, s= seconds, u= milliseconds. For example, the string '5s30u' will translate to five seconds and 30 milliseconds of network timeout." }, + "refreshAhead": { + "type": "string", + "description": "This duration string specifies the time ahead of the expiration of a cached resource when the Angular service worker should proactively attempt to refresh the resource from the network. The `refreshAhead` duration is an optional configuration that determines how much time before the expiration of a cached response the service worker should initiate a request to refresh the resource from the network." + }, "strategy": { "enum": [ "freshness", diff --git a/packages/service-worker/config/src/generator.ts b/packages/service-worker/config/src/generator.ts index 14acf2f682f4a..8382eaa963210 100644 --- a/packages/service-worker/config/src/generator.ts +++ b/packages/service-worker/config/src/generator.ts @@ -103,6 +103,8 @@ export class Generator { maxSize: group.cacheConfig.maxSize, maxAge: parseDurationToMs(group.cacheConfig.maxAge), timeoutMs: group.cacheConfig.timeout && parseDurationToMs(group.cacheConfig.timeout), + refreshAheadMs: + group.cacheConfig.refreshAhead && parseDurationToMs(group.cacheConfig.refreshAhead), cacheOpaqueResponses: group.cacheConfig.cacheOpaqueResponses, cacheQueryOptions: buildCacheQueryOptions(group.cacheQueryOptions), version: group.version !== undefined ? group.version : 1, diff --git a/packages/service-worker/config/src/in.ts b/packages/service-worker/config/src/in.ts index f48c37e6cda7e..76d877e2912ea 100644 --- a/packages/service-worker/config/src/in.ts +++ b/packages/service-worker/config/src/in.ts @@ -56,6 +56,7 @@ export interface DataGroup { maxSize: number; maxAge: Duration; timeout?: Duration; + refreshAhead?: Duration; strategy?: 'freshness' | 'performance'; cacheOpaqueResponses?: boolean; }; diff --git a/packages/service-worker/config/test/generator_spec.ts b/packages/service-worker/config/test/generator_spec.ts index 48536899175cd..ef1464715503b 100644 --- a/packages/service-worker/config/test/generator_spec.ts +++ b/packages/service-worker/config/test/generator_spec.ts @@ -100,6 +100,7 @@ describe('Generator', () => { maxAge: 259200000, timeoutMs: 60000, version: 1, + refreshAheadMs: undefined, cacheOpaqueResponses: undefined, cacheQueryOptions: {ignoreVary: true}, }, @@ -351,6 +352,7 @@ describe('Generator', () => { maxSize: 100, maxAge: 259200000, timeoutMs: undefined, + refreshAheadMs: undefined, version: 1, cacheOpaqueResponses: undefined, cacheQueryOptions: {ignoreVary: true}, @@ -362,6 +364,7 @@ describe('Generator', () => { maxSize: 100, maxAge: 259200000, timeoutMs: undefined, + refreshAheadMs: undefined, version: 1, cacheOpaqueResponses: false, cacheQueryOptions: {ignoreVary: true}, @@ -373,6 +376,7 @@ describe('Generator', () => { maxSize: 100, maxAge: 259200000, timeoutMs: undefined, + refreshAheadMs: undefined, version: 1, cacheOpaqueResponses: true, cacheQueryOptions: {ignoreVary: true}, @@ -384,6 +388,7 @@ describe('Generator', () => { maxSize: 100, maxAge: 259200000, timeoutMs: undefined, + refreshAheadMs: undefined, version: 1, cacheOpaqueResponses: undefined, cacheQueryOptions: {ignoreVary: true}, @@ -395,6 +400,7 @@ describe('Generator', () => { maxSize: 100, maxAge: 259200000, timeoutMs: undefined, + refreshAheadMs: undefined, version: 1, cacheOpaqueResponses: false, cacheQueryOptions: {ignoreVary: true}, @@ -406,6 +412,7 @@ describe('Generator', () => { maxSize: 100, maxAge: 259200000, timeoutMs: undefined, + refreshAheadMs: undefined, version: 1, cacheOpaqueResponses: true, cacheQueryOptions: {ignoreVary: true}, @@ -448,6 +455,7 @@ describe('Generator', () => { maxSize: 100, strategy: 'performance', timeout: '1m', + refreshAhead: '1h', }, cacheQueryOptions: {ignoreSearch: false}, }, @@ -477,6 +485,7 @@ describe('Generator', () => { maxSize: 100, maxAge: 259200000, timeoutMs: 60000, + refreshAheadMs: 3600000, version: 1, cacheOpaqueResponses: undefined, cacheQueryOptions: {ignoreSearch: false, ignoreVary: true}, From 471afd20e8f1c76f791b78d70824c9ff5d827ece Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 9 Oct 2024 13:44:44 +0000 Subject: [PATCH 08/15] refactor(migrations): detect ternary narrowing in input and query migrations (#58136) We should skip inputs/queries that are part of ternary narrowing expressions. Those would break builds and we can quickly avoid this in the safe mode as detection is rather easy with the existing analysis data we have. PR Close #58136 --- .../template_reference_visitor.ts | 26 ++++++++++++-- .../test/golden-test/ternary_narrowing.ts | 25 +++++++++++++ .../signal-migration/test/golden.txt | 36 +++++++++++++++++++ .../test/golden_best_effort.txt | 27 ++++++++++++++ 4 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 packages/core/schematics/migrations/signal-migration/test/golden-test/ternary_narrowing.ts diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts b/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts index fae6a2cd89bc6..f14c57f85ccfb 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts @@ -12,6 +12,7 @@ import {SymbolKind, TemplateTypeChecker} from '@angular/compiler-cli/src/ngtsc/t import { AST, BindingType, + Conditional, ImplicitReceiver, LiteralMap, ParsedEventType, @@ -240,6 +241,7 @@ export class TemplateExpressionReferenceVisitor< private activeTmplAstNode: ExprContext | null = null; private detectedInputReferences: TmplInputExpressionReference[] = []; private isInsideObjectShorthandExpression = false; + private insideConditionalExpressionsWithReads: AST[] = []; constructor( private typeChecker: ts.TypeChecker, @@ -292,6 +294,14 @@ export class TemplateExpressionReferenceVisitor< super.visitPropertyWrite(ast, context); } + override visitConditional(ast: Conditional, context: AST[]) { + this.visit(ast.condition, context); + this.insideConditionalExpressionsWithReads.push(ast.condition); + this.visit(ast.trueExp, context); + this.visit(ast.falseExp, context); + this.insideConditionalExpressionsWithReads.pop(); + } + /** * Inspects the property access and attempts to resolve whether they access * a known field. If so, the result is captured. @@ -347,7 +357,7 @@ export class TemplateExpressionReferenceVisitor< read: ast, readAstPath: astPath, context: this.activeTmplAstNode!, - isLikelyNarrowed: false, + isLikelyNarrowed: this._isPartOfNarrowingTernary(ast), isObjectShorthandExpression: this.isInsideObjectShorthandExpression, isWrite, }); @@ -400,11 +410,23 @@ export class TemplateExpressionReferenceVisitor< read: ast, readAstPath: astPath, context: this.activeTmplAstNode!, - isLikelyNarrowed: false, + isLikelyNarrowed: this._isPartOfNarrowingTernary(ast), isObjectShorthandExpression: this.isInsideObjectShorthandExpression, isWrite, }); } + + private _isPartOfNarrowingTernary(read: PropertyRead | PropertyWrite) { + // Note: We do not safe check that the reads are fully matching 1:1. This is acceptable + // as worst case we just skip an input from being migrated. This is very unlikely too. + return this.insideConditionalExpressionsWithReads.some( + (r): r is PropertyRead | PropertyWrite | SafePropertyRead => + (r instanceof PropertyRead || + r instanceof PropertyWrite || + r instanceof SafePropertyRead) && + r.name === read.name, + ); + } } /** diff --git a/packages/core/schematics/migrations/signal-migration/test/golden-test/ternary_narrowing.ts b/packages/core/schematics/migrations/signal-migration/test/golden-test/ternary_narrowing.ts new file mode 100644 index 0000000000000..add7fba78598b --- /dev/null +++ b/packages/core/schematics/migrations/signal-migration/test/golden-test/ternary_narrowing.ts @@ -0,0 +1,25 @@ +// tslint:disable + +import {Component, Input} from '@angular/core'; + +@Component({ + template: ` + {{ narrowed ? narrowed.substring(0, 1) : 'Empty' }} + {{ justChecked ? 'true' : 'false' }} + + {{ other?.safeRead ? other.safeRead : 'Empty' }} + {{ other?.safeRead2 ? other?.safeRead2 : 'Empty' }} + `, +}) +export class TernaryNarrowing { + @Input() narrowed: string | undefined = undefined; + @Input() justChecked = true; + + other?: OtherComponent; +} + +@Component({template: ''}) +export class OtherComponent { + @Input() safeRead: string = ''; + @Input() safeRead2: string = ''; +} diff --git a/packages/core/schematics/migrations/signal-migration/test/golden.txt b/packages/core/schematics/migrations/signal-migration/test/golden.txt index e20cdfffb79db..8cd32a4296b1e 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden.txt @@ -1276,6 +1276,42 @@ export class MyComp { } } } +@@@@@@ ternary_narrowing.ts @@@@@@ + +// tslint:disable + +import {Component, Input, input} from '@angular/core'; + +@Component({ + template: ` + {{ narrowed ? narrowed.substring(0, 1) : 'Empty' }} + {{ justChecked() ? 'true' : 'false' }} + + {{ other?.safeRead ? other.safeRead : 'Empty' }} + {{ other?.safeRead2 ? other?.safeRead2 : 'Empty' }} + `, +}) +export class TernaryNarrowing { + // TODO: Skipped for migration because: + // This input is used in a control flow expression (e.g. `@if` or `*ngIf`) + // and migrating would break narrowing currently. + @Input() narrowed: string | undefined = undefined; + readonly justChecked = input(true); + + other?: OtherComponent; +} + +@Component({template: ''}) +export class OtherComponent { + // TODO: Skipped for migration because: + // This input is used in a control flow expression (e.g. `@if` or `*ngIf`) + // and migrating would break narrowing currently. + @Input() safeRead: string = ''; + // TODO: Skipped for migration because: + // This input is used in a control flow expression (e.g. `@if` or `*ngIf`) + // and migrating would break narrowing currently. + @Input() safeRead2: string = ''; +} @@@@@@ transform_functions.ts @@@@@@ // tslint:disable diff --git a/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt b/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt index 61d8945a34dae..ea588bb728df5 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt @@ -1228,6 +1228,33 @@ export class MyComp { } } } +@@@@@@ ternary_narrowing.ts @@@@@@ + +// tslint:disable + +import {Component, input} from '@angular/core'; + +@Component({ + template: ` + {{ narrowed() ? narrowed().substring(0, 1) : 'Empty' }} + {{ justChecked() ? 'true' : 'false' }} + + {{ other?.safeRead() ? other.safeRead() : 'Empty' }} + {{ other?.safeRead2() ? other?.safeRead2() : 'Empty' }} + `, +}) +export class TernaryNarrowing { + readonly narrowed = input(); + readonly justChecked = input(true); + + other?: OtherComponent; +} + +@Component({template: ''}) +export class OtherComponent { + readonly safeRead = input(''); + readonly safeRead2 = input(''); +} @@@@@@ transform_functions.ts @@@@@@ // tslint:disable From bc5f1175e9f39dfa2699c4de19ee9af4ce4b50d1 Mon Sep 17 00:00:00 2001 From: Georgy Serga Date: Thu, 12 Sep 2024 20:05:12 +0000 Subject: [PATCH 09/15] fix(compiler): transform pseudo selectors correctly for the encapsulated view (#57796) fix scoping and transforming logic of the `shimCssText` for the components with encapsulated view: - add support for pseudo selector functions - apply content scoping for inner selectors of `:is()` and `:where()` - allow multiple comma separated selectors inside pseudo selectors Fixes #45686 PR Close #57796 --- packages/compiler/src/shadow_css.ts | 87 +++++++++++++++---- .../test/shadow_css/shadow_css_spec.ts | 79 +++++++++++++++++ 2 files changed, 151 insertions(+), 15 deletions(-) diff --git a/packages/compiler/src/shadow_css.ts b/packages/compiler/src/shadow_css.ts index d565e97f1c3ca..d34eda4a22854 100644 --- a/packages/compiler/src/shadow_css.ts +++ b/packages/compiler/src/shadow_css.ts @@ -618,7 +618,7 @@ export class ShadowCss { let selector = rule.selector; let content = rule.content; if (rule.selector[0] !== '@') { - selector = this._scopeSelector(rule.selector, scopeSelector, hostSelector); + selector = this._scopeSelector({selector, scopeSelector, hostSelector}); } else if (scopedAtRuleIdentifiers.some((atRule) => rule.selector.startsWith(atRule))) { content = this._scopeSelectors(rule.content, scopeSelector, hostSelector); } else if (rule.selector.startsWith('@font-face') || rule.selector.startsWith('@page')) { @@ -658,15 +658,34 @@ export class ShadowCss { }); } - private _scopeSelector(selector: string, scopeSelector: string, hostSelector: string): string { + private _scopeSelector({ + selector, + scopeSelector, + hostSelector, + shouldScope, + }: { + selector: string; + scopeSelector: string; + hostSelector: string; + shouldScope?: boolean; + }): string { + // Split the selector into independent parts by `,` (comma) unless + // comma is within parenthesis, for example `:is(.one, two)`. + const selectorSplitRe = / ?,(?![^\(]*\)) ?/; + return selector - .split(/ ?, ?/) + .split(selectorSplitRe) .map((part) => part.split(_shadowDeepSelectors)) .map((deepParts) => { const [shallowPart, ...otherParts] = deepParts; const applyScope = (shallowPart: string) => { if (this._selectorNeedsScoping(shallowPart, scopeSelector)) { - return this._applySelectorScope(shallowPart, scopeSelector, hostSelector); + return this._applySelectorScope({ + selector: shallowPart, + scopeSelector, + hostSelector, + shouldScope, + }); } else { return shallowPart; } @@ -715,11 +734,17 @@ export class ShadowCss { // return a selector with [name] suffix on each simple selector // e.g. .foo.bar > .zot becomes .foo[name].bar[name] > .zot[name] /** @internal */ - private _applySelectorScope( - selector: string, - scopeSelector: string, - hostSelector: string, - ): string { + private _applySelectorScope({ + selector, + scopeSelector, + hostSelector, + shouldScope, + }: { + selector: string; + scopeSelector: string; + hostSelector: string; + shouldScope?: boolean; + }): string { const isRe = /\[is=([^\]]*)\]/g; scopeSelector = scopeSelector.replace(isRe, (_: string, ...parts: string[]) => parts[0]); @@ -748,13 +773,46 @@ export class ShadowCss { return scopedP; }; + // Wraps `_scopeSelectorPart()` to not use it directly on selectors with + // pseudo selector functions like `:where()`. Selectors within pseudo selector + // functions are recursively sent to `_scopeSelector()` with the `shouldScope` + // argument, so the selectors get scoped correctly. + const _pseudoFunctionAwareScopeSelectorPart = (selectorPart: string) => { + let scopedPart = ''; + + const cssPseudoSelectorFunctionMatch = selectorPart.match(_cssPseudoSelectorFunctionPrefix); + if (cssPseudoSelectorFunctionMatch) { + const [cssPseudoSelectorFunction] = cssPseudoSelectorFunctionMatch; + // Unwrap the pseudo selector, to scope its contents. + // For example, `:where(selectorToScope)` -> `selectorToScope`. + const selectorToScope = selectorPart.slice(cssPseudoSelectorFunction.length, -1); + + const scopedInnerPart = this._scopeSelector({ + selector: selectorToScope, + scopeSelector, + hostSelector, + shouldScope: shouldScopeIndicator, + }); + // Put the result back into the pseudo selector function. + scopedPart = `${cssPseudoSelectorFunction}${scopedInnerPart})`; + } else { + shouldScopeIndicator = + shouldScopeIndicator || selectorPart.includes(_polyfillHostNoCombinator); + scopedPart = shouldScopeIndicator ? _scopeSelectorPart(selectorPart) : selectorPart; + } + + return scopedPart; + }; + const safeContent = new SafeSelector(selector); selector = safeContent.content(); let scopedSelector = ''; let startIndex = 0; let res: RegExpExecArray | null; - const sep = /( |>|\+|~(?!=))\s*/g; + // Spaces aren't used as a delimeter if they are within parenthesis, for example + // `:where(.one .two)` stays intact. + const sep = /( (?![^\(]*\))|>|\+|~(?!=))\s*/g; // If a selector appears before :host it should not be shimmed as it // matches on ancestor elements and not on elements in the host's shadow @@ -769,7 +827,7 @@ export class ShadowCss { // `:host-context(tag)`) const hasHost = selector.includes(_polyfillHostNoCombinator); // Only scope parts after the first `-shadowcsshost-no-combinator` when it is present - let shouldScope = !hasHost; + let shouldScopeIndicator = shouldScope ?? !hasHost; while ((res = sep.exec(selector)) !== null) { const separator = res[1]; @@ -788,15 +846,13 @@ export class ShadowCss { continue; } - shouldScope = shouldScope || part.includes(_polyfillHostNoCombinator); - const scopedPart = shouldScope ? _scopeSelectorPart(part) : part; + const scopedPart = _pseudoFunctionAwareScopeSelectorPart(part); scopedSelector += `${scopedPart} ${separator} `; startIndex = sep.lastIndex; } const part = selector.substring(startIndex); - shouldScope = shouldScope || part.includes(_polyfillHostNoCombinator); - scopedSelector += shouldScope ? _scopeSelectorPart(part) : part; + scopedSelector += _pseudoFunctionAwareScopeSelectorPart(part); // replace the placeholders with their original values return safeContent.restore(scopedSelector); @@ -864,6 +920,7 @@ class SafeSelector { } } +const _cssPseudoSelectorFunctionPrefix = /^:(where|is)\(/gi; const _cssContentNextSelectorRe = /polyfill-next-selector[^}]*content:[\s]*?(['"])(.*?)\1[;\s]*}([^{]*?){/gim; const _cssContentRuleRe = /(polyfill-rule)[^}]*(content:[\s]*(['"])(.*?)\3)[;\s]*[^}]*}/gim; diff --git a/packages/compiler/test/shadow_css/shadow_css_spec.ts b/packages/compiler/test/shadow_css/shadow_css_spec.ts index e7e038d1b52e8..49ebf51cf938e 100644 --- a/packages/compiler/test/shadow_css/shadow_css_spec.ts +++ b/packages/compiler/test/shadow_css/shadow_css_spec.ts @@ -79,6 +79,85 @@ describe('ShadowCss', () => { ); }); + it('should handle pseudo functions correctly', () => { + // :where() + expect(shim(':where(.one) {}', 'contenta', 'hosta')).toEqualCss(':where(.one[contenta]) {}'); + expect(shim(':where(div.one span.two) {}', 'contenta', 'hosta')).toEqualCss( + ':where(div.one[contenta] span.two[contenta]) {}', + ); + expect(shim(':where(.one) .two {}', 'contenta', 'hosta')).toEqualCss( + ':where(.one[contenta]) .two[contenta] {}', + ); + expect(shim(':where(:host) {}', 'contenta', 'hosta')).toEqualCss(':where([hosta]) {}'); + expect(shim(':where(.one) :where(:host) {}', 'contenta', 'hosta')).toEqualCss( + ':where(.one) :where([hosta]) {}', + ); + expect(shim(':where(.one :host) {}', 'contenta', 'hosta')).toEqualCss( + ':where(.one [hosta]) {}', + ); + expect(shim('div :where(.one) {}', 'contenta', 'hosta')).toEqualCss( + 'div[contenta] :where(.one[contenta]) {}', + ); + expect(shim(':host :where(.one .two) {}', 'contenta', 'hosta')).toEqualCss( + '[hosta] :where(.one[contenta] .two[contenta]) {}', + ); + expect(shim(':where(.one, .two) {}', 'contenta', 'hosta')).toEqualCss( + ':where(.one[contenta], .two[contenta]) {}', + ); + + // :is() + expect(shim('div:is(.foo) {}', 'contenta', 'a-host')).toEqualCss('div[contenta]:is(.foo) {}'); + expect(shim(':is(.dark :host) {}', 'contenta', 'a-host')).toEqualCss(':is(.dark [a-host]) {}'); + expect(shim(':host:is(.foo) {}', 'contenta', 'a-host')).toEqualCss('[a-host]:is(.foo) {}'); + expect(shim(':is(.foo) {}', 'contenta', 'a-host')).toEqualCss(':is(.foo[contenta]) {}'); + expect(shim(':is(.foo, .bar, .baz) {}', 'contenta', 'a-host')).toEqualCss( + ':is(.foo[contenta], .bar[contenta], .baz[contenta]) {}', + ); + expect(shim(':is(.foo, .bar) :host {}', 'contenta', 'a-host')).toEqualCss( + ':is(.foo, .bar) [a-host] {}', + ); + + // :is() and :where() + expect( + shim( + ':is(.foo, .bar) :is(.baz) :where(.one, .two) :host :where(.three:first-child) {}', + 'contenta', + 'a-host', + ), + ).toEqualCss( + ':is(.foo, .bar) :is(.baz) :where(.one, .two) [a-host] :where(.three[contenta]:first-child) {}', + ); + + // complex selectors + expect(shim(':host:is([foo],[foo-2])>div.example-2 {}', 'contenta', 'a-host')).toEqualCss( + '[a-host]:is([foo],[foo-2]) > div.example-2[contenta] {}', + ); + expect(shim(':host:is([foo], [foo-2]) > div.example-2 {}', 'contenta', 'a-host')).toEqualCss( + '[a-host]:is([foo], [foo-2]) > div.example-2[contenta] {}', + ); + expect(shim(':host:has([foo],[foo-2])>div.example-2 {}', 'contenta', 'a-host')).toEqualCss( + '[a-host]:has([foo],[foo-2]) > div.example-2[contenta] {}', + ); + expect(shim(':host:is([foo], [foo-2]) > div.example-2 {}', 'contenta', 'a-host')).toEqualCss( + '[a-host]:is([foo], [foo-2]) > div.example-2[contenta] {}', + ); + + // :has() + expect(shim('div:has(a) {}', 'contenta', 'hosta')).toEqualCss('div[contenta]:has(a) {}'); + expect(shim('div:has(a) :host {}', 'contenta', 'hosta')).toEqualCss('div:has(a) [hosta] {}'); + expect(shim(':has(a) :host :has(b) {}', 'contenta', 'hosta')).toEqualCss( + ':has(a) [hosta] [contenta]:has(b) {}', + ); + // Unlike `:is()` or `:where()` the attribute selector isn't placed inside + // of `:has()`. That is deliberate, `[contenta]:has(a)` would select all + // `[contenta]` with `a` inside, while `:has(a[contenta])` would select + // everything that contains `a[contenta]`, targeting elements outside of + // encapsulated scope. + expect(shim(':has(a) :has(b) {}', 'contenta', 'hosta')).toEqualCss( + '[contenta]:has(a) [contenta]:has(b) {}', + ); + }); + it('should handle escaped selector with space (if followed by a hex char)', () => { // When esbuild runs with optimization.minify // selectors are escaped: .über becomes .\fc ber. From 82144b6d63d072d112d1a7f4dcc018a1d64bb994 Mon Sep 17 00:00:00 2001 From: Georgy Serga Date: Thu, 12 Sep 2024 20:41:45 +0000 Subject: [PATCH 10/15] fix(compiler): allow combinators inside pseudo selectors (#57796) allow css combinators within pseudo selector functions, parsing those correctly. Similarly to previous version, don't break selectors into part if combinators are within parenthesis, for example `:where(.one > .two)` PR Close #57796 --- packages/compiler/src/shadow_css.ts | 4 ++-- packages/compiler/test/shadow_css/shadow_css_spec.ts | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/compiler/src/shadow_css.ts b/packages/compiler/src/shadow_css.ts index d34eda4a22854..24975a6cb18f5 100644 --- a/packages/compiler/src/shadow_css.ts +++ b/packages/compiler/src/shadow_css.ts @@ -810,9 +810,9 @@ export class ShadowCss { let scopedSelector = ''; let startIndex = 0; let res: RegExpExecArray | null; - // Spaces aren't used as a delimeter if they are within parenthesis, for example + // Combinators aren't used as a delimeter if they are within parenthesis, for example // `:where(.one .two)` stays intact. - const sep = /( (?![^\(]*\))|>|\+|~(?!=))\s*/g; + const sep = /( |>|\+|~(?!=))(?![^\(]*\))\s*/g; // If a selector appears before :host it should not be shimmed as it // matches on ancestor elements and not on elements in the host's shadow diff --git a/packages/compiler/test/shadow_css/shadow_css_spec.ts b/packages/compiler/test/shadow_css/shadow_css_spec.ts index 49ebf51cf938e..915fad2dc2271 100644 --- a/packages/compiler/test/shadow_css/shadow_css_spec.ts +++ b/packages/compiler/test/shadow_css/shadow_css_spec.ts @@ -104,6 +104,15 @@ describe('ShadowCss', () => { expect(shim(':where(.one, .two) {}', 'contenta', 'hosta')).toEqualCss( ':where(.one[contenta], .two[contenta]) {}', ); + expect(shim(':where(.one > .two) {}', 'contenta', 'hosta')).toEqualCss( + ':where(.one[contenta] > .two[contenta]) {}', + ); + expect(shim(':where(> .one) {}', 'contenta', 'hosta')).toEqualCss( + ':where( > .one[contenta]) {}', + ); + expect(shim(':where(:not(.one) ~ .two) {}', 'contenta', 'hosta')).toEqualCss( + ':where([contenta]:not(.one) ~ .two[contenta]) {}', + ); // :is() expect(shim('div:is(.foo) {}', 'contenta', 'a-host')).toEqualCss('div[contenta]:is(.foo) {}'); @@ -148,6 +157,9 @@ describe('ShadowCss', () => { expect(shim(':has(a) :host :has(b) {}', 'contenta', 'hosta')).toEqualCss( ':has(a) [hosta] [contenta]:has(b) {}', ); + expect(shim('div:has(~ .one) {}', 'contenta', 'hosta')).toEqualCss( + 'div[contenta]:has(~ .one) {}', + ); // Unlike `:is()` or `:where()` the attribute selector isn't placed inside // of `:has()`. That is deliberate, `[contenta]:has(a)` would select all // `[contenta]` with `a` inside, while `:has(a[contenta])` would select From 292ea4714fb7e76cf1748d2f9059991e05c42574 Mon Sep 17 00:00:00 2001 From: Georgy Serga Date: Fri, 13 Sep 2024 04:49:28 +0000 Subject: [PATCH 11/15] fix(compiler): fix comment typo (#57796) fix spelling in the comment PR Close #57796 --- packages/compiler/src/shadow_css.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/compiler/src/shadow_css.ts b/packages/compiler/src/shadow_css.ts index 24975a6cb18f5..21f2484d53584 100644 --- a/packages/compiler/src/shadow_css.ts +++ b/packages/compiler/src/shadow_css.ts @@ -810,8 +810,8 @@ export class ShadowCss { let scopedSelector = ''; let startIndex = 0; let res: RegExpExecArray | null; - // Combinators aren't used as a delimeter if they are within parenthesis, for example - // `:where(.one .two)` stays intact. + // Combinators aren't used as a delimiter if they are within parenthesis, + // for example `:where(.one .two)` stays intact. const sep = /( |>|\+|~(?!=))(?![^\(]*\))\s*/g; // If a selector appears before :host it should not be shimmed as it From e8d1944999e1fdfbd67630d475334c0d7f41a0eb Mon Sep 17 00:00:00 2001 From: Georgy Serga Date: Mon, 23 Sep 2024 06:20:32 +0000 Subject: [PATCH 12/15] fix(compiler): add multiple :host and nested selectors support (#57796) add support for nested and deeply nested (up to three levels) selectors, parse multiple :host selectors, scope selectors within pseudo functions PR Close #57796 --- packages/compiler/src/shadow_css.ts | 109 +++++++++++++----- .../test/shadow_css/shadow_css_spec.ts | 65 ++++++++++- 2 files changed, 140 insertions(+), 34 deletions(-) diff --git a/packages/compiler/src/shadow_css.ts b/packages/compiler/src/shadow_css.ts index 21f2484d53584..ddb85cd6b0bec 100644 --- a/packages/compiler/src/shadow_css.ts +++ b/packages/compiler/src/shadow_css.ts @@ -340,7 +340,7 @@ export class ShadowCss { * captures how many (if any) leading whitespaces are present or a comma * - (?:(?:(['"])((?:\\\\|\\\2|(?!\2).)+)\2)|(-?[A-Za-z][\w\-]*)) * captures two different possible keyframes, ones which are quoted or ones which are valid css - * idents (custom properties excluded) + * indents (custom properties excluded) * - (?=[,\s;]|$) * simply matches the end of the possible keyframe, valid endings are: a comma, a space, a * semicolon or the end of the string @@ -461,7 +461,7 @@ export class ShadowCss { */ private _scopeCssText(cssText: string, scopeSelector: string, hostSelector: string): string { const unscopedRules = this._extractUnscopedRulesFromCssText(cssText); - // replace :host and :host-context -shadowcsshost and -shadowcsshost respectively + // replace :host and :host-context with -shadowcsshost and -shadowcsshostcontext respectively cssText = this._insertPolyfillHostInCssText(cssText); cssText = this._convertColonHost(cssText); cssText = this._convertColonHostContext(cssText); @@ -618,7 +618,12 @@ export class ShadowCss { let selector = rule.selector; let content = rule.content; if (rule.selector[0] !== '@') { - selector = this._scopeSelector({selector, scopeSelector, hostSelector}); + selector = this._scopeSelector({ + selector, + scopeSelector, + hostSelector, + isParentSelector: true, + }); } else if (scopedAtRuleIdentifiers.some((atRule) => rule.selector.startsWith(atRule))) { content = this._scopeSelectors(rule.content, scopeSelector, hostSelector); } else if (rule.selector.startsWith('@font-face') || rule.selector.startsWith('@page')) { @@ -658,20 +663,30 @@ export class ShadowCss { }); } + private _safeSelector: SafeSelector | undefined; + private _shouldScopeIndicator: boolean | undefined; + + // `isParentSelector` is used to distinguish the selectors which are coming from + // the initial selector string and any nested selectors, parsed recursively, + // for example `selector = 'a:where(.one)'` could be the parent, while recursive call + // would have `selector = '.one'`. private _scopeSelector({ selector, scopeSelector, hostSelector, - shouldScope, + isParentSelector = false, }: { selector: string; scopeSelector: string; hostSelector: string; - shouldScope?: boolean; + isParentSelector?: boolean; }): string { // Split the selector into independent parts by `,` (comma) unless // comma is within parenthesis, for example `:is(.one, two)`. - const selectorSplitRe = / ?,(?![^\(]*\)) ?/; + // Negative lookup after comma allows not splitting inside nested parenthesis, + // up to three levels (((,))). + const selectorSplitRe = + / ?,(?!(?:[^)(]*(?:\([^)(]*(?:\([^)(]*(?:\([^)(]*\)[^)(]*)*\)[^)(]*)*\)[^)(]*)*\))) ?/; return selector .split(selectorSplitRe) @@ -684,7 +699,7 @@ export class ShadowCss { selector: shallowPart, scopeSelector, hostSelector, - shouldScope, + isParentSelector, }); } else { return shallowPart; @@ -718,9 +733,9 @@ export class ShadowCss { if (_polyfillHostRe.test(selector)) { const replaceBy = `[${hostSelector}]`; return selector - .replace(_polyfillHostNoCombinatorRe, (hnc, selector) => { + .replace(_polyfillHostNoCombinatorReGlobal, (_hnc, selector) => { return selector.replace( - /([^:]*)(:*)(.*)/, + /([^:\)]*)(:*)(.*)/, (_: string, before: string, colon: string, after: string) => { return before + replaceBy + colon + after; }, @@ -738,12 +753,12 @@ export class ShadowCss { selector, scopeSelector, hostSelector, - shouldScope, + isParentSelector, }: { selector: string; scopeSelector: string; hostSelector: string; - shouldScope?: boolean; + isParentSelector?: boolean; }): string { const isRe = /\[is=([^\]]*)\]/g; scopeSelector = scopeSelector.replace(isRe, (_: string, ...parts: string[]) => parts[0]); @@ -759,6 +774,10 @@ export class ShadowCss { if (p.includes(_polyfillHostNoCombinator)) { scopedP = this._applySimpleSelectorScope(p, scopeSelector, hostSelector); + if (_polyfillHostNoCombinatorWithinPseudoFunction.test(p)) { + const [_, before, colon, after] = scopedP.match(/([^:]*)(:*)(.*)/)!; + scopedP = before + attrName + colon + after; + } } else { // remove :host since it should be unnecessary const t = p.replace(_polyfillHostRe, ''); @@ -775,44 +794,66 @@ export class ShadowCss { // Wraps `_scopeSelectorPart()` to not use it directly on selectors with // pseudo selector functions like `:where()`. Selectors within pseudo selector - // functions are recursively sent to `_scopeSelector()` with the `shouldScope` - // argument, so the selectors get scoped correctly. + // functions are recursively sent to `_scopeSelector()`. const _pseudoFunctionAwareScopeSelectorPart = (selectorPart: string) => { let scopedPart = ''; - const cssPseudoSelectorFunctionMatch = selectorPart.match(_cssPseudoSelectorFunctionPrefix); - if (cssPseudoSelectorFunctionMatch) { - const [cssPseudoSelectorFunction] = cssPseudoSelectorFunctionMatch; + const cssPrefixWithPseudoSelectorFunctionMatch = selectorPart.match( + _cssPrefixWithPseudoSelectorFunction, + ); + if (cssPrefixWithPseudoSelectorFunctionMatch) { + const [cssPseudoSelectorFunction, mainSelector, pseudoSelector] = + cssPrefixWithPseudoSelectorFunctionMatch; + const hasOuterHostNoCombinator = mainSelector.includes(_polyfillHostNoCombinator); + const scopedMainSelector = mainSelector.replace( + _polyfillHostNoCombinatorReGlobal, + `[${hostSelector}]`, + ); + // Unwrap the pseudo selector, to scope its contents. - // For example, `:where(selectorToScope)` -> `selectorToScope`. + // For example, + // - `:where(selectorToScope)` -> `selectorToScope`; + // - `div:is(.foo, .bar)` -> `.foo, .bar`. const selectorToScope = selectorPart.slice(cssPseudoSelectorFunction.length, -1); + if (selectorToScope.includes(_polyfillHostNoCombinator)) { + this._shouldScopeIndicator = true; + } + const scopedInnerPart = this._scopeSelector({ selector: selectorToScope, scopeSelector, hostSelector, - shouldScope: shouldScopeIndicator, }); + // Put the result back into the pseudo selector function. - scopedPart = `${cssPseudoSelectorFunction}${scopedInnerPart})`; + scopedPart = `${scopedMainSelector}:${pseudoSelector}(${scopedInnerPart})`; + + this._shouldScopeIndicator = this._shouldScopeIndicator || hasOuterHostNoCombinator; } else { - shouldScopeIndicator = - shouldScopeIndicator || selectorPart.includes(_polyfillHostNoCombinator); - scopedPart = shouldScopeIndicator ? _scopeSelectorPart(selectorPart) : selectorPart; + this._shouldScopeIndicator = + this._shouldScopeIndicator || selectorPart.includes(_polyfillHostNoCombinator); + scopedPart = this._shouldScopeIndicator ? _scopeSelectorPart(selectorPart) : selectorPart; } return scopedPart; }; - const safeContent = new SafeSelector(selector); - selector = safeContent.content(); + if (isParentSelector) { + this._safeSelector = new SafeSelector(selector); + selector = this._safeSelector.content(); + } let scopedSelector = ''; let startIndex = 0; let res: RegExpExecArray | null; // Combinators aren't used as a delimiter if they are within parenthesis, // for example `:where(.one .two)` stays intact. - const sep = /( |>|\+|~(?!=))(?![^\(]*\))\s*/g; + // Similarly to selector separation by comma initially, negative lookahead + // is used here to not break selectors within nested parenthesis up to three + // nested layers. + const sep = + /( |>|\+|~(?!=))(?!([^)(]*(?:\([^)(]*(?:\([^)(]*(?:\([^)(]*\)[^)(]*)*\)[^)(]*)*\)[^)(]*)*\)))\s*/g; // If a selector appears before :host it should not be shimmed as it // matches on ancestor elements and not on elements in the host's shadow @@ -826,8 +867,13 @@ export class ShadowCss { // - `tag :host` -> `tag [h]` (`tag` is not scoped because it's considered part of a // `:host-context(tag)`) const hasHost = selector.includes(_polyfillHostNoCombinator); - // Only scope parts after the first `-shadowcsshost-no-combinator` when it is present - let shouldScopeIndicator = shouldScope ?? !hasHost; + // Only scope parts after or on the same level as the first `-shadowcsshost-no-combinator` + // when it is present. The selector has the same level when it is a part of a pseudo + // selector, like `:where()`, for example `:where(:host, .foo)` would result in `.foo` + // being scoped. + if (isParentSelector || this._shouldScopeIndicator) { + this._shouldScopeIndicator = !hasHost; + } while ((res = sep.exec(selector)) !== null) { const separator = res[1]; @@ -855,7 +901,8 @@ export class ShadowCss { scopedSelector += _pseudoFunctionAwareScopeSelectorPart(part); // replace the placeholders with their original values - return safeContent.restore(scopedSelector); + // using values stored inside the `safeSelector` instance. + return this._safeSelector!.restore(scopedSelector); } private _insertPolyfillHostInCssText(selector: string): string { @@ -920,7 +967,7 @@ class SafeSelector { } } -const _cssPseudoSelectorFunctionPrefix = /^:(where|is)\(/gi; +const _cssPrefixWithPseudoSelectorFunction = /^([^:]*):(where|is)\(/i; const _cssContentNextSelectorRe = /polyfill-next-selector[^}]*content:[\s]*?(['"])(.*?)\1[;\s]*}([^{]*?){/gim; const _cssContentRuleRe = /(polyfill-rule)[^}]*(content:[\s]*(['"])(.*?)\3)[;\s]*[^}]*}/gim; @@ -934,7 +981,11 @@ const _cssColonHostRe = new RegExp(_polyfillHost + _parenSuffix, 'gim'); const _cssColonHostContextReGlobal = new RegExp(_polyfillHostContext + _parenSuffix, 'gim'); const _cssColonHostContextRe = new RegExp(_polyfillHostContext + _parenSuffix, 'im'); const _polyfillHostNoCombinator = _polyfillHost + '-no-combinator'; +const _polyfillHostNoCombinatorWithinPseudoFunction = new RegExp( + `:.*(.*${_polyfillHostNoCombinator}.*)`, +); const _polyfillHostNoCombinatorRe = /-shadowcsshost-no-combinator([^\s]*)/; +const _polyfillHostNoCombinatorReGlobal = new RegExp(_polyfillHostNoCombinatorRe, 'g'); const _shadowDOMSelectorsRe = [ /::shadow/g, /::content/g, diff --git a/packages/compiler/test/shadow_css/shadow_css_spec.ts b/packages/compiler/test/shadow_css/shadow_css_spec.ts index 915fad2dc2271..ff70c407b2aed 100644 --- a/packages/compiler/test/shadow_css/shadow_css_spec.ts +++ b/packages/compiler/test/shadow_css/shadow_css_spec.ts @@ -66,6 +66,8 @@ describe('ShadowCss', () => { expect(shim('one[attr="va lue"] {}', 'contenta')).toEqualCss('one[attr="va lue"][contenta] {}'); expect(shim('one[attr] {}', 'contenta')).toEqualCss('one[attr][contenta] {}'); expect(shim('[is="one"] {}', 'contenta')).toEqualCss('[is="one"][contenta] {}'); + expect(shim('[attr] {}', 'contenta')).toEqualCss('[attr][contenta] {}'); + expect(shim(':host [attr] {}', 'contenta', 'hosta')).toEqualCss('[hosta] [attr][contenta] {}'); }); it('should handle escaped sequences in selectors', () => { @@ -89,6 +91,9 @@ describe('ShadowCss', () => { ':where(.one[contenta]) .two[contenta] {}', ); expect(shim(':where(:host) {}', 'contenta', 'hosta')).toEqualCss(':where([hosta]) {}'); + expect(shim(':where(:host) .one {}', 'contenta', 'hosta')).toEqualCss( + ':where([hosta]) .one[contenta] {}', + ); expect(shim(':where(.one) :where(:host) {}', 'contenta', 'hosta')).toEqualCss( ':where(.one) :where([hosta]) {}', ); @@ -113,10 +118,14 @@ describe('ShadowCss', () => { expect(shim(':where(:not(.one) ~ .two) {}', 'contenta', 'hosta')).toEqualCss( ':where([contenta]:not(.one) ~ .two[contenta]) {}', ); + expect(shim(':where([foo]) {}', 'contenta', 'hosta')).toEqualCss(':where([foo][contenta]) {}'); // :is() - expect(shim('div:is(.foo) {}', 'contenta', 'a-host')).toEqualCss('div[contenta]:is(.foo) {}'); + expect(shim('div:is(.foo) {}', 'contenta', 'a-host')).toEqualCss('div:is(.foo[contenta]) {}'); expect(shim(':is(.dark :host) {}', 'contenta', 'a-host')).toEqualCss(':is(.dark [a-host]) {}'); + expect(shim(':is(.dark) :is(:host) {}', 'contenta', 'a-host')).toEqualCss( + ':is(.dark) :is([a-host]) {}', + ); expect(shim(':host:is(.foo) {}', 'contenta', 'a-host')).toEqualCss('[a-host]:is(.foo) {}'); expect(shim(':is(.foo) {}', 'contenta', 'a-host')).toEqualCss(':is(.foo[contenta]) {}'); expect(shim(':is(.foo, .bar, .baz) {}', 'contenta', 'a-host')).toEqualCss( @@ -136,10 +145,34 @@ describe('ShadowCss', () => { ).toEqualCss( ':is(.foo, .bar) :is(.baz) :where(.one, .two) [a-host] :where(.three[contenta]:first-child) {}', ); + expect(shim(':where(:is(a)) {}', 'contenta', 'hosta')).toEqualCss( + ':where(:is(a[contenta])) {}', + ); + expect(shim(':where(:is(a, b)) {}', 'contenta', 'hosta')).toEqualCss( + ':where(:is(a[contenta], b[contenta])) {}', + ); + expect(shim(':where(:host:is(.one, .two)) {}', 'contenta', 'hosta')).toEqualCss( + ':where([hosta]:is(.one, .two)) {}', + ); + expect(shim(':where(:host :is(.one, .two)) {}', 'contenta', 'hosta')).toEqualCss( + ':where([hosta] :is(.one[contenta], .two[contenta])) {}', + ); + expect(shim(':where(:is(a, b) :is(.one, .two)) {}', 'contenta', 'hosta')).toEqualCss( + ':where(:is(a[contenta], b[contenta]) :is(.one[contenta], .two[contenta])) {}', + ); + expect( + shim( + ':where(:where(a:has(.foo), b) :is(.one, .two:where(.foo > .bar))) {}', + 'contenta', + 'hosta', + ), + ).toEqualCss( + ':where(:where(a[contenta]:has(.foo), b[contenta]) :is(.one[contenta], .two:where(.foo[contenta] > .bar[contenta]))) {}', + ); // complex selectors expect(shim(':host:is([foo],[foo-2])>div.example-2 {}', 'contenta', 'a-host')).toEqualCss( - '[a-host]:is([foo],[foo-2]) > div.example-2[contenta] {}', + '[a-host]:is([foo], [foo-2]) > div.example-2[contenta] {}', ); expect(shim(':host:is([foo], [foo-2]) > div.example-2 {}', 'contenta', 'a-host')).toEqualCss( '[a-host]:is([foo], [foo-2]) > div.example-2[contenta] {}', @@ -147,9 +180,6 @@ describe('ShadowCss', () => { expect(shim(':host:has([foo],[foo-2])>div.example-2 {}', 'contenta', 'a-host')).toEqualCss( '[a-host]:has([foo],[foo-2]) > div.example-2[contenta] {}', ); - expect(shim(':host:is([foo], [foo-2]) > div.example-2 {}', 'contenta', 'a-host')).toEqualCss( - '[a-host]:is([foo], [foo-2]) > div.example-2[contenta] {}', - ); // :has() expect(shim('div:has(a) {}', 'contenta', 'hosta')).toEqualCss('div[contenta]:has(a) {}'); @@ -170,6 +200,31 @@ describe('ShadowCss', () => { ); }); + it('should handle :host inclusions inside pseudo-selectors selectors', () => { + expect(shim('.header:not(.admin) {}', 'contenta', 'hosta')).toEqualCss( + '.header[contenta]:not(.admin) {}', + ); + expect(shim('.header:is(:host > .toolbar, :host ~ .panel) {}', 'contenta', 'hosta')).toEqualCss( + '.header:is([hosta] > .toolbar[contenta], [hosta] ~ .panel[contenta]) {}', + ); + expect( + shim('.header:where(:host > .toolbar, :host ~ .panel) {}', 'contenta', 'hosta'), + ).toEqualCss('.header:where([hosta] > .toolbar[contenta], [hosta] ~ .panel[contenta]) {}'); + expect(shim('.header:not(.admin, :host.super .header) {}', 'contenta', 'hosta')).toEqualCss( + '.header[contenta]:not(.admin, .super[hosta] .header) {}', + ); + expect( + shim('.header:not(.admin, :host.super .header, :host.mega .header) {}', 'contenta', 'hosta'), + ).toEqualCss('.header[contenta]:not(.admin, .super[hosta] .header, .mega[hosta] .header) {}'); + + expect(shim('.one :where(.two, :host) {}', 'contenta', 'hosta')).toEqualCss( + '.one :where(.two[contenta], [hosta]) {}', + ); + expect(shim('.one :where(:host, .two) {}', 'contenta', 'hosta')).toEqualCss( + '.one :where([hosta], .two[contenta]) {}', + ); + }); + it('should handle escaped selector with space (if followed by a hex char)', () => { // When esbuild runs with optimization.minify // selectors are escaped: .über becomes .\fc ber. From 2374b87b643e0373f85cf126d4b01b2fff785f64 Mon Sep 17 00:00:00 2001 From: Georgy Serga Date: Mon, 23 Sep 2024 17:55:48 +0000 Subject: [PATCH 13/15] fix(compiler): preserve attributes attached to :host selector (#57796) keep attributes used to scope :host selectors PR Close #57796 --- packages/compiler/src/shadow_css.ts | 3 ++- packages/compiler/test/shadow_css/shadow_css_spec.ts | 10 ++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/compiler/src/shadow_css.ts b/packages/compiler/src/shadow_css.ts index ddb85cd6b0bec..00769edc852b2 100644 --- a/packages/compiler/src/shadow_css.ts +++ b/packages/compiler/src/shadow_css.ts @@ -806,7 +806,7 @@ export class ShadowCss { cssPrefixWithPseudoSelectorFunctionMatch; const hasOuterHostNoCombinator = mainSelector.includes(_polyfillHostNoCombinator); const scopedMainSelector = mainSelector.replace( - _polyfillHostNoCombinatorReGlobal, + _polyfillExactHostNoCombinatorReGlobal, `[${hostSelector}]`, ); @@ -984,6 +984,7 @@ const _polyfillHostNoCombinator = _polyfillHost + '-no-combinator'; const _polyfillHostNoCombinatorWithinPseudoFunction = new RegExp( `:.*(.*${_polyfillHostNoCombinator}.*)`, ); +const _polyfillExactHostNoCombinatorReGlobal = /-shadowcsshost-no-combinator/g; const _polyfillHostNoCombinatorRe = /-shadowcsshost-no-combinator([^\s]*)/; const _polyfillHostNoCombinatorReGlobal = new RegExp(_polyfillHostNoCombinatorRe, 'g'); const _shadowDOMSelectorsRe = [ diff --git a/packages/compiler/test/shadow_css/shadow_css_spec.ts b/packages/compiler/test/shadow_css/shadow_css_spec.ts index ff70c407b2aed..573af91852042 100644 --- a/packages/compiler/test/shadow_css/shadow_css_spec.ts +++ b/packages/compiler/test/shadow_css/shadow_css_spec.ts @@ -67,7 +67,17 @@ describe('ShadowCss', () => { expect(shim('one[attr] {}', 'contenta')).toEqualCss('one[attr][contenta] {}'); expect(shim('[is="one"] {}', 'contenta')).toEqualCss('[is="one"][contenta] {}'); expect(shim('[attr] {}', 'contenta')).toEqualCss('[attr][contenta] {}'); + }); + + it('should transform :host with attributes and pseudo selectors', () => { expect(shim(':host [attr] {}', 'contenta', 'hosta')).toEqualCss('[hosta] [attr][contenta] {}'); + expect(shim(':host(create-first-project) {}', 'contenta', 'hosta')).toEqualCss( + 'create-first-project[hosta] {}', + ); + expect(shim(':host[attr] {}', 'contenta', 'hosta')).toEqualCss('[attr][hosta] {}'); + expect(shim(':host[attr]:where(:not(.cm-button)) {}', 'contenta', 'hosta')).toEqualCss( + '[hosta][attr]:where(:not(.cm-button)) {}', + ); }); it('should handle escaped sequences in selectors', () => { From 69529d8873fbd7888ab68fddc6e7c654c5065764 Mon Sep 17 00:00:00 2001 From: Georgy Serga Date: Tue, 24 Sep 2024 06:55:35 +0000 Subject: [PATCH 14/15] fix(compiler): fix parsing of the :host-context with pseudo selectors (#57796) fix regexp which is used to test for host inside pseudo selectors PR Close #57796 --- packages/compiler/src/shadow_css.ts | 2 +- packages/compiler/test/shadow_css/shadow_css_spec.ts | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/compiler/src/shadow_css.ts b/packages/compiler/src/shadow_css.ts index 00769edc852b2..76007cf03d99a 100644 --- a/packages/compiler/src/shadow_css.ts +++ b/packages/compiler/src/shadow_css.ts @@ -982,7 +982,7 @@ const _cssColonHostContextReGlobal = new RegExp(_polyfillHostContext + _parenSuf const _cssColonHostContextRe = new RegExp(_polyfillHostContext + _parenSuffix, 'im'); const _polyfillHostNoCombinator = _polyfillHost + '-no-combinator'; const _polyfillHostNoCombinatorWithinPseudoFunction = new RegExp( - `:.*(.*${_polyfillHostNoCombinator}.*)`, + `:.*\\(.*${_polyfillHostNoCombinator}.*\\)`, ); const _polyfillExactHostNoCombinatorReGlobal = /-shadowcsshost-no-combinator/g; const _polyfillHostNoCombinatorRe = /-shadowcsshost-no-combinator([^\s]*)/; diff --git a/packages/compiler/test/shadow_css/shadow_css_spec.ts b/packages/compiler/test/shadow_css/shadow_css_spec.ts index 573af91852042..452e0393547f0 100644 --- a/packages/compiler/test/shadow_css/shadow_css_spec.ts +++ b/packages/compiler/test/shadow_css/shadow_css_spec.ts @@ -69,7 +69,7 @@ describe('ShadowCss', () => { expect(shim('[attr] {}', 'contenta')).toEqualCss('[attr][contenta] {}'); }); - it('should transform :host with attributes and pseudo selectors', () => { + it('should transform :host and :host-context with attributes and pseudo selectors', () => { expect(shim(':host [attr] {}', 'contenta', 'hosta')).toEqualCss('[hosta] [attr][contenta] {}'); expect(shim(':host(create-first-project) {}', 'contenta', 'hosta')).toEqualCss( 'create-first-project[hosta] {}', @@ -78,6 +78,11 @@ describe('ShadowCss', () => { expect(shim(':host[attr]:where(:not(.cm-button)) {}', 'contenta', 'hosta')).toEqualCss( '[hosta][attr]:where(:not(.cm-button)) {}', ); + expect( + shim(':host-context(backdrop:not(.borderless)) .backdrop {}', 'contenta', 'hosta'), + ).toEqualCss( + 'backdrop:not(.borderless)[hosta] .backdrop[contenta], backdrop:not(.borderless) [hosta] .backdrop[contenta] {}', + ); }); it('should handle escaped sequences in selectors', () => { From 46a6324c82a41b69c16a4c8c9f3fc52d1ecf6917 Mon Sep 17 00:00:00 2001 From: Georgy Serga Date: Thu, 26 Sep 2024 21:25:44 +0000 Subject: [PATCH 15/15] fix(compiler): scope :host-context inside pseudo selectors, do not decrease specificity (#57796) parse constructions like `:where(:host-context(.foo))` correctly revert logic which lead to decreased specificity if `:where` was applied to another selector, for example `div` is transformed to `div[contenta]` with specificity of (0,1,1) so `div:where(.foo)` should not decrease it leading to `div[contenta]:where(.foo)` with the same specificity (0,1,1) instead of `div:where(.foo[contenta])` with specificity equal to (0,0,1) PR Close #57796 --- packages/compiler/src/shadow_css.ts | 45 ++++++++++--------- .../shadow_css/host_and_host_context_spec.ts | 36 +++++++++++++++ .../test/shadow_css/shadow_css_spec.ts | 40 ++++++++++++----- 3 files changed, 87 insertions(+), 34 deletions(-) diff --git a/packages/compiler/src/shadow_css.ts b/packages/compiler/src/shadow_css.ts index 76007cf03d99a..1d7988568fbf4 100644 --- a/packages/compiler/src/shadow_css.ts +++ b/packages/compiler/src/shadow_css.ts @@ -541,7 +541,7 @@ export class ShadowCss { * .foo .bar { ... } */ private _convertColonHostContext(cssText: string): string { - return cssText.replace(_cssColonHostContextReGlobal, (selectorText) => { + return cssText.replace(_cssColonHostContextReGlobal, (selectorText, pseudoPrefix) => { // We have captured a selector that contains a `:host-context` rule. // For backward compatibility `:host-context` may contain a comma separated list of selectors. @@ -596,10 +596,12 @@ export class ShadowCss { } // The context selectors now must be combined with each other to capture all the possible - // selectors that `:host-context` can match. See `combineHostContextSelectors()` for more + // selectors that `:host-context` can match. See `_combineHostContextSelectors()` for more // info about how this is done. return contextSelectorGroups - .map((contextSelectors) => combineHostContextSelectors(contextSelectors, selectorText)) + .map((contextSelectors) => + _combineHostContextSelectors(contextSelectors, selectorText, pseudoPrefix), + ) .join(', '); }); } @@ -802,18 +804,12 @@ export class ShadowCss { _cssPrefixWithPseudoSelectorFunction, ); if (cssPrefixWithPseudoSelectorFunctionMatch) { - const [cssPseudoSelectorFunction, mainSelector, pseudoSelector] = - cssPrefixWithPseudoSelectorFunctionMatch; - const hasOuterHostNoCombinator = mainSelector.includes(_polyfillHostNoCombinator); - const scopedMainSelector = mainSelector.replace( - _polyfillExactHostNoCombinatorReGlobal, - `[${hostSelector}]`, - ); - - // Unwrap the pseudo selector, to scope its contents. + const [cssPseudoSelectorFunction] = cssPrefixWithPseudoSelectorFunctionMatch; + + // Unwrap the pseudo selector to scope its contents. // For example, // - `:where(selectorToScope)` -> `selectorToScope`; - // - `div:is(.foo, .bar)` -> `.foo, .bar`. + // - `:is(.foo, .bar)` -> `.foo, .bar`. const selectorToScope = selectorPart.slice(cssPseudoSelectorFunction.length, -1); if (selectorToScope.includes(_polyfillHostNoCombinator)) { @@ -827,9 +823,7 @@ export class ShadowCss { }); // Put the result back into the pseudo selector function. - scopedPart = `${scopedMainSelector}:${pseudoSelector}(${scopedInnerPart})`; - - this._shouldScopeIndicator = this._shouldScopeIndicator || hasOuterHostNoCombinator; + scopedPart = `${cssPseudoSelectorFunction}${scopedInnerPart})`; } else { this._shouldScopeIndicator = this._shouldScopeIndicator || selectorPart.includes(_polyfillHostNoCombinator); @@ -967,7 +961,8 @@ class SafeSelector { } } -const _cssPrefixWithPseudoSelectorFunction = /^([^:]*):(where|is)\(/i; +const _cssScopedPseudoFunctionPrefix = '(:(where|is)\\()?'; +const _cssPrefixWithPseudoSelectorFunction = /^:(where|is)\(/i; const _cssContentNextSelectorRe = /polyfill-next-selector[^}]*content:[\s]*?(['"])(.*?)\1[;\s]*}([^{]*?){/gim; const _cssContentRuleRe = /(polyfill-rule)[^}]*(content:[\s]*(['"])(.*?)\3)[;\s]*[^}]*}/gim; @@ -978,13 +973,15 @@ const _polyfillHost = '-shadowcsshost'; const _polyfillHostContext = '-shadowcsscontext'; const _parenSuffix = '(?:\\((' + '(?:\\([^)(]*\\)|[^)(]*)+?' + ')\\))?([^,{]*)'; const _cssColonHostRe = new RegExp(_polyfillHost + _parenSuffix, 'gim'); -const _cssColonHostContextReGlobal = new RegExp(_polyfillHostContext + _parenSuffix, 'gim'); +const _cssColonHostContextReGlobal = new RegExp( + _cssScopedPseudoFunctionPrefix + '(' + _polyfillHostContext + _parenSuffix + ')', + 'gim', +); const _cssColonHostContextRe = new RegExp(_polyfillHostContext + _parenSuffix, 'im'); const _polyfillHostNoCombinator = _polyfillHost + '-no-combinator'; const _polyfillHostNoCombinatorWithinPseudoFunction = new RegExp( `:.*\\(.*${_polyfillHostNoCombinator}.*\\)`, ); -const _polyfillExactHostNoCombinatorReGlobal = /-shadowcsshost-no-combinator/g; const _polyfillHostNoCombinatorRe = /-shadowcsshost-no-combinator([^\s]*)/; const _polyfillHostNoCombinatorReGlobal = new RegExp(_polyfillHostNoCombinatorRe, 'g'); const _shadowDOMSelectorsRe = [ @@ -1237,7 +1234,11 @@ function unescapeQuotes(str: string, isQuoted: boolean): string { * @param contextSelectors an array of context selectors that will be combined. * @param otherSelectors the rest of the selectors that are not context selectors. */ -function combineHostContextSelectors(contextSelectors: string[], otherSelectors: string): string { +function _combineHostContextSelectors( + contextSelectors: string[], + otherSelectors: string, + pseudoPrefix = '', +): string { const hostMarker = _polyfillHostNoCombinator; _polyfillHostRe.lastIndex = 0; // reset the regex to ensure we get an accurate test const otherSelectorsHasHost = _polyfillHostRe.test(otherSelectors); @@ -1266,8 +1267,8 @@ function combineHostContextSelectors(contextSelectors: string[], otherSelectors: return combined .map((s) => otherSelectorsHasHost - ? `${s}${otherSelectors}` - : `${s}${hostMarker}${otherSelectors}, ${s} ${hostMarker}${otherSelectors}`, + ? `${pseudoPrefix}${s}${otherSelectors}` + : `${pseudoPrefix}${s}${hostMarker}${otherSelectors}, ${pseudoPrefix}${s} ${hostMarker}${otherSelectors}`, ) .join(','); } diff --git a/packages/compiler/test/shadow_css/host_and_host_context_spec.ts b/packages/compiler/test/shadow_css/host_and_host_context_spec.ts index c4dc29c372b4e..5b5689feb2be1 100644 --- a/packages/compiler/test/shadow_css/host_and_host_context_spec.ts +++ b/packages/compiler/test/shadow_css/host_and_host_context_spec.ts @@ -107,6 +107,42 @@ describe('ShadowCss, :host and :host-context', () => { }); describe(':host-context', () => { + it('should transform :host-context with pseudo selectors', () => { + expect( + shim(':host-context(backdrop:not(.borderless)) .backdrop {}', 'contenta', 'hosta'), + ).toEqualCss( + 'backdrop:not(.borderless)[hosta] .backdrop[contenta], backdrop:not(.borderless) [hosta] .backdrop[contenta] {}', + ); + expect(shim(':where(:host-context(backdrop)) {}', 'contenta', 'hosta')).toEqualCss( + ':where(backdrop[hosta]), :where(backdrop [hosta]) {}', + ); + expect(shim(':where(:host-context(outer1)) :host(bar) {}', 'contenta', 'hosta')).toEqualCss( + ':where(outer1) bar[hosta] {}', + ); + expect( + shim(':where(:host-context(.one)) :where(:host-context(.two)) {}', 'contenta', 'a-host'), + ).toEqualCss( + ':where(.one.two[a-host]), ' + // `one` and `two` both on the host + ':where(.one.two [a-host]), ' + // `one` and `two` are both on the same ancestor + ':where(.one .two[a-host]), ' + // `one` is an ancestor and `two` is on the host + ':where(.one .two [a-host]), ' + // `one` and `two` are both ancestors (in that order) + ':where(.two .one[a-host]), ' + // `two` is an ancestor and `one` is on the host + ':where(.two .one [a-host])' + // `two` and `one` are both ancestors (in that order) + ' {}', + ); + expect( + shim(':where(:host-context(backdrop)) .foo ~ .bar {}', 'contenta', 'hosta'), + ).toEqualCss( + ':where(backdrop[hosta]) .foo[contenta] ~ .bar[contenta], :where(backdrop [hosta]) .foo[contenta] ~ .bar[contenta] {}', + ); + expect(shim(':where(:host-context(backdrop)) :host {}', 'contenta', 'hosta')).toEqualCss( + ':where(backdrop) [hosta] {}', + ); + expect(shim('div:where(:host-context(backdrop)) :host {}', 'contenta', 'hosta')).toEqualCss( + 'div:where(backdrop) [hosta] {}', + ); + }); + it('should handle tag selector', () => { expect(shim(':host-context(div) {}', 'contenta', 'a-host')).toEqualCss( 'div[a-host], div [a-host] {}', diff --git a/packages/compiler/test/shadow_css/shadow_css_spec.ts b/packages/compiler/test/shadow_css/shadow_css_spec.ts index 452e0393547f0..77a0a361a3199 100644 --- a/packages/compiler/test/shadow_css/shadow_css_spec.ts +++ b/packages/compiler/test/shadow_css/shadow_css_spec.ts @@ -69,19 +69,14 @@ describe('ShadowCss', () => { expect(shim('[attr] {}', 'contenta')).toEqualCss('[attr][contenta] {}'); }); - it('should transform :host and :host-context with attributes and pseudo selectors', () => { + it('should transform :host with attributes', () => { expect(shim(':host [attr] {}', 'contenta', 'hosta')).toEqualCss('[hosta] [attr][contenta] {}'); expect(shim(':host(create-first-project) {}', 'contenta', 'hosta')).toEqualCss( 'create-first-project[hosta] {}', ); expect(shim(':host[attr] {}', 'contenta', 'hosta')).toEqualCss('[attr][hosta] {}'); expect(shim(':host[attr]:where(:not(.cm-button)) {}', 'contenta', 'hosta')).toEqualCss( - '[hosta][attr]:where(:not(.cm-button)) {}', - ); - expect( - shim(':host-context(backdrop:not(.borderless)) .backdrop {}', 'contenta', 'hosta'), - ).toEqualCss( - 'backdrop:not(.borderless)[hosta] .backdrop[contenta], backdrop:not(.borderless) [hosta] .backdrop[contenta] {}', + '[attr][hosta]:where(:not(.cm-button)) {}', ); }); @@ -94,6 +89,27 @@ describe('ShadowCss', () => { expect(shim('.one\\:two .three\\:four {}', 'contenta')).toEqualCss( '.one\\:two[contenta] .three\\:four[contenta] {}', ); + expect(shim('div:where(.one) {}', 'contenta', 'hosta')).toEqualCss( + 'div[contenta]:where(.one) {}', + ); + expect(shim('div:where() {}', 'contenta', 'hosta')).toEqualCss('div[contenta]:where() {}'); + // See `xit('should parse concatenated pseudo selectors'` + expect(shim(':where(a):where(b) {}', 'contenta', 'hosta')).toEqualCss( + ':where(a)[contenta]:where(b) {}', + ); + expect(shim('*:where(.one) {}', 'contenta', 'hosta')).toEqualCss('*[contenta]:where(.one) {}'); + expect(shim('*:where(.one) ::ng-deep .foo {}', 'contenta', 'hosta')).toEqualCss( + '*[contenta]:where(.one) .foo {}', + ); + }); + + xit('should parse concatenated pseudo selectors', () => { + // Current logic leads to a result with an outer scope + // It could be changed, to not increase specificity + // Requires a more complex parsing + expect(shim(':where(a):where(b) {}', 'contenta', 'hosta')).toEqualCss( + ':where(a[contenta]):where(b[contenta]) {}', + ); }); it('should handle pseudo functions correctly', () => { @@ -136,7 +152,7 @@ describe('ShadowCss', () => { expect(shim(':where([foo]) {}', 'contenta', 'hosta')).toEqualCss(':where([foo][contenta]) {}'); // :is() - expect(shim('div:is(.foo) {}', 'contenta', 'a-host')).toEqualCss('div:is(.foo[contenta]) {}'); + expect(shim('div:is(.foo) {}', 'contenta', 'a-host')).toEqualCss('div[contenta]:is(.foo) {}'); expect(shim(':is(.dark :host) {}', 'contenta', 'a-host')).toEqualCss(':is(.dark [a-host]) {}'); expect(shim(':is(.dark) :is(:host) {}', 'contenta', 'a-host')).toEqualCss( ':is(.dark) :is([a-host]) {}', @@ -182,12 +198,12 @@ describe('ShadowCss', () => { 'hosta', ), ).toEqualCss( - ':where(:where(a[contenta]:has(.foo), b[contenta]) :is(.one[contenta], .two:where(.foo[contenta] > .bar[contenta]))) {}', + ':where(:where(a[contenta]:has(.foo), b[contenta]) :is(.one[contenta], .two[contenta]:where(.foo > .bar))) {}', ); // complex selectors expect(shim(':host:is([foo],[foo-2])>div.example-2 {}', 'contenta', 'a-host')).toEqualCss( - '[a-host]:is([foo], [foo-2]) > div.example-2[contenta] {}', + '[a-host]:is([foo],[foo-2]) > div.example-2[contenta] {}', ); expect(shim(':host:is([foo], [foo-2]) > div.example-2 {}', 'contenta', 'a-host')).toEqualCss( '[a-host]:is([foo], [foo-2]) > div.example-2[contenta] {}', @@ -220,11 +236,11 @@ describe('ShadowCss', () => { '.header[contenta]:not(.admin) {}', ); expect(shim('.header:is(:host > .toolbar, :host ~ .panel) {}', 'contenta', 'hosta')).toEqualCss( - '.header:is([hosta] > .toolbar[contenta], [hosta] ~ .panel[contenta]) {}', + '.header[contenta]:is([hosta] > .toolbar, [hosta] ~ .panel) {}', ); expect( shim('.header:where(:host > .toolbar, :host ~ .panel) {}', 'contenta', 'hosta'), - ).toEqualCss('.header:where([hosta] > .toolbar[contenta], [hosta] ~ .panel[contenta]) {}'); + ).toEqualCss('.header[contenta]:where([hosta] > .toolbar, [hosta] ~ .panel) {}'); expect(shim('.header:not(.admin, :host.super .header) {}', 'contenta', 'hosta')).toEqualCss( '.header[contenta]:not(.admin, .super[hosta] .header) {}', );