Land remaining unsafe/legacy TypeScript type cleanup from #586 - #713
Merged
Conversation
Runs `ng lint --fix` (via direct ESLint invocation, with @angular-eslint/prefer-standalone temporarily disabled to avoid a behavior-changing false-positive fix — see rationale in PR) across both workspaces and locks the improvement in via lint:baseline:bump. - root warnings: 1619 -> 1307 - test-app warnings: 4 -> 3 - 0 lint errors in either workspace - full test-app suite remains green (1380/1380) - coverage and publish-validation gates unaffected Closes #582
Copilot review caught three real defects introduced by the safe-fix pass: - data-table.component.ts: implements OnChanges<T> is invalid (OnChanges is not generic); restore the generic argument on _SamTable<T> instead and implement plain OnChanges. - listbox.component.ts: setHighlightedItem read item["lable"] (typo, pre-existing bug the autofix format-touched but didn't create) — fixed to item["label"] to match OptionModel. - title-section.component.ts: removing the @input("id") alias exposed that attrId was only ever initialized once from the field initializer, so it never tracked later updates to id; turned attrId into a getter so the attr.id host binding stays in sync with the id input. All three verified via tsc --noEmit; full test-app suite (1380 tests) still green.
…slint-autofixes-and-lower-the-baseline # Conflicts: # eslint-baseline.json
…slint-autofixes-and-lower-the-baseline # Conflicts: # eslint-baseline.json
…and misc small areas (#702) * Replace unsafe/legacy types in ui-kit/utilities (#586) - are-equal.ts: any -> unknown, precise internal helper types - deprecator.ts: any -> object for parentClass/render, typed record cast for dynamic property access - key-helper.ts: introduce KeyEventLike to model the cross-browser event shapes this helper reads (including legacy keyIdentifier/numeric key fixtures), replacing all any/Function usages; preserves the original tolerant behavior for malformed event input via toKeyEventLike Lowers the root ESLint warning baseline 1307 -> 1289. * Replace unsafe/legacy types in ui-kit/directives (#586) - taboutside.directive.ts: EventEmitter<any> -> EventEmitter<void>, typed hasFocusChanged(target: EventTarget) - sticky.component.ts: typed resize/scroll(event: Event), replaced any casts on getElementsByClassName results with HTMLCollectionOf<HTMLElement>, introduced exported OffsetParentLike interface for getElemDistanceToTop's recursive offsetParent walk - click-outside/focus/tab-outside/drag-drop/sticky specs: replaced EventEmitter<any>/fixture: any with concrete EventEmitter<boolean>/ComponentFixture<TestComponent> types, and Event() instances instead of {}/undefined for resize/scroll calls Lowers the root ESLint warning baseline 1289 -> 1274. * Replace unsafe/legacy types in ui-kit/layout (#586) - dynamic-chips.directive.ts: typed `map` input, `remove` emitter, and internal filter/field mapping helpers (FilterField, FilterDrawerChipModel interfaces) instead of any/untyped params - filter-drawer-chip.component.ts / filter-drawer-item.component.ts / filter-drawer.component.ts: EventEmitter<any> -> EventEmitter<unknown | Record<string, unknown> | void>, `values: any[]` -> `unknown[]` - filters-wrapper.component.ts: Subject<any> -> Subject<unknown> for runReportEvent/resetReportEvent - pagination.component.ts: `options: any[]` -> concrete option shape matching the template's usage (value/label/disabled/selected) - toolbar.component.ts: EventEmitter<any> -> EventEmitter<ToolbarItem> matching the emitted contentModel item type - aside-toggle.spec.ts / toolbar.spec.ts: `fixture: any` -> ComponentFixture<...> Lowers the root ESLint warning baseline 1274 -> 1259. * Replace unsafe/legacy types in small misc areas (#586) Covers the remaining small pockets of unsafe/legacy types outside the larger per-area slices already addressed: formly/sam-formly.ts, type-check-helpers.ts, wrappers, elements, types.ts, pipes, formly/components, filters, typings.d.ts. - sam-formly.ts: AbstractSamFormly is now generic over a SamFormlyTemplateComponent-constrained template type instead of `any`; setProperties/ngOnInit typed against that generic and Record<string, unknown> configuration - type-check-helpers.ts: isString/isObject/isArray/safeTypeOf take `unknown` instead of `any` - types.ts: HistoryNodeType.queryParams -> Record<string, string | number | boolean>, AutocompleteConfig.serviceOptions -> unknown, UploadedFileData.icon -> new UploadedFileIcon interface (matches upload-v2 template's `icon.name`/`icon.iconClass` usage) - typings.d.ts: `*.json` module value -> unknown - elements/button: onClick -> EventEmitter<Event>, click($event) -> click($event: Event), btnClassMap -> Record<string, string>; spec updated to pass real Event instances and typed fixture - wrappers/fieldset-wrapper: errorMessages -> string[], hintContainer -> ElementRef<HTMLElement>, ngOnChanges/onResize/toggleHint/ calculateNumberOfLines/setInvalidError given concrete parameter types; specs updated with typed fixture and a complete SimpleChange mock - wrappers/label-wrapper.spec.ts: typed fixture - filters/filters.component.ts: model -> Record<string, unknown> - pipes/date-time-display: transform(datetime) typed as string | number | Date, returns string | undefined - formly/components/autocomplete/test.service.ts: fetch() -> Observable<unknown> Lowers the root ESLint warning baseline 1259 -> 1231. * Address PR review feedback on #702 - Rebase branch tip onto latest #675 (gh-582) so this PR's diff/baseline reflect only #586's type-safety changes, not #675's still-open autofix set. #675 is CLEAN/MERGEABLE and independently reviewed; stacking is the documented convention for this repo's slice-based PRs. - Retarget PR base to gh-582-apply-safe-eslint-autofixes-and-lower-the-baseline so the GitHub diff matches (rebase alone doesn't move a PR's base). - Drop the #674 datepicker outside-click fix and #677 sidenav RxJS fix commits from this branch entirely (dropped during rebase) -- both are unrelated bugfixes already merged to master via their own PRs; they were only present here as inherited ancestry from an earlier base choice, not something this PR should carry or take credit for. - pagination.component.ts: options.value widened to 'string | number' -- the template already supports numeric option values via attribute binding; the prior 'any[]' allowed this and the fix's 'string' literal would have narrowed a supported case. - date-time-display.pipe.ts: transform() parameter widened to include 'undefined' explicitly -- the implementation's own guard branch handles undefined and the pipe previously accepted it; the stricter signature would have been a source-compatibility break for existing callers. - types.ts: HistoryNodeType.queryParams value type widened to include readonly arrays and null, matching Angular Router's actual accepted queryParams value shapes (repeated params, param removal) instead of the narrower 'string | number | boolean' which would reject valid existing usage. * Fix Prettier formatting on date-time-display.pipe.ts * Merge branch 'gh-582-apply-safe-eslint-autofixes-and-lower-the-baseline' into gh-586-replace-unsafe-and-legacy-typescript-types Resolves conflict in eslint-baseline.json by keeping gh-582's lowered baseline (root: 1059, test-app: 0); gh-586's own changes reduce the warning count further under this baseline (983 warnings measured after merge). --------- Co-authored-by: Frank Pigeon Jr. <4629398+fpigeonjr@users.noreply.github.com>
…ng-type-safety-work # Conflicts: # src/ui-kit/layout/filter-drawer/dynamic-chips/dynamic-chips.directive.ts # src/ui-kit/utilities/key-helper/key-helper.ts # src/ui-kit/wrappers/fieldset-wrapper/fieldset-wrapper.component.ts
There was a problem hiding this comment.
🟡 Changes recommended
The updated typings in SamStickyComponent and SamTabOutsideDirective introduce type/contract unsoundness and potential runtime throws that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR rebases and lands the remaining TypeScript type-safety cleanup work from the earlier #586 slice (utilities/directives/layout + a few misc areas) onto current master, along with the corresponding test updates and an updated ESLint warning baseline.
Changes:
- Replace legacy/unsafe
any/Functiontypes with more explicit types (unknown,Record<string, unknown>, specific interfaces) across utilities, directives, layout components, and shared types. - Update unit tests to use typed
ComponentFixture<T>and to pass realEventinstances where outputs/handlers are now typed. - Lower
eslint-baseline.jsonto reflect the reduced warning count after the cleanup.
File summaries
| File | Description |
|---|---|
| src/ui-kit/wrappers/label-wrapper/label-wrapper.spec.ts | Types the TestBed fixture as ComponentFixture<LabelWrapper>. |
| src/ui-kit/wrappers/fieldset-wrapper/fieldset-wrapper.spec.ts | Types the fixture and updates mocked SimpleChange shape. |
| src/ui-kit/wrappers/fieldset-wrapper/fieldset-wrapper.component.ts | Tightens wrapper input/output and DOM types (errorMessages, ViewChild, SimpleChanges, etc.). |
| src/ui-kit/utilities/key-helper/key-helper.ts | Introduces KeyEventLike + unknown inputs to avoid unsafe event typing. |
| src/ui-kit/utilities/deprecator/deprecator.ts | Replaces any with object/Record<string, unknown> access. |
| src/ui-kit/utilities/are-equal/are-equal.ts | Moves equality helpers from any to unknown + safer object/array shapes. |
| src/ui-kit/typings.d.ts | Changes *.json module default export from any to unknown. |
| src/ui-kit/types.ts | Refines public types (HistoryNodeType.queryParams, UploadedFileData.icon, etc.). |
| src/ui-kit/type-check-helpers.ts | Changes helper inputs from any to unknown. |
| src/ui-kit/pipes/date-time-display/date-time-display.pipe.ts | Narrows transform input/return types for date-time formatting. |
| src/ui-kit/layout/toolbar/toolbar.spec.ts | Types the toolbar test fixture. |
| src/ui-kit/layout/toolbar/toolbar.component.ts | Types the action output as ToolbarItem. |
| src/ui-kit/layout/toolbar/aside-toggle.spec.ts | Types the aside-toggle test fixtures. |
| src/ui-kit/layout/pagination/pagination.component.ts | Gives pagination options input a concrete inline type. |
| src/ui-kit/layout/filters-wrapper/filters-wrapper.component.ts | Replaces Subject<any> with Subject<unknown>. |
| src/ui-kit/layout/filter-drawer/filter-drawer.component.ts | Types the clear output as void. |
| src/ui-kit/layout/filter-drawer/filter-drawer-item/filter-drawer-item.component.ts | Types values, remove output, and the emitted payload. |
| src/ui-kit/layout/filter-drawer/filter-drawer-chip/filter-drawer-chip.component.ts | Types the remove output payload as unknown. |
| src/ui-kit/layout/filter-drawer/dynamic-chips/dynamic-chips.directive.ts | Adds typed filter/chip models and refines directive inputs/outputs. |
| src/ui-kit/filters/filters.component.ts | Types model as Record<string, unknown>. |
| src/ui-kit/elements/button/button.spec.ts | Types the fixture and updates tests to pass real click events. |
| src/ui-kit/elements/button/button.component.ts | Types onClick output as Event, btnClassMap as Record<string,string>, and click() arg as Event. |
| src/ui-kit/directives/tab-outside/taboutside.spec.ts | Types the fixture and the test output emitter. |
| src/ui-kit/directives/tab-outside/taboutside.directive.ts | Types the output as void and hasFocusChanged parameter. |
| src/ui-kit/directives/sticky/sticky.spec.ts | Types the fixture and updates tests to pass real Events + typed mock offset-parent chain. |
| src/ui-kit/directives/sticky/sticky.component.ts | Adds OffsetParentLike and strengthens DOM collection/element typing. |
| src/ui-kit/directives/focus/focus.spec.ts | Types the fixture and the test output emitter. |
| src/ui-kit/directives/drag-drop/drag-drop.spec.ts | Types the fixture. |
| src/ui-kit/directives/click-outside/click-outside.spec.ts | Types the fixture and the test output emitter. |
| src/formly/sam-formly.ts | Makes AbstractSamFormly generic over a constrained template component type. |
| src/formly/components/autocomplete/test.service.ts | Changes fetch() return type to Observable<unknown>. |
| eslint-baseline.json | Lowers the root ESLint warning baseline to match the cleaned-up state. |
Review details
Suppressed comments (1)
src/ui-kit/directives/tab-outside/taboutside.directive.ts:26
Element.contains()expects aNode | null. AcceptingEventTargetand passing it through directly can throw if a non-Node target is ever provided (and makes the contract less clear). Narrow/guard the value before callingcontains.
@HostListener("document:keyup", ["$event.target"])
public hasFocusChanged(target: EventTarget): void {
const isInsideHost = this._elementRef.nativeElement.contains(target);
if (!isInsideHost) {
this.tabOutside.emit(undefined);
}
- Files reviewed: 31/32 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- sticky.component.ts: guard isTallestAmongSiblings()/findDirectChild() against a missing/empty container class match instead of indexing parentContainer[0] unconditionally; findDirectChild() now returns HTMLElement | undefined to reflect that it can legitimately find no match, and callers check for that. - fieldset-wrapper.component.ts: type the hintContainer @ViewChild as ElementRef<HTMLElement> | undefined, matching its actual pre-view-init state and the existing if (this.hintContainer) guard, so raw-source consumers compiling with strictPropertyInitialization/strictNullChecks don't hit an unsound non-optional type. - taboutside.directive.ts: accept EventTarget | null (matching the DOM target property's real type) and narrow with instanceof Node before calling Element.contains(), which requires a Node | null argument.
fpigeonjr
marked this pull request as ready for review
September 9, 2026 23:29
pgill-flex
approved these changes
Sep 10, 2026
fpigeonjr
added a commit
that referenced
this pull request
Sep 10, 2026
…d-declarations-and-const-related-lint-debt Resolves conflicts introduced by master's #713 (unsafe/legacy TypeScript type cleanup) landing after this branch's own unused-vars/prefer-const cleanup (#585): - eslint-baseline.json: keep master's lower root baseline (924); this branch's own conflicting files resolve cleanly under it (verified via lint:baseline check below). - test.service.ts: keep master's typed fetch(val: string) signature, add void val; to satisfy this branch's now-error-level @typescript-eslint/no-unused-vars. - click-outside/focus/tab-outside specs: keep master's typed ComponentFixture<TestComponent> import, drop the waitForAsync/fakeAsync/tick imports this branch already removed as unused. - sticky.component.ts: keep this branch's optional resize(event?: Event)/scroll(event?: Event) + void event; signatures (unused-param cleanup) over master's typed-but-unused resize(event: Event): void. - sticky.spec.ts: keep master's ComponentFixture<TestComponent> typing, drop the unused waitForAsync/fakeAsync/tick imports and the never-declared 'component' local this branch already removed. - button.spec.ts: keep master's ComponentFixture typing, drop the unused waitForAsync import. - toolbar.spec.ts: this branch's spec never used component/fixture/ComponentFixture locally (TestBed.createComponent's result is discarded); drop master's newly-added unused component/fixture declarations and the now-unused ComponentFixture import. - fieldset-wrapper.component.ts: keep this branch's parameterless onResize() (the unused UIEvent param removed) over master's typed but unused onResize(event: UIEvent). Also carries forward this branch's already-resolved fieldset-wrapper.spec.ts and label-wrapper.spec.ts non-conflicting hunks, which needed their ComponentFixture import restored after auto-merge silently dropped it alongside this branch's unused-import cleanup on the same line. Verified post-merge: full test-app Vitest suite (1423/1423) passes, root ESLint baseline gate passes (565 warnings vs 924 baseline, 0 errors), test-app lint is clean, and Prettier format:check passes.
This was referenced Sep 10, 2026
fpigeonjr
added a commit
that referenced
this pull request
Sep 11, 2026
* Replace unsafe/legacy types in ui-kit/components Replaces unsafe and legacy TypeScript types (@typescript-eslint/no-explicit-any, no-unsafe-function-type) across src/ui-kit/components, as a slice of #586, following the approach used in #702/#713. - modal.component.ts: typed EventEmitter outputs (unknown[]), types map, scroll helpers, and openModal args. - comments.component.ts/spec.ts: typed form/observable streams against Comment/FormGroup, replaced ad-hoc keyIdentified handling. - sidenav.service.ts, sidenav.component.ts, menu-item.component.ts: widened any/any[] to the existing MenuItem interface (sidenav.service.ts is a frozen consumer-deep-import — no signature changes, only precise typing); added MenuItem#selection. - image.component.ts: typed Observable/EventEmitter streams and drag/drop handlers against DOM event types. - actions/action-interface.ts: callback?: Function -> () => unknown; typed action-button/actions-dropdown EventEmitters against SamActionInterface. - alert.component.ts, alert-footer.service.ts, multiselect-dropdown.component.ts, breadcrumbs.component.ts, accordion.component.ts, header.component.ts: typed EventEmitter outputs and internal fields against existing types (AlertType, OptionsType, IBreadcrumb) instead of any. - Spec files across the directory: replaced any-typed fixtures/locals with ComponentFixture<T> and concrete payload types. Public API and consumer-compiled source compatibility preserved throughout — no breaking signature changes. Verification: - npx eslint src/ui-kit/components: 0 no-explicit-any/no-unsafe-function-type findings remain (108 resolved); 10 unrelated Angular-style warnings (no-output-native/no-output-on-prefix/no-empty-lifecycle-method/ no-input-rename) left untouched, out of scope for this issue. - npx tsc -p tsconfig.json --noEmit: no new errors vs. pre-change baseline. - npm --prefix test-app test: 155 files / 1423 tests pass; coverage gate passes (89.84%/79.68%/86.98%/89.58%, all above floor). - npm run format:check: clean. - eslint-baseline.json root lowered 924 -> 457 via lint:baseline:bump. Closes #704 * Address Copilot PR review feedback for #729 - sidenav/menu-item.component.ts, sidenav/sidenav.component.ts: expose MenuItem (not unknown) on data/selection outputs, matching every real emit site, and align emitChildData()'s parameter type with the child (selection) binding's MenuItem payload so strict Angular template checking no longer sees an Event/MenuItem mismatch. - sidenav/services/sidenav.service.ts: setModel() now returns the assigned MenuItem again (restoring prior runtime behavior), typed precisely instead of reverting to any. - sidenav/sidenav.component.ts: lookupLabelInModel()'s return type widened to number[] | false | undefined to reflect the real fall-through-returns-undefined path instead of silently omitting it. - image/image.component.ts: reverted the error output's payload type to EventEmitter<void> since image.template.html calls error.emit() with no argument; Event was not actually being forwarded. - actions/action-interface.ts: callback? widened to (...args: never[]) => unknown so existing callbacks with parameters remain structurally assignable (source-compatible), rather than the stricter zero-arg () => unknown. - sidenav/sidenav.spec.ts: updated the affected test to emit/assert a MenuItem instead of a bare Event. Verified via npx eslint (0 no-explicit-any/no-unsafe-function-type findings), npx tsc --noEmit (no new errors vs. master), npm --prefix test-app test (155/155 files, 1423/1423 tests), npm run format:check, and both lint:baseline gates. --------- Co-authored-by: Frank Pigeon Jr. <4629398+fpigeonjr@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Lands the remaining unsafe/legacy TypeScript type cleanup from #586 (
ui-kit/utilities,ui-kit/directives,ui-kit/layout, and misc small areas), which had been merged into the now-stalegh-582-apply-safe-eslint-autofixes-and-lower-the-baselinebranch instead ofmaster. That branch's own PR (#675, the safe-autofix slice) merged tomasteron 2026-09-09; #702 (this type-safety work) was reviewed and merged intogh-582an hour later, leaving the additional commits stranded offmasterwith no open PR.This PR rebases that remaining work onto current
master:masterinto thegh-582branch, resolving 3 conflicts against unrelated intervening changes onmaster(Defer standalone-component migration and disable prefer-standalone lint rule #683, Fix Angular template accessibility lint debt #685, Retire resolved ZAP 10003 exception now that test-app is on Angular 20.3.30 #712):src/ui-kit/layout/filter-drawer/dynamic-chips/dynamic-chips.directive.ts— kept the typedFilterDrawerChipModelparameter over master's untypedfilterparam.src/ui-kit/utilities/key-helper/key-helper.ts— kept theunknown+toKeyEventLike()narrowing signature over master'sKeyboardEvent | any.src/ui-kit/wrappers/fieldset-wrapper/fieldset-wrapper.component.ts— kept the addedElementRef/SimpleChangesimports needed by this branch's typedngOnChanges/@ViewChild.eslint-baseline.json's root entry from1059to924(measured, vianpm run lint:baseline:bump) to reflect the cleanup already present on this branch.No new source changes beyond conflict resolution — the underlying type-safety work is what #702 already reviewed and approved.
Motivation and Context
Closes #586
Type of Change (Select One and Apply Label)
bugfixlabelenhancementlabelbreakinglabelmaintenancelabelHow to Test
npm ci && npm ci --prefix test-appnpm run lint:baseline(root: 924/924, 0 errors) andnpm --prefix test-app run lint:baseline(test-app: 0/0)npm run format:checknpm --prefix test-app run build— Angular 20 production build succeedsnpm --prefix test-app test— 155 spec files / 1419 tests passnpm run coverage:check— coverage floor gate passes (statements 89.56%, branches 79.29%, functions 86.70%, lines 89.29%, all above floor)Expected result: all checks above pass; no consumer-facing API changes.
Screenshots (if appropriate)
N/A — internal type-safety cleanup, no UI changes.
Checklist
gh-<number>-<slug>)format:checkpasses (npm run format:check)lintpasses (npm run lint)buildpasses (cd test-app && npm run build)cd test-app && npm test)