From c7530cfe36aeefd3b9521968c13289fd36667b3a Mon Sep 17 00:00:00 2001 From: Artem Belik Date: Fri, 4 Sep 2026 10:20:55 +0300 Subject: [PATCH 1/2] fix(timepicker)!: errors following a full review of the component MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `KbqTimepicker` implements `KbqFormFieldControl`, which declares `value`, `id`, `placeholder`, `required`, `disabled`, `focused`, `empty` and `errorState` as plain members — those stay plain accessors. The four inputs the timepicker owns are signals now. `min` and `max` parsed in their setters and reported the parsed result, so a value the date adapter could not read came back as `null`. They report what was bound; the parsed values stay internal, drive the validators and re-run them from an effect. `kbqValidationTooltip` was a setter-only input that subscribed to `incorrectInput` every time it ran and never unsubscribed — re-binding it stacked another subscription and the last one outlived the directive. It is an effect with a teardown. `format` clamps through the input's transform, and the two side effects its setter carried are subsumed by the locale effect that already re-read the placeholder. That effect no longer returns early on a consumer-provided placeholder, so a locale change reformats the rendered time either way. The generated id comes from the CDK `_IdGenerator`, and the six members that stay accessors say why instead of carrying stale migration TODOs. BREAKING CHANGE: `KbqTimepicker.format`, `min`, `max` and `kbqValidationTooltip` are signal inputs; `min` and `max` report the bound value rather than the parsed one; generated ids changed shape. Reported and partly rewritten by the `timepicker-signals` schematic. Co-Authored-By: Claude Opus 5 --- docs/guides/migration.en.md | 22 +- docs/guides/migration.ru.md | 22 +- .../timepicker/timepicker.directive.ts | 202 +++++------- .../components/timepicker/timepicker.spec.ts | 57 ++++ packages/schematics/src/collection.json | 5 + packages/schematics/src/migrations.json | 5 + .../migrations/timepicker-signals/README.md | 61 ++++ .../src/migrations/timepicker-signals/data.ts | 73 +++++ .../timepicker-signals/index.spec.ts | 212 ++++++++++++ .../migrations/timepicker-signals/index.ts | 305 ++++++++++++++++++ .../migrations/timepicker-signals/schema.json | 20 ++ .../migrations/timepicker-signals/schema.ts | 6 + .../components/timepicker.api.md | 29 +- 13 files changed, 887 insertions(+), 132 deletions(-) create mode 100644 packages/schematics/src/migrations/timepicker-signals/README.md create mode 100644 packages/schematics/src/migrations/timepicker-signals/data.ts create mode 100644 packages/schematics/src/migrations/timepicker-signals/index.spec.ts create mode 100644 packages/schematics/src/migrations/timepicker-signals/index.ts create mode 100644 packages/schematics/src/migrations/timepicker-signals/schema.json create mode 100644 packages/schematics/src/migrations/timepicker-signals/schema.ts diff --git a/docs/guides/migration.en.md b/docs/guides/migration.en.md index ccb9a89267..1c9882ca0c 100644 --- a/docs/guides/migration.en.md +++ b/docs/guides/migration.en.md @@ -1035,7 +1035,7 @@ for each option it deselected and reporting the shortened value to the form cont ### 18. Component review (20.3.0) -Ten components went through a full review in 20.3.0: notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree and tree-select. Each review closed the members that were never part of the component's contract, moved inputs to signals where that was the point of it, and fixed the behavior it uncovered along the way. Only the changes that reach a consumer are listed here. +Components went through a full review in 20.3.0, in two waves. The first covered notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree and tree-select; the second is the one each subsection below belongs to. Each review closed the members that were never part of the component's contract, moved inputs to signals where that was the point of it, and fixed the behavior it uncovered along the way. Only the changes that reach a consumer are listed here. Every schematic named below runs automatically: @@ -1113,6 +1113,26 @@ A `` with no projected button no longer throws outside dev mod Reported by `split-button-optional-disabled`. +#### Timepicker + +`KbqTimepicker` implements `KbqFormFieldControl`, which declares `value`, `id`, `placeholder`, `required`, `disabled`, `focused`, `empty` and `errorState` as plain members — those stay plain accessors. The four inputs the timepicker owns moved. + +`min` and `max` parsed in their setters and reported the parsed result, so an unparseable bound value read back as `null`. They report what was bound now; the parsed values stay internal and still drive the validators. + +| Pattern | Manual migration | +| --------------------------------------------------------------------- | ------------------------------------------------------------------------ | +| `.format` | Read as `format()` — rewritten for you | +| `.min` / `.max` | `min()` / `max()`, and expect the bound value rather than the parsed one | +| `.format = …` / `.min = …` / `.max = …` / `.kbqValidationTooltip = …` | Bind them in the template; the inputs are read-only | + +**`kbqValidationTooltip` unsubscribes.** The setter subscribed to `incorrectInput` every time it ran and never unsubscribed, so re-binding the input stacked another subscription and the last one outlived the directive. It is an effect with a teardown now. + +**A locale change reformats the rendered time even when the placeholder was set by the consumer.** The effect used to return early on a consumer-provided placeholder, which skipped the reformat with it — the two are separate concerns now. + +**Generated ids changed shape**, from `kbq-timepicker-1` to `kbq-timepicker-a1`. + +Handled by `timepicker-signals`: the `format` reads are rewritten, the rest is reported. + #### Title `kbq-title` measures its host and opens a tooltip when the text is truncated. The review kept that surface — the `kbq-title` input and the tooltip it opens — and closed the measurement machinery behind it. diff --git a/docs/guides/migration.ru.md b/docs/guides/migration.ru.md index 73fd0cd51d..67c5069825 100644 --- a/docs/guides/migration.ru.md +++ b/docs/guides/migration.ru.md @@ -1039,7 +1039,7 @@ ng g @koobiq/components:list-tree-multiple-input --project ### 18. Ревью компонентов (20.3.0) -В 20.3.0 полное ревью прошли десять компонентов: notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree и tree-select. Каждое ревью закрывало члены, которые никогда не были частью контракта компонента, переводило входы на сигналы там, где в этом и был его смысл, и попутно исправляло найденные ошибки поведения. Ниже перечислено только то, что доходит до потребителя. +В 20.3.0 полное ревью прошло в две волны. В первую вошли notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree и tree-select; ко второй относится каждый из подразделов ниже. Каждое ревью закрывало члены, которые никогда не были частью контракта компонента, переводило входы на сигналы там, где в этом и был его смысл, и попутно исправляло найденные ошибки поведения. Ниже перечислено только то, что доходит до потребителя. Все схематики, названные ниже, запускаются автоматически: @@ -1117,6 +1117,26 @@ if (splitButton.disabled === false) { Сообщает `split-button-optional-disabled`. +#### Timepicker + +`KbqTimepicker` реализует `KbqFormFieldControl`, который объявляет `value`, `id`, `placeholder`, `required`, `disabled`, `focused`, `empty` и `errorState` обычными членами — они остались обычными геттерами и сеттерами. Переехали четыре входа, которые принадлежат самому `KbqTimepicker`. + +`min` и `max` разбирали значение в сеттерах и возвращали результат разбора, поэтому значение, которое адаптер дат прочитать не смог, читалось обратно как `null`. Теперь они возвращают то, что было привязано; разобранные значения остались внутренними и по-прежнему питают валидаторы. + +| Что было | Как мигрировать вручную | +| --------------------------------------------------------------------- | ------------------------------------------------------------------- | +| `.format` | Читать как `format()` — переписывается за вас | +| `.min` / `.max` | `min()` / `max()`, и ожидать привязанное значение, а не разобранное | +| `.format = …` / `.min = …` / `.max = …` / `.kbqValidationTooltip = …` | Привязать в шаблоне; входы доступны только на чтение | + +**`kbqValidationTooltip` теперь отписывается.** Сеттер подписывался на `incorrectInput` при каждом срабатывании и никогда не отписывался, поэтому повторная привязка входа накапливала ещё одну подписку, а последняя переживала саму директиву. Теперь это `effect` с очисткой. + +**Смена локали переформатирует показанное время, даже если placeholder задал потребитель.** Раньше эффект досрочно выходил при пользовательском placeholder и вместе с ним пропускал переформатирование — теперь это две независимые вещи. + +**Формат генерируемых `id` изменился** с `kbq-timepicker-1` на `kbq-timepicker-a1`. + +Закрывается схематиком `timepicker-signals`: чтения `format` переписываются, остальное сообщается в отчёте. + #### Title `kbq-title` измеряет свой хост и открывает тултип, когда текст обрезан. Ревью сохранило эту поверхность — вход `kbq-title` и открываемый тултип — и закрыло стоящий за ней механизм измерения. diff --git a/packages/components/timepicker/timepicker.directive.ts b/packages/components/timepicker/timepicker.directive.ts index c421ecc520..73708cffce 100644 --- a/packages/components/timepicker/timepicker.directive.ts +++ b/packages/components/timepicker/timepicker.directive.ts @@ -1,6 +1,8 @@ -import { coerceBooleanProperty } from '@angular/cdk/coercion'; +import { _IdGenerator } from '@angular/cdk/a11y'; import { AfterContentInit, + booleanAttribute, + computed, Directive, DoCheck, effect, @@ -9,6 +11,7 @@ import { inject, InjectionToken, Input, + input, OnDestroy, output, Provider, @@ -111,14 +114,16 @@ export const kbqTimepickerLocaleConfigurationProvider = ( configuration: KbqDeepPartial ): Provider => kbqLocaleConfigurationOverrideProvider('timepicker', configuration); -let uniqueComponentIdSuffix: number = 0; - const shortFormatSize: number = 5; const fullFormatSize: number = 8; /** Maximum number of digits in a single time part */ const timePartLength: number = 2; +/** Coerces a time format, falling back to the default for anything the enum does not name. */ +const timeFormatAttribute = (value: unknown): TimeFormats => + Object.values(TimeFormats).includes(value as TimeFormats) ? (value as TimeFormats) : DEFAULT_TIME_FORMAT; + @Directive({ selector: 'input[kbqTimepicker]', providers: [ @@ -146,6 +151,8 @@ const timePartLength: number = 2; export class KbqTimepicker implements KbqFormFieldControl, ControlValueAccessor, Validator, OnDestroy, DoCheck, AfterContentInit { + private readonly uid = inject(_IdGenerator).getId('kbq-timepicker-'); + private elementRef = inject>(ElementRef); private renderer = inject(Renderer2); private dateAdapter = inject>(DateAdapter, { optional: true })!; @@ -170,8 +177,8 @@ export class KbqTimepicker controlType: string = 'timepicker'; /** Object used to control when error messages are shown. */ - // TODO: Skipped for migration because: - // Accessor inputs cannot be migrated as they are too complex. + // Stays an accessor: `CanUpdateErrorState` declares it as a plain member, and it delegates to the + // shared `KbqErrorStateTracker`. @Input() get errorStateMatcher() { return this.errorStateTracker.errorStateMatcher; @@ -185,8 +192,8 @@ export class KbqTimepicker * Implemented as part of KbqFormFieldControl. * @docs-private */ - // TODO: Skipped for migration because: - // Accessor inputs cannot be migrated as they are too complex. + // Stays an accessor: `KbqFormFieldControl` declares `placeholder` as a plain member, and the setter + // records that the consumer took it over from the locale-provided default. @Input() get placeholder(): string { return this._placeholder; @@ -200,15 +207,14 @@ export class KbqTimepicker private _placeholder = TIMEFORMAT_PLACEHOLDERS[DEFAULT_TIME_FORMAT]; - // TODO: Skipped for migration because: - // Accessor inputs cannot be migrated as they are too complex. - @Input() + // Stays an accessor: `KbqFormFieldControl` declares `disabled` as a plain member. + @Input({ transform: booleanAttribute }) get disabled(): boolean { return this._disabled; } set disabled(value: boolean) { - this._disabled = coerceBooleanProperty(value); + this._disabled = value; // Browsers may not fire the blur event if the input is disabled too quickly. // Reset from here to ensure that the element doesn't become stuck. @@ -221,8 +227,7 @@ export class KbqTimepicker private _disabled: boolean; - // TODO: Skipped for migration because: - // Accessor inputs cannot be migrated as they are too complex. + // Stays an accessor: `KbqFormFieldControl` declares `id` as a plain member. @Input() get id(): string { return this._id; @@ -232,81 +237,41 @@ export class KbqTimepicker this._id = value || this.uid; } - private _id: string; + private _id: string = this.uid; /** * Implemented as part of KbqFormFieldControl. * @docs-private */ - // TODO: Skipped for migration because: - // Accessor inputs cannot be migrated as they are too complex. - @Input() + // Stays an accessor: `KbqFormFieldControl` declares `required` as a plain member. + @Input({ transform: booleanAttribute }) get required(): boolean { return this._required; } set required(value: boolean) { - this._required = coerceBooleanProperty(value); + this._required = value; } private _required: boolean; - // TODO: Skipped for migration because: - // Accessor inputs cannot be migrated as they are too complex. - @Input() - get format(): TimeFormats { - return this._format; - } - - set format(formatValue: TimeFormats) { - this._format = - Object.keys(TimeFormats) - .map((timeFormatKey) => TimeFormats[timeFormatKey]) - .indexOf(formatValue) > -1 - ? formatValue - : DEFAULT_TIME_FORMAT; - - if (this.defaultPlaceholder) { - this._placeholder = this.timeFormatPlaceholder; - } - - if (this.value) { - this.updateView(); - } - } - - private _format: TimeFormats = DEFAULT_TIME_FORMAT; + /** Time format the input parses and renders. An unsupported value falls back to the default. */ + readonly format = input(DEFAULT_TIME_FORMAT, { transform: timeFormatAttribute }); - // TODO: Skipped for migration because: - // Accessor inputs cannot be migrated as they are too complex. - @Input() - get min(): D | null { - return this._min; - } + /** Earliest time the control accepts. Anything the date adapter cannot read is treated as unset. */ + readonly min = input(null); - set min(value: D | null) { - this._min = this.getValidDateOrNull(this.dateAdapter.deserialize(value)); - this.validatorOnChange(); - } + /** Latest time the control accepts. Anything the date adapter cannot read is treated as unset. */ + readonly max = input(null); - private _min: D | null = null; + /** `min` as the date adapter reads it, or null when it cannot. */ + private readonly minDate = computed(() => this.getValidDateOrNull(this.dateAdapter.deserialize(this.min()))); - // TODO: Skipped for migration because: - // Accessor inputs cannot be migrated as they are too complex. - @Input() - get max(): D | null { - return this._max; - } + /** `max` as the date adapter reads it, or null when it cannot. */ + private readonly maxDate = computed(() => this.getValidDateOrNull(this.dateAdapter.deserialize(this.max()))); - set max(value: D | null) { - this._max = this.getValidDateOrNull(this.dateAdapter.deserialize(value)); - this.validatorOnChange(); - } - - private _max: D | null = null; - - // TODO: Skipped for migration because: - // Accessor inputs cannot be migrated as they are too complex. + // Stays an accessor: `KbqFormFieldControl` declares `value` as a plain member, and the setter is the + // single place the view is re-rendered from. @Input() get value(): D | null { return this._value; @@ -324,29 +289,8 @@ export class KbqTimepicker private _value: D | null; - // TODO: Skipped for migration because: - // Accessor inputs cannot be migrated as they are too complex. - @Input() - set kbqValidationTooltip(tooltip: KbqTooltipTrigger) { - if (!tooltip) { - return; - } - - tooltip.enterDelay = validationTooltipShowDelay; - tooltip.trigger = 'manual'; - - tooltip.initListeners(); - - this.incorrectInput.subscribe(() => { - if (tooltip.isOpen) { - return; - } - - tooltip.show(); - - setTimeout(() => tooltip.hide(), validationTooltipHideDelay); - }); - } + /** Tooltip shown for a moment whenever a keystroke is rejected. */ + readonly kbqValidationTooltip = input(); readonly incorrectInput = output(); @@ -355,11 +299,11 @@ export class KbqTimepicker } get isFullFormat(): boolean { - return this.format === TimeFormats.HHmmss; + return this.format() === TimeFormats.HHmmss; } get isShortFormat(): boolean { - return this.format === TimeFormats.HHmm; + return this.format() === TimeFormats.HHmm; } get viewValue(): string { @@ -401,8 +345,8 @@ export class KbqTimepicker /** Localized placeholder */ get timeFormatPlaceholder(): string { return ( - this.configuration().placeholder[TimeFormatToLocaleKeys[this.format]] || - TIMEFORMAT_PLACEHOLDERS[this.format] + this.configuration().placeholder[TimeFormatToLocaleKeys[this.format()]] || + TIMEFORMAT_PLACEHOLDERS[this.format()] ); } @@ -415,8 +359,6 @@ export class KbqTimepicker this.errorStateTracker.errorState = value; } - private readonly uid = `kbq-timepicker-${uniqueComponentIdSuffix++}`; - private readonly validator: ValidatorFn | null; private lastValueValid = false; @@ -444,9 +386,6 @@ export class KbqTimepicker this.onChange = noop; this.onTouched = noop; - // Force setter to be called in case id was not specified. - this.id = this.id; - this.errorStateTracker = new KbqErrorStateTracker( inject(ErrorStateMatcher), null, @@ -457,17 +396,48 @@ export class KbqTimepicker effect(() => { // Read before the guard: an early return that skipped it would leave the effect with nothing - // to track, and the next locale change would never reach the input. + // to track, and the next locale or format change would never reach the input. const placeholder = this.timeFormatPlaceholder; - if (!this.defaultPlaceholder) return; + if (this.defaultPlaceholder) { + // Assigned through the private field so the setter does not mark it consumer-provided. + this._placeholder = placeholder; + } - // Assigned through the private field so that the setter does not mark it as consumer-provided. - this._placeholder = placeholder; - // Re-assigning the value re-runs it through the date adapter, which formats on the new locale. + // Re-assigning the value re-runs it through the date adapter, which formats on the new locale + // and the new format. this.value = this._value; }); + // `min` and `max` feed the validators, which Angular only re-runs when it is told to. + effect(() => { + this.minDate(); + this.maxDate(); + + this.validatorOnChange(); + }); + + effect((onCleanup) => { + const tooltip = this.kbqValidationTooltip(); + + if (!tooltip) return; + + tooltip.enterDelay = validationTooltipShowDelay; + tooltip.trigger = 'manual'; + + tooltip.initListeners(); + + const subscription = this.incorrectInput.subscribe(() => { + if (tooltip.isOpen) return; + + tooltip.show(); + + setTimeout(() => tooltip.hide(), validationTooltipHideDelay); + }); + + onCleanup(() => subscription.unsubscribe()); + }); + this.timezoneService.changes.pipe(takeUntilDestroyed()).subscribe(() => { // The rendered text names a wall clock in the zone it was formatted in. Left as it is, the // next keystroke re-parses it against the new zone and emits a different instant. @@ -511,7 +481,7 @@ export class KbqTimepicker onBlur() { this.focusChanged(false); - if (this.viewValue !== this.getTimeStringFromDate(this.value, this.format)) { + if (this.viewValue !== this.getTimeStringFromDate(this.value, this.format())) { this.setViewValue(this.formatUserPaste(this.viewValue)); this.onInput(); @@ -529,7 +499,7 @@ export class KbqTimepicker return; } - this.setViewValue(this.getTimeStringFromDate(newTimeObj, this.format)); + this.setViewValue(this.getTimeStringFromDate(newTimeObj, this.format())); this.value = newTimeObj; this.onChange(newTimeObj); @@ -545,7 +515,7 @@ export class KbqTimepicker const selectionStart = this.selectionStart; const selectionEnd = this.selectionEnd; - const nextViewValue = newTimeObj ? this.getTimeStringFromDate(newTimeObj, this.format) : formattedValue; + const nextViewValue = newTimeObj ? this.getTimeStringFromDate(newTimeObj, this.format()) : formattedValue; // A complete time is always rewritten, so that the caret keeps walking between the time parts. // An incomplete one (e.g. `23:1`) is rewritten only when normalization trimmed it — otherwise // the extra digits stay in the input and the value grows unbounded. @@ -1019,17 +989,21 @@ export class KbqTimepicker private minValidator: ValidatorFn = (control: AbstractControl): ValidationErrors | null => { const controlValue = this.getValidDateOrNull(this.dateAdapter.deserialize(control.value)); - return !this.min || !controlValue || this.dateAdapter.compareDateTime(this.min, controlValue) <= 0 + const min = this.minDate(); + + return !min || !controlValue || this.dateAdapter.compareDateTime(min, controlValue) <= 0 ? null - : { kbqTimepickerLowerThenMin: { min: this.min, actual: controlValue } }; + : { kbqTimepickerLowerThenMin: { min, actual: controlValue } }; }; private maxValidator: ValidatorFn = (control: AbstractControl): ValidationErrors | null => { const controlValue = this.getValidDateOrNull(this.dateAdapter.deserialize(control.value)); - return !this.max || !controlValue || this.dateAdapter.compareDateTime(this.max, controlValue) >= 0 + const max = this.maxDate(); + + return !max || !controlValue || this.dateAdapter.compareDateTime(max, controlValue) >= 0 ? null - : { kbqTimepickerHigherThenMax: { max: this.max, actual: controlValue } }; + : { kbqTimepickerHigherThenMax: { max, actual: controlValue } }; }; private getValidDateOrNull(obj: any): D | null { @@ -1051,7 +1025,7 @@ export class KbqTimepicker } private updateView() { - const formattedValue = this.getTimeStringFromDate(this.value, this.format); + const formattedValue = this.getTimeStringFromDate(this.value, this.format()); this.setViewValue(formattedValue); } diff --git a/packages/components/timepicker/timepicker.spec.ts b/packages/components/timepicker/timepicker.spec.ts index 4e9eee8355..5d859a552e 100644 --- a/packages/components/timepicker/timepicker.spec.ts +++ b/packages/components/timepicker/timepicker.spec.ts @@ -1205,4 +1205,61 @@ describe(KbqTimepicker.name, () => { subscription.unsubscribe(); })); }); + describe('signal inputs', () => { + it('should clamp an unsupported format to the default', () => { + const fixture = createStandaloneComponent(TimepickerSignalInputs); + const timepicker = fixture.componentInstance.timepicker(); + + fixture.componentInstance.timeFormat = 'Hourglass' as TimeFormats; + fixture.detectChanges(); + + expect(timepicker.format()).toBe(DEFAULT_TIME_FORMAT); + }); + + it('should report the bound min and max rather than the parsed ones', () => { + const fixture = createStandaloneComponent(TimepickerSignalInputs); + const timepicker = fixture.componentInstance.timepicker(); + + expect(timepicker.min()).toBeNull(); + expect(timepicker.max()).toBeNull(); + + fixture.componentInstance.min = 'not a time' as unknown as DateTime; + fixture.detectChanges(); + + // The getter used to hand back the parsed value, so an unparseable bound value read as null. + expect(timepicker.min()).toBe('not a time'); + }); + + it('should re-run the validators when min changes', () => { + const fixture = createStandaloneComponent(TimepickerSignalInputs); + const { control } = fixture.componentInstance; + + control.setValue(DateTime.fromObject({ hour: 10, minute: 0 })); + fixture.detectChanges(); + + expect(control.errors).toBeNull(); + + fixture.componentInstance.min = DateTime.fromObject({ hour: 12, minute: 0 }); + fixture.detectChanges(); + + expect(control.errors?.kbqTimepickerLowerThenMin).toBeTruthy(); + }); + }); }); + +@Component({ + imports: [KbqFormFieldModule, KbqTimepickerModule, ReactiveFormsModule, KbqLuxonDateModule], + template: ` + + + + ` +}) +class TimepickerSignalInputs { + readonly timepicker = viewChild.required(KbqTimepicker); + + control = new FormControl(null); + timeFormat: TimeFormats = DEFAULT_TIME_FORMAT; + min: DateTime | null = null; + max: DateTime | null = null; +} diff --git a/packages/schematics/src/collection.json b/packages/schematics/src/collection.json index c5131fd7d8..9ad179dd36 100644 --- a/packages/schematics/src/collection.json +++ b/packages/schematics/src/collection.json @@ -158,6 +158,11 @@ "description": "Reports the breaking parts of the toast review — the members KbqToastComponent lost when it moved from KbqToastService to the KBQ_TOAST_STACK token, the narrowed animation subject and showTemplate context — and notes that the data passed to show() is no longer written to, so its style and icon defaults are resolved inside the toast", "factory": "./migrations/toast-stack-and-defaults/index", "schema": "./migrations/toast-stack-and-defaults/schema.json" + }, + "timepicker-signals": { + "description": "Migrates KbqTimepicker consumers to its signal-based inputs. KbqTimepicker implements KbqFormFieldControl, which declares value, id, placeholder, required, disabled, focused, empty and errorState as plain members, so those stay plain accessors; the four inputs the timepicker owns are signals now. Rewrites programmatic reads of format to calls on receivers typed KbqTimepicker. Warns - without auto-fixing - on min and max, whose getters used to hand back what the date adapter had parsed so an unparseable bound value read as null, on writes to the read-only inputs including kbqValidationTooltip, and on view/content queries that return the instance. Reports that the validation tooltip finally unsubscribes, that a locale change reformats the rendered time even with a consumer-provided placeholder, and that generated ids come from the CDK _IdGenerator.", + "factory": "./migrations/timepicker-signals/index", + "schema": "./migrations/timepicker-signals/schema.json" } } } diff --git a/packages/schematics/src/migrations.json b/packages/schematics/src/migrations.json index c4fe76dcc6..90ea1b9d25 100644 --- a/packages/schematics/src/migrations.json +++ b/packages/schematics/src/migrations.json @@ -120,6 +120,11 @@ "version": "20.3.0-0", "description": "Reports the breaking parts of the toast review. KbqToastComponent resolves its stack through the new KBQ_TOAST_STACK token instead of KbqToastService, so `service` is gone together with `elementRef`, `ttl`, `delay`, `isTemplateRef()`, `themePalette` and `toastStyle`, and everything the template renders became protected; KbqToastService.animation was narrowed from BehaviorSubject to Subject, so `.getValue()` / `.value` no longer exist; showTemplate() and `templates` return EmbeddedViewRef. Nothing is rewritten — a subclass has to be rewired by hand. Also notes the changes no call site can point at: the data passed to show() is no longer written to (the `style` and `icon` defaults are resolved inside the toast, so anything rendering the same object needs its own), hover and focus pause the shared countdown while a showTemplate() toast is not paused, every toast is a role=\"alert\"/\"status\" live region inside a labelled role=\"region\" stack named by the new `toastRegion` a11y locale key, and focus is handed on only for a keyboard dismissal.", "factory": "./migrations/toast-stack-and-defaults/index" + }, + "timepicker-signals": { + "version": "20.3.0-0", + "description": "Migrates KbqTimepicker consumers to its signal-based inputs. KbqTimepicker implements KbqFormFieldControl, which declares value, id, placeholder, required, disabled, focused, empty and errorState as plain members, so those stay plain accessors; the four inputs the timepicker owns are signals now. Rewrites programmatic reads of format to calls on receivers typed KbqTimepicker. Warns - without auto-fixing - on min and max, whose getters used to hand back what the date adapter had parsed so an unparseable bound value read as null, on writes to the read-only inputs including kbqValidationTooltip, and on view/content queries that return the instance. Reports that the validation tooltip finally unsubscribes, that a locale change reformats the rendered time even with a consumer-provided placeholder, and that generated ids come from the CDK _IdGenerator.", + "factory": "./migrations/timepicker-signals/index" } } } diff --git a/packages/schematics/src/migrations/timepicker-signals/README.md b/packages/schematics/src/migrations/timepicker-signals/README.md new file mode 100644 index 0000000000..70d5c0d2fe --- /dev/null +++ b/packages/schematics/src/migrations/timepicker-signals/README.md @@ -0,0 +1,61 @@ +# timepicker-signals + +Migration schematic invoked automatically by `ng update @koobiq/components@20` (registered for +`20.3.0-0`). Migrates `KbqTimepicker` consumers to its signal-based inputs. + +## Background + +`KbqTimepicker` implements `KbqFormFieldControl`, which declares `value`, `id`, `placeholder`, +`required`, `disabled`, `focused`, `empty` and `errorState` as plain members. Those stay plain +accessors. The four inputs the timepicker owns moved. + +`min` and `max` parsed in their setters and reported the parsed result: + +```ts +set min(value: D | null) { + this._min = this.getValidDateOrNull(this.dateAdapter.deserialize(value)); + this.validatorOnChange(); +} +``` + +So an unparseable bound value read back as `null`. They report what was bound now; the parsed values +stay internal and still drive the validators, re-running them from an effect. + +`kbqValidationTooltip` was a setter-only input that subscribed to `incorrectInput` and never +unsubscribed — re-binding it stacked another subscription, and the last one outlived the directive. + +## What it rewrites + +| Before | After | +| ------------------- | --------------------- | +| `timepicker.format` | `timepicker.format()` | + +On receivers explicitly typed `KbqTimepicker`. Already-migrated reads are left alone, so the +schematic is idempotent. There is no template pass: `kbqTimepicker` is an attribute on a native +``. + +## What it does _not_ do + +| Pattern | Manual migration | +| --------------------------------------- | ------------------------------------------------------------------------ | +| `.min` / `.max` | `min()` / `max()`, and expect the bound value rather than the parsed one | +| `.format = …` / `.min = …` / `.max = …` | Bind them; the inputs are read-only | +| `.kbqValidationTooltip = …` | Bind `[kbqValidationTooltip]` | +| `viewChild(KbqTimepicker)` | The query returns the instance, so a read is a double call | + +## Notes with no call site to point at + +- **`kbqValidationTooltip` unsubscribes.** The setter subscribed to `incorrectInput` every time it + ran and never unsubscribed. It is an effect with a teardown now. +- **A locale change reformats the rendered time even when the placeholder was set by the consumer.** + The effect used to return early on a consumer-provided placeholder, which skipped the reformat with + it; the two are separate concerns now. +- **Generated ids changed shape**, from `kbq-timepicker-1` to `kbq-timepicker-a1`. + +## Running it manually + +``` +ng generate @koobiq/components:timepicker-signals --project my-app +``` + +Pass `--fix=false` to see what it would change without writing. diff --git a/packages/schematics/src/migrations/timepicker-signals/data.ts b/packages/schematics/src/migrations/timepicker-signals/data.ts new file mode 100644 index 0000000000..be33f55cd7 --- /dev/null +++ b/packages/schematics/src/migrations/timepicker-signals/data.ts @@ -0,0 +1,73 @@ +/** + * Data for the `timepicker-signals` migration. + * + * `KbqTimepicker` implements `KbqFormFieldControl`, which declares `value`, `id`, `placeholder`, + * `required`, `disabled`, `focused`, `empty` and `errorState` as plain members — so those stay plain + * accessors. The four inputs the timepicker owns are signals now. + * + * - `timepicker.format` → `timepicker.format()` (value unchanged — auto-fixed) + * - `timepicker.min` / `max` → signals whose value changed: the getters returned the parsed date (warn) + * - `kbqValidationTooltip` → a plain input driven by an effect that finally unsubscribes (warn on writes) + */ + +/** Members whose value is unchanged; a read must become a call. Auto-fixed. */ +export const SIGNAL_MEMBERS: readonly string[] = ['format']; + +/** + * Signal members that are writable via `.set(...)`. Every migrated member is an `input()`, so this is + * empty — a programmatic write is left untouched and becomes a compile error. + */ +export const WRITABLE_MEMBERS: ReadonlySet = new Set(); + +/** TypeScript type annotation that marks a receiver as a timepicker. */ +export const TIMEPICKER_TYPE = 'KbqTimepicker'; + +/** Import specifier that marks a file as a timepicker consumer. */ +export const TIMEPICKER_PACKAGE = '@koobiq/components/timepicker'; + +/** + * `min` and `max` became read-only `InputSignal`s AND changed their value: the getters returned + * `getValidDateOrNull(dateAdapter.deserialize(bound))`, so an unparseable bound value read back as `null`. + * They report what was bound now; the parsed values stay internal and still drive the validators. + */ +export const VALUE_CHANGED_MEMBERS: readonly string[] = ['min', 'max']; + +export interface WarnPattern { + /** Owner of the member. The pattern is only evaluated for files that also name it. */ + anchor: string; + pattern: string; + message: string; +} + +const TIMEPICKER_ANCHOR = '\\bKbqTimepicker\\b'; + +export const warnPatterns: WarnPattern[] = [ + { + anchor: TIMEPICKER_ANCHOR, + pattern: '\\.\\s*(?:format|min|max|kbqValidationTooltip)\\s*=[^=]', + message: + 'KbqTimepicker.format, min, max and kbqValidationTooltip are read-only signal inputs now, so a ' + + 'programmatic write no longer compiles. Bind them in the template instead.' + }, + { + anchor: TIMEPICKER_ANCHOR, + pattern: '(?:viewChild|ViewChild|contentChild|ContentChild)[^\\n;]*\\bKbqTimepicker\\b', + message: + 'A KbqTimepicker view/content query returns the directive instance, whose `format`, `min` and ' + + '`max` are now signals — reading one is a double call, e.g. `this.timepicker().format()`. ' + + 'Verify query reads manually.' + } +]; + +/** Printed once per project, after the per-file reports. */ +export const SUMMARY = [ + ' `kbqValidationTooltip` unsubscribes. The setter subscribed to `incorrectInput` every time it ran ' + + 'and never unsubscribed, so re-binding the input stacked another subscription and the last one ' + + 'outlived the directive. It is an effect with a teardown now.', + ' A locale change reformats the rendered time even when the placeholder was set by the consumer. The ' + + 'effect used to return early on a consumer-provided placeholder, which skipped the reformat with ' + + 'it — the two are separate concerns now.', + ' Generated ids come from the CDK `_IdGenerator`, so their shape changed from `kbq-timepicker-1` to ' + + '`kbq-timepicker-a1` — the app id is part of the prefix, which keeps two Angular apps on one page ' + + 'from colliding.' +]; diff --git a/packages/schematics/src/migrations/timepicker-signals/index.spec.ts b/packages/schematics/src/migrations/timepicker-signals/index.spec.ts new file mode 100644 index 0000000000..85346dfa69 --- /dev/null +++ b/packages/schematics/src/migrations/timepicker-signals/index.spec.ts @@ -0,0 +1,212 @@ +import { workspaces } from '@angular-devkit/core'; +import { Tree } from '@angular-devkit/schematics'; +import { SchematicTestRunner } from '@angular-devkit/schematics/testing'; +import { getWorkspace } from '@schematics/angular/utility/workspace'; +import * as path from 'path'; +import { createTestApp } from '../../utils/testing'; +import { Schema } from './schema'; + +const collectionPath = path.join(__dirname, '../../collection.json'); +const SCHEMATIC_NAME = 'timepicker-signals'; + +describe(SCHEMATIC_NAME, () => { + let runner: SchematicTestRunner; + let appTree: Tree; + let projects: workspaces.ProjectDefinitionCollection; + let messages: string[]; + + beforeEach(async () => { + runner = new SchematicTestRunner('schematics', collectionPath); + appTree = await createTestApp(runner, { style: 'scss' }); + const workspace = await getWorkspace(appTree); + + projects = workspace.projects as unknown as workspaces.ProjectDefinitionCollection; + + messages = []; + runner.logger.subscribe((entry) => messages.push(entry.message)); + }); + + function paths(project: workspaces.ProjectDefinition) { + const root = `/${project.root}/src/app`; + const ts = appTree.exists(`${root}/app.ts`) ? `${root}/app.ts` : `${root}/app.component.ts`; + const html = appTree.exists(`${root}/app.html`) ? `${root}/app.html` : `${root}/app.component.html`; + + return { ts, html }; + } + + async function run(fix: boolean = true): Promise { + const [first] = projects.keys(); + + return runner.runSchematic(SCHEMATIC_NAME, { project: first, fix } satisfies Schema, appTree); + } + + function firstTsPath(): string { + const [first] = projects.keys(); + + return paths(projects.get(first)!).ts; + } + + it('rewrites format reads on a parameter typed KbqTimepicker (incl. optional chain) to calls', async () => { + const ts = firstTsPath(); + + appTree.overwrite( + ts, + "import { KbqTimepicker } from '@koobiq/components/timepicker';\n" + + 'class Demo {\n' + + ' read(timepicker: KbqTimepicker) {\n' + + ' return timepicker.format ?? timepicker?.format;\n' + + ' }\n' + + '}\n' + ); + + const updated = (await run()).readText(ts); + + expect(updated).toContain('timepicker.format() ?? timepicker?.format()'); + }); + + it('rewrites reads on a @ViewChild field (this.timepicker)', async () => { + const ts = firstTsPath(); + + appTree.overwrite( + ts, + "import { ViewChild } from '@angular/core';\n" + + "import { KbqTimepicker } from '@koobiq/components/timepicker';\n" + + 'class Demo {\n' + + ' @ViewChild(KbqTimepicker) timepicker: KbqTimepicker;\n' + + ' read() {\n' + + ' return this.timepicker.format;\n' + + ' }\n' + + '}\n' + ); + + expect((await run()).readText(ts)).toContain('return this.timepicker.format();'); + }); + + it('leaves reads on a receiver of an unrelated type alone', async () => { + const ts = firstTsPath(); + const source = + "import { KbqTimepicker } from '@koobiq/components/timepicker';\n" + + 'class Other {\n' + + " format = 'HH:mm';\n" + + '}\n' + + 'class Demo {\n' + + ' read(other: Other) {\n' + + ' return other.format;\n' + + ' }\n' + + '}\n'; + + appTree.overwrite(ts, source); + + expect((await run()).readText(ts)).toBe(source); + }); + + it('is idempotent — an already migrated read is left alone', async () => { + const ts = firstTsPath(); + const source = + "import { KbqTimepicker } from '@koobiq/components/timepicker';\n" + + 'class Demo {\n' + + ' read(timepicker: KbqTimepicker) {\n' + + ' return timepicker.format();\n' + + ' }\n' + + '}\n'; + + appTree.overwrite(ts, source); + + expect((await run()).readText(ts)).toBe(source); + }); + + it('leaves a programmatic write alone — the input is read-only', async () => { + const ts = firstTsPath(); + + appTree.overwrite( + ts, + "import { KbqTimepicker } from '@koobiq/components/timepicker';\n" + + 'class Demo {\n' + + ' write(timepicker: KbqTimepicker) {\n' + + " timepicker.format = 'HH:mm';\n" + + ' }\n' + + '}\n' + ); + + expect((await run()).readText(ts)).toContain("timepicker.format = 'HH:mm';"); + }); + + it('warns about min and max instead of rewriting them', async () => { + const ts = firstTsPath(); + const source = + "import { KbqTimepicker } from '@koobiq/components/timepicker';\n" + + 'class Demo {\n' + + ' read(timepicker: KbqTimepicker) {\n' + + ' return timepicker.min ?? timepicker.max;\n' + + ' }\n' + + '}\n'; + + appTree.overwrite(ts, source); + + expect((await run()).readText(ts)).toContain('return timepicker.min ?? timepicker.max;'); + expect(messages.join('\n')).toContain('date adapter had parsed'); + }); + + it('warns about a write to the validation tooltip input', async () => { + const ts = firstTsPath(); + + appTree.overwrite( + ts, + "import { KbqTimepicker } from '@koobiq/components/timepicker';\n" + + 'class Demo {\n' + + ' wire(timepicker: KbqTimepicker, tooltip: any) {\n' + + ' timepicker.kbqValidationTooltip = tooltip;\n' + + ' }\n' + + '}\n' + ); + + await run(); + + expect(messages.join('\n')).toContain('read-only signal inputs'); + }); + + it('reports the teardown, the locale reformat and the id shape once per project', async () => { + const ts = firstTsPath(); + + appTree.overwrite( + ts, + "import { KbqTimepicker } from '@koobiq/components/timepicker';\n" + + 'class Demo {\n' + + ' read(timepicker: KbqTimepicker) {\n' + + ' return timepicker.format;\n' + + ' }\n' + + '}\n' + ); + + await run(); + + const summary = messages.join('\n'); + + expect(summary).toContain('unsubscribes'); + expect(summary).toContain('locale change'); + expect(summary).toContain('_IdGenerator'); + expect(summary.match(/_IdGenerator/g)!.length).toBe(1); + }); + + it('stays silent for a workspace that does not use the timepicker', async () => { + await run(); + + expect(messages.join('\n')).not.toContain(`[${SCHEMATIC_NAME}]`); + }); + + it('does not write when fix is false', async () => { + const ts = firstTsPath(); + const source = + "import { KbqTimepicker } from '@koobiq/components/timepicker';\n" + + 'class Demo {\n' + + ' read(timepicker: KbqTimepicker) {\n' + + ' return timepicker.format;\n' + + ' }\n' + + '}\n'; + + appTree.overwrite(ts, source); + + expect((await run(false)).readText(ts)).toBe(source); + expect(messages.join('\n')).toContain('would update'); + }); +}); diff --git a/packages/schematics/src/migrations/timepicker-signals/index.ts b/packages/schematics/src/migrations/timepicker-signals/index.ts new file mode 100644 index 0000000000..682784bc26 --- /dev/null +++ b/packages/schematics/src/migrations/timepicker-signals/index.ts @@ -0,0 +1,305 @@ +import { Path } from '@angular-devkit/core'; +import { Rule, SchematicContext, Tree } from '@angular-devkit/schematics'; +import ts from 'typescript'; +import { logMessage } from '../../utils/messages'; +import { setupOptions } from '../../utils/package-config'; +import { + SIGNAL_MEMBERS, + SUMMARY, + TIMEPICKER_PACKAGE, + TIMEPICKER_TYPE, + VALUE_CHANGED_MEMBERS, + warnPatterns, + WRITABLE_MEMBERS +} from './data'; +import { Schema } from './schema'; + +const LABEL = '[timepicker-signals]'; +const TS_EXT = '.ts'; + +/** A text-span edit on the original file content. Applied right-to-left so offsets stay valid. */ +interface Edit { + start: number; + end: number; + text: string; +} + +/** A receiver whose static type is a timepicker, valid within `[start, end]` of the source. */ +interface Receiver { + /** Source text of the receiver expression, e.g. `timepicker` or `this.timepicker`. */ + text: string; + start: number; + end: number; +} + +/** Applies text-span edits to `content`, right-to-left, so earlier edits don't shift later offsets. */ +function applyEdits(content: string, edits: Edit[]): string { + const sorted = [...edits].sort((a, b) => b.start - a.start || b.end - a.end); + let result = content; + + for (const { start, end, text } of sorted) { + result = result.slice(0, start) + text + result.slice(end); + } + + return result; +} + +const isFunctionLike = (node: ts.Node): boolean => + ts.isFunctionDeclaration(node) || + ts.isMethodDeclaration(node) || + ts.isConstructorDeclaration(node) || + ts.isArrowFunction(node) || + ts.isFunctionExpression(node) || + ts.isGetAccessorDeclaration(node) || + ts.isSetAccessorDeclaration(node); + +/** Walks up from `node` to the nearest ancestor matching `predicate`. */ +function findAncestor(node: ts.Node, predicate: (node: ts.Node) => boolean): ts.Node | undefined { + let current = node.parent; + + while (current) { + if (predicate(current)) return current; + current = current.parent; + } + + return undefined; +} + +/** Whether a type annotation refers to `typeName`. */ +function isTypeReference(type: ts.TypeNode | undefined, typeName: string): boolean { + return !!type && ts.isTypeReferenceNode(type) && ts.isIdentifier(type.typeName) && type.typeName.text === typeName; +} + +const FIELD_MODIFIERS = new Set([ + ts.SyntaxKind.PrivateKeyword, + ts.SyntaxKind.PublicKeyword, + ts.SyntaxKind.ProtectedKeyword, + ts.SyntaxKind.ReadonlyKeyword +]); + +/** + * Collects the receivers annotated with `typeName`, by explicit annotation only (no cross-package type + * resolution): method/function params, class fields (incl. `@ViewChild(KbqTimepicker) x: KbqTimepicker` and constructor + * parameter-properties) and typed locals. + */ +function collectReceivers(sourceFile: ts.SourceFile, typeName: string): Receiver[] { + const receivers: Receiver[] = []; + const add = (text: string, scope: ts.Node) => + receivers.push({ text, start: scope.getStart(sourceFile), end: scope.getEnd() }); + + const visit = (node: ts.Node): void => { + if (ts.isParameter(node) && ts.isIdentifier(node.name) && isTypeReference(node.type, typeName)) { + add(node.name.text, findAncestor(node, isFunctionLike) ?? sourceFile); + + // A constructor parameter-property is also a class field, reachable as `this.`. + if (node.modifiers?.some((modifier) => FIELD_MODIFIERS.has(modifier.kind))) { + const owner = findAncestor(node, ts.isClassDeclaration); + + if (owner) add(`this.${node.name.text}`, owner); + } + } else if ( + ts.isPropertyDeclaration(node) && + ts.isIdentifier(node.name) && + isTypeReference(node.type, typeName) + ) { + const owner = findAncestor(node, ts.isClassDeclaration); + + if (owner) add(`this.${node.name.text}`, owner); + } else if ( + ts.isVariableDeclaration(node) && + ts.isIdentifier(node.name) && + isTypeReference(node.type, typeName) + ) { + add(node.name.text, findAncestor(node, isFunctionLike) ?? sourceFile); + } + + node.forEachChild(visit); + }; + + visit(sourceFile); + + return receivers; +} + +/** Whether a property access on a receiver is within one of the receiver's scopes. */ +function inReceiverScope(node: ts.PropertyAccessExpression, sourceFile: ts.SourceFile, receivers: Receiver[]): boolean { + const receiverText = node.expression.getText(sourceFile); + const start = node.getStart(sourceFile); + const end = node.getEnd(); + + return receivers.some((r) => r.text === receiverText && start >= r.start && end <= r.end); +} + +/** Classifies a matched property access and appends the resulting edit(s). */ +function classifyAccess(node: ts.PropertyAccessExpression, sourceFile: ts.SourceFile, edits: Edit[]): void { + const parent = node.parent; + + // Already migrated: a call, or a `.set(...)` write — leave alone (idempotent). + if (ts.isCallExpression(parent) && parent.expression === node) return; + if (ts.isPropertyAccessExpression(parent) && parent.expression === node && parent.name.text === 'set') return; + + // Write target: `x.member = RHS`. Every KbqTimepicker signal member is `input()` (read-only), so there is no + // writable member — leave the write untouched (it becomes a compile error the consumer fixes by hand). + if ( + ts.isBinaryExpression(parent) && + parent.left === node && + parent.operatorToken.kind === ts.SyntaxKind.EqualsToken + ) { + if (WRITABLE_MEMBERS.has(node.name.text)) { + const rhs = parent.right; + + edits.push({ start: node.getEnd(), end: rhs.getStart(sourceFile), text: '.set(' }); + edits.push({ start: rhs.getEnd(), end: rhs.getEnd(), text: ')' }); + } + + return; + } + + // Read (incl. optional chain `x?.compact`): append `()`. + edits.push({ start: node.getEnd(), end: node.getEnd(), text: '()' }); +} + +/** Collects edits for every read/write of a value-safe signal member on a known timepicker receiver. */ +function collectAccessEdits(sourceFile: ts.SourceFile, receivers: Receiver[]): Edit[] { + const edits: Edit[] = []; + + const visit = (node: ts.Node): void => { + if ( + ts.isPropertyAccessExpression(node) && + ts.isIdentifier(node.name) && + SIGNAL_MEMBERS.includes(node.name.text) && + inReceiverScope(node, sourceFile, receivers) + ) { + classifyAccess(node, sourceFile, edits); + } + + node.forEachChild(visit); + }; + + visit(sourceFile); + + return edits; +} + +/** Collects the members read on a timepicker receiver whose value changed. */ +function collectValueChangedAccess(sourceFile: ts.SourceFile, receivers: Receiver[]): Set { + const valueChanged = new Set(); + + const visit = (node: ts.Node): void => { + if ( + ts.isPropertyAccessExpression(node) && + ts.isIdentifier(node.name) && + VALUE_CHANGED_MEMBERS.includes(node.name.text) && + inReceiverScope(node, sourceFile, receivers) + ) { + valueChanged.add(node.name.text); + } + + node.forEachChild(visit); + }; + + visit(sourceFile); + + return valueChanged; +} + +/** Pass A — rewrite value-safe programmatic reads of timepicker signal members in TypeScript code. */ +function migrateTsExpressions(content: string, fileName: string): string { + const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + const receivers = collectReceivers(sourceFile, TIMEPICKER_TYPE); + + if (receivers.length === 0) return content; + + const edits = collectAccessEdits(sourceFile, receivers); + + return edits.length > 0 ? applyEdits(content, edits) : content; +} + +/** Emits precise, receiver-scoped warnings for the members that can't be auto-fixed. */ +function warnReceiverMembers(context: SchematicContext, filePath: string, content: string): void { + const sourceFile = ts.createSourceFile(filePath, content, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + const receivers = collectReceivers(sourceFile, TIMEPICKER_TYPE); + + if (receivers.length === 0) return; + + const valueChanged = collectValueChangedAccess(sourceFile, receivers); + + if (valueChanged.size > 0) { + logMessage(context.logger, [ + `${LABEL} ${filePath}`, + ` \`${[...valueChanged].join('` and `')}\` are read-only InputSignals now, and their value`, + ` changed: the getters handed back what the date adapter had parsed, so an unparseable bound`, + ` value read as null. They report what was bound. Migrate by hand.` + ]); + } +} + +function logWarnings(context: SchematicContext, filePath: string, content: string): void { + for (const { anchor, pattern, message } of warnPatterns) { + if (!new RegExp(anchor).test(content) || !new RegExp(pattern).test(content)) continue; + + logMessage(context.logger, [`${LABEL} ${filePath}`, ` ${message}`]); + } +} + +/** + * A `.ts` file is a timepicker consumer if it names any of the exported symbols or imports the package. + * There is no element to look for: `kbqTimepicker` is an attribute on a native ``. + */ +function referencesTimepicker(content: string): boolean { + return /\bKbqTimepicker\w*\b/.test(content) || content.includes(TIMEPICKER_PACKAGE); +} + +export default function timepickerSignals(options: Schema): Rule { + return async (tree: Tree, context: SchematicContext) => { + const { project, fix } = options; + const projectDefinition = await setupOptions(project, tree); + const root = projectDefinition?.root ?? ''; + const rootDir = root ? tree.getDir(root as Path) : tree.root; + + const tsPaths: string[] = []; + + rootDir.visit((filePath) => { + if (filePath.includes('node_modules') || filePath.includes('/dist/')) return; + + if (filePath.endsWith(TS_EXT)) tsPaths.push(filePath); + }); + + let touched = 0; + let consumers = 0; + + const commit = (filePath: string, original: string, updated: string) => { + if (updated === original) return; + + touched++; + + if (fix) { + tree.overwrite(filePath, updated); + } else { + logMessage(context.logger, [`${LABEL} would update ${filePath} (run with --fix to apply)`]); + } + }; + + for (const filePath of tsPaths) { + const original = tree.read(filePath)?.toString(); + + if (!original || !referencesTimepicker(original)) continue; + + consumers++; + + logWarnings(context, filePath, original); + warnReceiverMembers(context, filePath, original); + + commit(filePath, original, migrateTsExpressions(original, filePath)); + } + + // Nothing here uses the timepicker, so the summary would only be noise. + if (consumers === 0) return; + + logMessage(context.logger, [ + `${LABEL} processed tree under "${root || ''}", ` + + `${fix ? 'updated' : 'would update'} ${touched} file(s).`, + ...SUMMARY + ]); + }; +} diff --git a/packages/schematics/src/migrations/timepicker-signals/schema.json b/packages/schematics/src/migrations/timepicker-signals/schema.json new file mode 100644 index 0000000000..f10928fce1 --- /dev/null +++ b/packages/schematics/src/migrations/timepicker-signals/schema.json @@ -0,0 +1,20 @@ +{ + "$schema": "http://json-schema.org/schema", + "$id": "koobiq-components-timepicker-signals", + "title": "Koobiq components timepicker signals migration", + "type": "object", + "properties": { + "project": { + "type": "string", + "description": "Name of the project to migrate. If omitted, the migration runs over the whole tree.", + "$default": { + "$source": "projectName" + } + }, + "fix": { + "type": "boolean", + "default": true, + "description": "When true, applies all auto-fix replacements. When false, prints what would change without writing." + } + } +} diff --git a/packages/schematics/src/migrations/timepicker-signals/schema.ts b/packages/schematics/src/migrations/timepicker-signals/schema.ts new file mode 100644 index 0000000000..b9f56acac5 --- /dev/null +++ b/packages/schematics/src/migrations/timepicker-signals/schema.ts @@ -0,0 +1,6 @@ +export interface Schema { + /** Name of the project to migrate. */ + project?: string; + /** When true, applies replacements; when false, only logs what would change. */ + fix: boolean; +} diff --git a/tools/public_api_guard/components/timepicker.api.md b/tools/public_api_guard/components/timepicker.api.md index 856224cfd6..48e4e572d3 100644 --- a/tools/public_api_guard/components/timepicker.api.md +++ b/tools/public_api_guard/components/timepicker.api.md @@ -6,10 +6,10 @@ import { AbstractControl } from '@angular/forms'; import { AfterContentInit } from '@angular/core'; +import * as _angular_core from '@angular/core'; import { ControlValueAccessor } from '@angular/forms'; import { DoCheck } from '@angular/core'; import { ErrorStateMatcher } from '@koobiq/components/core'; -import * as i0 from '@angular/core'; import * as i1 from '@angular/cdk/a11y'; import * as i2 from '@angular/cdk/platform'; import * as i3 from '@angular/forms'; @@ -72,9 +72,7 @@ export class KbqTimepicker implements KbqFormFieldControl, ControlValueAcc // (undocumented) focusChanged(isFocused: boolean): void; focused: boolean; - // (undocumented) - get format(): TimeFormats; - set format(formatValue: TimeFormats); + readonly format: _angular_core.InputSignalWithTransform; // (undocumented) getSize(): number; // (undocumented) @@ -83,19 +81,18 @@ export class KbqTimepicker implements KbqFormFieldControl, ControlValueAcc get id(): string; set id(value: string); // (undocumented) - readonly incorrectInput: i0.OutputEmitterRef; + readonly incorrectInput: _angular_core.OutputEmitterRef; // (undocumented) get isFullFormat(): boolean; // (undocumented) get isShortFormat(): boolean; + readonly kbqValidationTooltip: _angular_core.InputSignal; + readonly max: _angular_core.InputSignal; + readonly min: _angular_core.InputSignal; // (undocumented) - set kbqValidationTooltip(tooltip: KbqTooltipTrigger); - // (undocumented) - get max(): D | null; - set max(value: D | null); + static ngAcceptInputType_disabled: unknown; // (undocumented) - get min(): D | null; - set min(value: D | null); + static ngAcceptInputType_required: unknown; // (undocumented) ngAfterContentInit(): void; // (undocumented) @@ -144,9 +141,9 @@ export class KbqTimepicker implements KbqFormFieldControl, ControlValueAcc // (undocumented) writeValue(value: D | null): void; // (undocumented) - static ɵdir: i0.ɵɵDirectiveDeclaration, "input[kbqTimepicker]", ["kbqTimepicker"], { "errorStateMatcher": { "alias": "errorStateMatcher"; "required": false; }; "placeholder": { "alias": "placeholder"; "required": false; }; "disabled": { "alias": "disabled"; "required": false; }; "id": { "alias": "id"; "required": false; }; "required": { "alias": "required"; "required": false; }; "format": { "alias": "format"; "required": false; }; "min": { "alias": "min"; "required": false; }; "max": { "alias": "max"; "required": false; }; "value": { "alias": "value"; "required": false; }; "kbqValidationTooltip": { "alias": "kbqValidationTooltip"; "required": false; }; }, { "incorrectInput": "incorrectInput"; }, never, never, true, never>; + static ɵdir: _angular_core.ɵɵDirectiveDeclaration, "input[kbqTimepicker]", ["kbqTimepicker"], { "errorStateMatcher": { "alias": "errorStateMatcher"; "required": false; }; "placeholder": { "alias": "placeholder"; "required": false; }; "disabled": { "alias": "disabled"; "required": false; }; "id": { "alias": "id"; "required": false; }; "required": { "alias": "required"; "required": false; }; "format": { "alias": "format"; "required": false; "isSignal": true; }; "min": { "alias": "min"; "required": false; "isSignal": true; }; "max": { "alias": "max"; "required": false; "isSignal": true; }; "value": { "alias": "value"; "required": false; }; "kbqValidationTooltip": { "alias": "kbqValidationTooltip"; "required": false; "isSignal": true; }; }, { "incorrectInput": "incorrectInput"; }, never, never, true, never>; // (undocumented) - static ɵfac: i0.ɵɵFactoryDeclaration, never>; + static ɵfac: _angular_core.ɵɵFactoryDeclaration, never>; } // @public @@ -155,11 +152,11 @@ export const kbqTimepickerLocaleConfigurationProvider: (configuration: KbqDeepPa // @public (undocumented) export class KbqTimepickerModule { // (undocumented) - static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵfac: _angular_core.ɵɵFactoryDeclaration; // (undocumented) - static ɵinj: i0.ɵɵInjectorDeclaration; + static ɵinj: _angular_core.ɵɵInjectorDeclaration; // (undocumented) - static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵmod: _angular_core.ɵɵNgModuleDeclaration; } // @public (undocumented) From 8ba92c0009257daca8889109a4ec59e8979da936 Mon Sep 17 00:00:00 2001 From: Artem Belik Date: Fri, 4 Sep 2026 10:30:05 +0300 Subject: [PATCH 2/2] fix(timepicker): address the review of the component review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The validation tooltip teardown had no test, so nothing proved the leak was fixed. It does now — the test fails against a no-op cleanup and passes against the real one. Injections that are never reassigned are `readonly`. Co-Authored-By: Claude Opus 5 --- .../timepicker/timepicker.directive.ts | 4 +- .../components/timepicker/timepicker.spec.ts | 44 ++++++++++++++++++- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/packages/components/timepicker/timepicker.directive.ts b/packages/components/timepicker/timepicker.directive.ts index 73708cffce..7267c6a56e 100644 --- a/packages/components/timepicker/timepicker.directive.ts +++ b/packages/components/timepicker/timepicker.directive.ts @@ -154,7 +154,7 @@ export class KbqTimepicker private readonly uid = inject(_IdGenerator).getId('kbq-timepicker-'); private elementRef = inject>(ElementRef); - private renderer = inject(Renderer2); + private readonly renderer = inject(Renderer2); private dateAdapter = inject>(DateAdapter, { optional: true })!; private readonly timezoneService = inject(KbqDateTimezoneService); private readonly configuration = kbqInjectLocaleConfiguration('timepicker', KBQ_TIMEPICKER_CONFIGURATION); @@ -371,7 +371,7 @@ export class KbqTimepicker private onChange: (value: any) => void; private onTouched: () => void; - private errorStateTracker: KbqErrorStateTracker; + private readonly errorStateTracker: KbqErrorStateTracker; constructor() { if (!this.dateAdapter) { diff --git a/packages/components/timepicker/timepicker.spec.ts b/packages/components/timepicker/timepicker.spec.ts index 5d859a552e..1dd0436368 100644 --- a/packages/components/timepicker/timepicker.spec.ts +++ b/packages/components/timepicker/timepicker.spec.ts @@ -1,4 +1,4 @@ -import { Component, DebugElement, Inject, Type, inject, viewChild } from '@angular/core'; +import { Component, DebugElement, Inject, Type, inject, viewChild, viewChildren } from '@angular/core'; import { ComponentFixture, TestBed, fakeAsync, tick } from '@angular/core/testing'; import { AsyncValidatorFn, @@ -33,6 +33,7 @@ import { } from '@koobiq/components/core'; import { KbqFormFieldModule } from '@koobiq/components/form-field'; import { KbqIconModule } from '@koobiq/components/icon'; +import { KbqToolTipModule, KbqTooltipTrigger } from '@koobiq/components/tooltip'; import { DateTime } from 'luxon'; import { Observable, map, timer } from 'rxjs'; import { @@ -1230,6 +1231,28 @@ describe(KbqTimepicker.name, () => { expect(timepicker.min()).toBe('not a time'); }); + it('should stop driving a validation tooltip once it is unbound', () => { + const fixture = createStandaloneComponent(TimepickerWithValidationTooltip); + const { componentInstance } = fixture; + const [first, second] = componentInstance.tooltips(); + const firstShow = jest.spyOn(first, 'show'); + const secondShow = jest.spyOn(second, 'show'); + + componentInstance.timepicker().incorrectInput.emit(); + + expect(firstShow).toHaveBeenCalledTimes(1); + expect(secondShow).not.toHaveBeenCalled(); + + componentInstance.useSecondTooltip = true; + fixture.detectChanges(); + + componentInstance.timepicker().incorrectInput.emit(); + + // The old setter never unsubscribed, so the first tooltip kept receiving every rejection. + expect(firstShow).toHaveBeenCalledTimes(1); + expect(secondShow).toHaveBeenCalledTimes(1); + }); + it('should re-run the validators when min changes', () => { const fixture = createStandaloneComponent(TimepickerSignalInputs); const { control } = fixture.componentInstance; @@ -1263,3 +1286,22 @@ class TimepickerSignalInputs { min: DateTime | null = null; max: DateTime | null = null; } + +@Component({ + imports: [KbqLuxonDateModule, KbqFormFieldModule, KbqTimepickerModule, KbqToolTipModule, FormsModule], + template: ` + + + + + + + ` +}) +class TimepickerWithValidationTooltip { + readonly timepicker = viewChild.required(KbqTimepicker); + readonly tooltips = viewChildren(KbqTooltipTrigger); + + value: DateTime | null = null; + useSecondTooltip = false; +}