refactor(core-web): TS strict mode across utils-testing, utils, dotcms-js, sdk-create-app, sdk-react, sdk-analytics, sdk-angular, data-access + webcomponents groundwork - #36957
Conversation
`sdk-types` needs no code change: `libs/sdk/types/tsconfig.json` has carried `strict: true` plus the four extra safety flags since the library was created (#31967), and `tsc -p tsconfig.lib.json --noEmit` passes with zero errors. It is already enforced too. Because `tsconfig.lib.json` sets `"declaration": true`, `@rollup/plugin-typescript` sits in the Rollup chain and reports type diagnostics, so `sdk-types:build` fails on a strict violation — verified by removing a constructor assignment and watching the build report TS2564. CI builds every project via the `build-test` execution in `core-web/pom.xml`, so the gate already runs on each PR. A dedicated `typecheck` target would be redundant. `lint` does not catch this: ESLint reports lint rules, not TS diagnostics. What was actually missing is documentation, so the remaining 42 projects in epic #35932 have a pattern to follow: - Add a `## TypeScript Strict Mode` section covering the per-project flags, what enforces them, and the Vite exception (esbuild skips type checking, which is why the Nx Vite plugin infers a separate `typecheck` target). - Fix the line that forbade `"strict": true` in project tsconfigs. It sat under the Jest config guidance but read as a blanket ban, contradicted `docs/frontend/TYPESCRIPT_STANDARDS.md`, and blocked the epic outright. The restriction now points at `tsconfig.spec.json`, which is what it meant. Closes #35935 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add the standard per-project strict flags to `libs/sdk/create-app/tsconfig.json`, following the pattern established in #36879 (dotcms-models). `tsconfig.base.json` is left at `strict: false`. Two errors surfaced, both from flags beyond plain `strict`: - `src/index.ts:393` — `process.env.DEBUG` needs bracket access under `noPropertyAccessFromIndexSignature` (TS4111). It is the only `process.env.*` dot access in the project. - `src/utils/index.ts:41` — `fetchWithRetry` tripped `noImplicitReturns` (TS7030). The loop returns on success and throws on the last attempt, but with `retries < 1` the loop never runs and the function fell through returning `undefined`. Its only caller already guarded with `if (res && ...)`, so nothing broke in practice, but the signature was lying. Throwing after the loop closes the gap and narrows the return type. No build or CI wiring needed. The `@nx/esbuild:esbuild` executor type-checks before bundling — `skipTypeCheck` defaults to false and is not overridden — and CI already builds this project via `nx run-many -t build` (`build-test` in core-web/pom.xml). The same build runs in the SDK release pipeline (`cicd_release-sdk.yml` → `nx run-many --projects='sdk-*'`), so the flags are enforced on every release. Verified: tsc clean on lib and spec; `nx run sdk-create-app:build/lint/test` green; `nx affected -t build,lint` green; `node dist/libs/sdk/create-app/index.js --help` still works. Negative test — reverting the DEBUG fix makes `nx run sdk-create-app:build` fail with TS4111, confirming the gate is real. Closes #35938 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @nicobytes's task in 43s —— View job SDK Compatibility Analysis
No SDK breaking change detected. This PR is a TypeScript strict-mode rollout (
The Everything else is internal library typing (nullability widening, |
There was a problem hiding this comment.
Pull request overview
This PR opts the sdk-create-app library into the workspace’s incremental TypeScript strict-mode rollout (issue #35932), and adjusts docs/runtime code to align with stricter typing and clearer failure modes.
Changes:
- Enabled strict TypeScript compiler flags for
core-web/libs/sdk/create-appvia its projecttsconfig.json. - Updated
fetchWithRetryto throw when misconfigured with< 1attempts to avoid an implicitundefinedreturn path. - Updated strict-mode rollout documentation and adjusted DEBUG env access to bracket notation for
noPropertyAccessFromIndexSignature.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core-web/libs/sdk/create-app/tsconfig.json | Enables strict compiler options at the project level for the strict-mode rollout. |
| core-web/libs/sdk/create-app/src/utils/index.ts | Adds an explicit throw path for invalid retries values in fetchWithRetry. |
| core-web/libs/sdk/create-app/src/index.ts | Switches DEBUG env access to process.env['DEBUG'] for strict-mode compatibility. |
| core-web/CLAUDE.md | Documents the strict-mode rollout procedure and clarifies portlet tsconfig guidance. |
Add the standard per-project strict flags to `libs/dotcms-js/tsconfig.json`, following the pattern from #36879 (dotcms-models), and resolve the 38 errors they surface across 11 files. `tsconfig.base.json` stays at `strict: false`. Notable type corrections rather than mechanical silencing: - `Auth.loginAsUser` was typed `User` but the code has always passed `null` when nobody is impersonating, and every consumer already guards with `auth.loginAsUser || auth.user`. Corrected to `User | null`. - `StringUtils.getLine` and `HttpRequestUtils.getQueryStringParam` both document "null if it does not exist" but were typed `string`. Corrected. - `RoutingService.getPortletURL` returns `Map.get()`, so `string | undefined`. - `SiteService.switchSiteById` emits `of(null)` when no site is found, so `Observable<Site | null>`. Its one consumer already handles null. - `ResponseView` now models `HttpResponse.body` as nullable instead of assigning `null` into a non-nullable field inside a `try/catch` that could never throw. The dead try/catch is removed. - `LoginService.urls` is typed by inference instead of `Record<string, string>`, which keeps dot access valid and gives each endpoint a named property. Two definite-assignment assertions were used, each with a TODO: `_auth` and `selectedSite` are assigned during init but not in the constructor. Modelling them as `| undefined` is the truthful type, but their public getters (`auth`, `currentSite`) are consumed by already-strict projects, so widening them is a public-API change that belongs in its own issue. No new `any`, `@ts-ignore`, or `@ts-expect-error`. Verified: - `tsc -p libs/dotcms-js/tsconfig.lib.json --noEmit` — 0 errors - All six already-strict consumers build green (data-access, global-store, portlets-dot-analytics, portlets-dot-analytics-data-access, portlets-dot-locales-portlet, utils-testing) - `data-access` typecheck went from 106 errors to 68, with zero new errors introduced — the honest types upstream remove noise downstream - `dotcms-ui` typecheck clean apart from a pre-existing missing `dotcms-webcomponents/loader` dist - `nx format:check` green; dotcms-js lint went from 42 to 41 problems Note: this project has no `build` target and is tag-excluded from lint and test, so nothing in CI verifies these flags. That was an explicit scoping decision — no `typecheck` target or CI gate was added. See `specs/35939-dotcms-js-strict-mode/spec.md`. Closes #35939 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add the standard per-project strict flags to `libs/utils/tsconfig.json`, following the pattern from #36879 (dotcms-models), and resolve the 32 errors they surface across 3 files. `tsconfig.base.json` stays at `strict: false`. The flags also propagate to `tsconfig.spec.json`, which surfaced 17 further errors in the spec files (baseline was 0). Those are fixed here too rather than left as a regression. Notable changes: - `EMPTY_FIELD` assigned `null` to 18 members that `DotCMSContentTypeField` declares non-nullable. Replaced with zero values of the declared types. Nothing compares those members to `null` strictly — consumers use falsy checks such as `isNewField`'s `!field.id` — so `''`, `0` and `false` behave identically at runtime. - `clazz` has no zero value (`DotCMSClazz` is a union of concrete Java class names), so `EMPTY_FIELD` and `EMPTY_SYSTEM_FIELD` are now typed `Omit<DotCMSContentTypeField, 'clazz'>`. They are partial templates, not valid fields, and the type now says so. The derived `COLUMN_FIELD`, `ROW_FIELD` and `TAB_FIELD` already supply their own `clazz`. - `getFieldsWithoutLayout` used a truthy `.filter()` that does not narrow the optional `row.columns`. Replaced with a type predicate, which clears the TS2532 and both TS2769 errors without a cast. - `ellipsizeText` accepted `null`/`undefined` at runtime — its own guard and its tests document that — but declared `string` and `number`. Widened to match, with an explicit `limit == null` check so the later comparisons narrow. - `fallbackErrorMessages` typed `{ [key: number]: string }`, mirroring the identical declaration already in `libs/data-access/.../dot-upload.service.ts`. - `dot-utils.ts` uses bracket access for the six `DotCMSContentlet` index-signature reads in `getImageAssetUrl`. No new `any`, `@ts-ignore`, or `@ts-expect-error`. The nine `as unknown as` casts added are all in spec files, on inputs the tests deliberately pass as invalid, matching the idiom those files already used. Verified: - `tsc -p libs/utils/tsconfig.lib.json --noEmit` — 0 errors (from 32) - `tsc -p libs/utils/tsconfig.spec.json --noEmit` — 0 errors (from 17) - `data-access` typecheck went from 68 errors to 36, zero new - `utils-testing` unchanged at 1 pre-existing error (missing jasmine types) - `dotcms-ui` typecheck clean apart from a pre-existing missing `dotcms-webcomponents/loader` dist - `nx format:check` green Note: `utils` has no `build` target and is tag-excluded from lint and test, so nothing in CI verifies these flags — the same accepted trade-off as #35939. Closes #35940 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses three review comments on #36957. 1. `dot-content-types.mock.ts` — real regression, now fixed. `dotcmsContentTypeFieldBasicMock` spreads `EMPTY_SYSTEM_FIELD`, which #35940 retyped to `Omit<DotCMSContentTypeField, 'clazz'>`, leaving the mock without a required property (TS2741). It now supplies `clazz: DotCMSClazzes.TEXT`; callers that care already override it. Why the original verification missed it: `libs/utils-testing/tsconfig.lib.json` declares `"types": ["jasmine"]` and that package is not installed, so tsc emits `TS2688: Cannot find type definition file for 'jasmine'` and stops before semantic checking. The "1 error before, 1 after" measurement reported in #35940 therefore proved nothing — nothing was being checked. Running with `--types node` reveals 33 errors, including the TS2741. It is 32 after this fix. Verified the runtime-value change, since the mock has ~103 consumers whose tests do run in CI: `clazz` went `null` (pre-PR) → absent (#35940) → `TEXT`. `FieldUtil.isRow`/`isColumn`/`isTabDivider` compare for equality and return false for all three, and there is no `!field.clazz` or `=== null` check anywhere. Test runs: `default-value-property` 7/7, `dot-content-types-edit` 545 passed across 48 suites, `data-access` 751 passed across 79 suites. 2. `sdk-create-app/src/utils/index.ts` — the throw said "requires at least 1 retry", but `retries` is the total attempt count (`for (i = 0; i < retries)`), so `retries = 1` means one attempt and zero retries. Reworded to "attempt" and the ambiguity noted in the comment. 3. `core-web/CLAUDE.md` — the verify snippet hard-coded `libs/<project>/tsconfig.lib.json`, which resolves for neither nested projects (`libs/sdk/create-app`, which has no `tsconfig.lib.json`) nor apps (`tsconfig.app.json`). Replaced with a `<projectRoot>` placeholder plus the two caveats, a reminder that `tsconfig.spec.json` inherits the flags, and a warning about unresolved `types` entries masking all semantic diagnostics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`sdk-uve` needs no change for [08/44]. The six strict flags have been in `libs/sdk/uve/tsconfig.json` since the library was created (`277cbbc8f7`, #31242, Feb 2025) as a verbatim copy of `sdk-client`'s config, `tsc --noEmit` is clean on both lib and spec, and there are zero `any`, `@ts-ignore` or non-null assertions across 4518 lines. It is also genuinely enforced, which is what separated `sdk-types` from `dotcms-js` and `utils`. `rollup.config.cjs` sets `compiler: 'babel'`, but that governs only transpilation — `@nx/rollup`'s `withNx` always inserts a TypeScript plugin with `check`/`noEmitOnError` tied to `skipTypeCheck`, which this project does not set. Two of the three type-checking paths run in CI, and the `build-test` execution in `core-web/pom.xml` has no `<skip>` element, so it cannot be turned off. Issue closed as completed with the evidence; not linked to PR #36957 since there is no diff and that PR did not resolve it. Also records an incidental finding, left unfixed: `tsconfig.base.json:104` maps `@dotcms/uve/types` to a file that does not exist, and nothing imports it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`sdk-client` needs no change for [09/44]. The six strict flags are already in `libs/sdk/client/tsconfig.json`, `tsc --noEmit` is clean on both lib and spec, and there are zero `any`, `@ts-ignore` or non-null assertions across 9600 lines of production source. Enforcement is unambiguous here, unlike the sibling projects that needed an argument: `rollup.config.cjs` sets `compiler: 'tsc'` against `tsconfig.lib.json` with no `skipTypeCheck`, so the build compiles with tsc directly against the strict config. `tags` is empty and the `build-test` execution in `core-web/pom.xml` has no `<skip>` element, so that build runs on every PR and gates every SDK release. Issue closed as completed with the evidence; not linked to PR #36957 since there is no diff and that PR did not resolve it. Also records an emerging pattern for the remaining issues: every `libs/sdk/*` project checked so far is already strict and already enforced — they share a tsconfig lineage (sdk-uve's config is a verbatim copy of this one) and all build through Nx executors that type-check. The unfinished work is concentrated in the non-SDK libraries and the apps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| @Element() element!: HTMLElement; | ||
|
|
||
| @State() showSuggestions: boolean; | ||
| @State() showSuggestions!: boolean; |
There was a problem hiding this comment.
These two @State fields are annotated with !, but the file's own logic treats them as optional — onKeyDown/onKeyPress check this.selectedSuggestionIndex === undefined, and onSelect assigns this.selectedSuggestionIndex = undefined, which won't compile against number once #35943 enables strict here. Since the split rule in this PR reserves ! for runtime-injected members, shouldn't these two be ? instead?
| error?: string; | ||
| }>; | ||
| @State() previewImg: string; | ||
| @State() previewImg!: string; |
There was a problem hiding this comment.
previewImg is only assigned inside the async postMessage handler, and render() itself branches on !this.previewImg to show the spinner — the component's own logic proves it's legitimately unset on first render. Given the !-for-runtime-injected / ?-for-optional split this PR establishes, shouldn't this one be previewImg?: string?
|
|
||
| @State() | ||
| classNames: DotFieldStatusClasses; | ||
| classNames!: DotFieldStatusClasses; |
There was a problem hiding this comment.
Unlike the sibling fields in this PR (dot-textfield/dot-tags assign status in componentWillLoad), classNames here is only assigned inside the _dotStatusChange listener, so it's genuinely undefined on first render — the {{ ...this.classNames }} spread just happens to tolerate it. Should this be classNames?: DotFieldStatusClasses rather than !?
| sortOrder: 0, | ||
| unique: false, | ||
| variable: '', | ||
| defaultValue: undefined, |
There was a problem hiding this comment.
The PR body documents the EMPTY_FIELD change as "null → zero values", but defaultValue, hint and values actually became undefined. Since JSON.stringify drops undefined-valued keys while keeping explicit nulls, every new row/column/tab-divider sent through DotFieldService.saveFields (PUT /api/v3/contenttype/{id}/fields/move) now omits those three keys instead of sending them as null. Was backend equivalence (missing key vs explicit null on Jackson deserialization) verified for this endpoint? If so, worth adding to the documented-changes list.
| // TODO(#35939): assigned by `setCurrentSite()` during init, never in the constructor. | ||
| // Same trade-off as `LoginService._auth`: the public `currentSite` getter is widely | ||
| // consumed, so widening it to `Site | undefined` belongs in its own issue. | ||
| private selectedSite!: Site; |
There was a problem hiding this comment.
This file has a specific history with exactly this bug class: #32019 shipped twice ("Site Selector disappearing" / "current site undefined", fixed in #33168 and #33204 by adding filter((site) => !!site) guards), and eventResponse() still dereferences this.selectedSite.identifier unguarded. Since the TODO defers the honest Site | undefined type to "its own issue" — has that follow-up issue been filed? Referencing it in the TODO would keep this from being deferred indefinitely on a known-regression surface.
| options: DotAssetCreateOptions | ||
| ): Promise<DotCMSContentlet[] | DotHttpErrorResponse[]> { | ||
| const promises = []; | ||
| const promises: Promise<Response>[] = []; |
There was a problem hiding this comment.
Each promise pushed here ends in .catch((e) => e), so a rejected fetch resolves with the error object — the settled value is really Response | Error, and the downstream res.json() would throw on it. Since the .catch returns any, the new Promise<Response>[] annotation compiles but codifies a contract the code doesn't honor. Would Promise<Response | Error>[] (or Promise.allSettled) be more truthful here?
) sdk-angular already carried all six strict flags plus Angular's strictTemplates/strictInjectionParameters/strictInputAccessModifiers, so no flags were added. The real defect was dead config: both tsconfigs referenced a `next/` directory that existed and was removed (refs added 2025-03-21 in 09e879b). One reference was fatal. tsconfig.spec.json listed `next/test-setup.ts` in `files`, so tsc aborted with TS6053 before semantic checking — that config had never completed a single semantic pass, and any error count taken from it was meaningless. A non-matching `include` glob is harmless; a missing `files` entry is not, which is why tsconfig.lib.json kept working. Removed the dangling references from both files. Both configs now report 0 own errors; the single remaining error is the pre-existing, unrelated `virtual:sdk-version` from sdk-client. Verified the build is a real gate rather than assuming it: a deliberate type error in dotcms.store.ts fails `nx run sdk-angular:build` with TS2322 (exit 1), since @nx/angular:package runs ngtsc. Probe reverted. Contrast sdk-analytics (#35946), where the Vite build ignores type errors. lint clean; test unchanged at 21 suites / 234 tests, which is the guard that nothing dropped out of the program. Also documents the TS6053 masking variant in core-web/CLAUDE.md alongside the existing TS2688 one, and restores the #35946 spec write-up that the issue comment references. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
data-access has carried all six strict flags plus Angular's strict template options for some time, and they were doing nothing: the project has no `build` target, so its own tsconfig is never read, and its 27 dependents compile these sources under their own non-strict configs. 36 errors sat in a layer-3 shared services hub with CI green. Production source (36): - paginator: 8 uninitialised fields given zero values; header reads guarded with `?? ''` (same NaN outcome); `setLinks` widened to `string | null` since its body already handled null; `Links` given an index signature because the Link-header parser stores arbitrary `rel` values. `_sortOrder` deliberately stays optional — defaulting it to OrderDirection.ASC would be a behaviour change, as getParams() gates the `direction` param on truthiness and ASC === 1 is truthy. - dot-page-state: widened six declarations that were simply wrong. The key one is handleSetPageStateFailed, which ends in `map(() => undefined)` and so really emits undefined — meaning the caller's `[null, null]` destructuring default is load-bearing, not dead code. Typing it honestly made the switchMap typable; it now destructures explicitly. `if (page)` became `if (page && user)`, which forkJoin already guaranteed. - dot-router, dot-localstorage, dot-content-types-info: nullable getters, localStorage reads, and a string index narrowed to keyof. Specs (47): 25 came from three untyped jest.fn mocks on a fake Router that inferred zero parameters. Two were real bugs hidden behind disabled suites — dot-global-message imported DotMessageService from a module that does not export it, and dot-ai.service.ts re-exported a type without `export type`. dot-page-layout was testing a payload shape production never sends. The content-drive fixture used a removed `offset` field copied from the model's own stale JSDoc example, which is fixed too. No new `any`, no @ts-ignore / @ts-expect-error. Verified: tsc 36→0 (lib) and 84→0 (spec); lint clean; test unchanged at 754 passing. Blast radius measured across all 7 strict dependents — zero new errors, 218 removed, and global-store, portlets-dot-analytics-data-access and portlets-dot-experiments-data-access went 36→0, having carried nothing but this library's leakage. Runtime guard: dotcms-ui 820 tests and ui 2184 tests pass. Still unenforced by CI, consistent with #35939 and #35940. Also documents that ts-jest runs transpile-only whenever tsconfig.spec.json sets isolatedModules, so no `test` target type-checks specs anywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Correction found while working #35948. The #35947 write-up claimed jest-preset-angular/ts-jest type-checks spec files, and that sdk-angular's 234 passing tests therefore evidenced type-cleanliness. It does not: ts-jest copies TypeScript's `isolatedModules` into its own transpile-only switch (config-set.js:229) and then skips building the language-service host that diagnostics require (ts-compiler.js:74). data-access is the counterexample that exposed it — same setup, 84 tsc errors alongside 754 passing tests. The #35947 verdict stands; it was measured with `tsc -p` directly, not inferred from the test run. Only the justification was wrong. Because CLAUDE.md mandates `isolatedModules: true` in every tsconfig.spec.json, this holds repo-wide: no project's `test` target type-checks its specs. Documented alongside the existing TS2688 and TS6053 masking notes, since it is the same trap in a third form — a green signal that checked nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tsconfig.json carried only `strict`. Measured the other five on the CLI before committing them: 0 errors on both tsconfig.lib.json and tsconfig.spec.json, so nothing was hiding behind them. Pure flags change. Enforcement confirmed by negative test rather than assumed — despite an inferred `typecheck` target, this project builds with Rollup, so @rollup/plugin-typescript is in the chain and a deliberate type error fails `nx run sdk-experiments:build` with TS2322 (exit 1). Probe reverted. lint, test (48) and build all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#35952 #35954) Four projects verified against the rollout bar. All four already carried all six flags, and none was masked by a TS6053/TS2688 config error, so their counts were real. Three needed no change at all — portlets-dot-locales-data-access (#35950), portlets-dot-experiments-data-access (#35952) and, after the one fix below, portlets-dot-analytics-data-access (#35954). Two of them reported 36 errors before #35948; every one was data-access leaking through imports, so fixing that library took them to 0 with nothing touched here. The one real defect was in global-store (#35951): src/index.ts re-exported WebSocketStatus with `export {}`, but it is a type, so under isolatedModules that is TS1205. global-store's own configs never reported it — no spec there imports ./index, so the file was never in its own program. It only surfaced from consumers, showing up as the single spec error attributed to portlets-dot-analytics-data-access. `export type` fixes both issues at once. Generalisation worth carrying: a barrel file can hold an isolatedModules error that only its consumers ever see. Second instance after dot-ai.service.ts in #35948. None of the four is enforced — no build target, and :test does not type-check since isolatedModules puts ts-jest in transpile-only mode. tsc clean on lib and spec for all four; lint and test green (global-store 187 tests, analytics-data-access 217). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Partial progress on ui, the rollout's biggest bottleneck: ~109 of its
errors leak into each of its 26 dependents, so this unblocks a large part
of the epic. tsconfig.lib.json is now at 0 (from 122); tsconfig.spec.json
is at 123 (from 427) and continues in a follow-up commit.
Flags added to libs/ui/tsconfig.json (it had none of the six).
Production fixes follow the policies agreed for the epic — zero values for
TS2564 where one exists, definite assignment only for FormGroup/Observable/
ViewChild, and guards rather than assertions where null is reachable:
- dot-icon's `size` deliberately became `size?: number` instead of `= 0`.
The template binds [style.font-size.px]="size", so 0 would have rendered
invisible icons where undefined inherits.
- dot-sidebar, dot-dropdown, dot-site-selector, dot-container-options and
dot-trim-input all inject their host with { optional: true } and then used
it unguarded. They now guard.
- Types that were simply wrong: `formEl` was declared HTMLFormElement while
the template says #formEl="ngForm" (it is an NgForm); getVariableIndexChanged
declared `number` while its own JSDoc documented `number | null`;
getDefaultBundle returned null under a non-nullable type.
- Real defect found: dot-add-to-bundle invoked getDefaultBundle twice for the
same value. Now once.
- tsconfig.lib.json excluded a non-existent src/test.ts but not test-setup.ts,
**/*.test.ts or __mocks__/, so test files were compiled into the library
program. That alone accounted for 15 errors.
- htmldiff-js ships no types; added a minimal module declaration rather than
silencing the import.
Specs went 427 -> 123 mostly by asserting at the point of declaration rather
than at every use: 53 usages of one `select` local came from a single line, so
95 declaration-level assertions cleared roughly 300 errors.
One behaviour change worth naming: the gravatar directive now clears the
PrimeNG avatar with `undefined` instead of `null`, because Avatar declares
image?/label? as optional strings and null was never assignable. Five spec
assertions moved from toBeNull to toBeUndefined to match.
lint clean (8 pre-existing warnings); test unchanged at 81 suites / 820
passing, verified against the pre-change baseline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
Nine steps of the strict-mode rollout (epic #35932), plus groundwork on a tenth:
data-accessbuildtarget) — fixes the 36 lib + 47 spec errors hiding behind themsdk-angularnext/tsconfig refs that madetsconfig.spec.jsonunverifiable (TS6053)sdk-analyticsTS4111in source and 29 in specs (14 pre-existing)sdk-reactTS4111index-signature accessesutils-testingtypes: ["jasmine"]aborted all type checkingutilsstrict+ fix the 32 (+17 spec) resulting type errorsdotcms-jsstrict+ fix the 38 resulting type errorssdk-create-appstrict+ fix the 2 resulting type errorssdk-typesAll three add the standard six flags to the project's own
tsconfig.json, following the pattern established in #36879 (dotcms-models).tsconfig.base.jsonstays at"strict": false— the rollout never flips it globally.dotcms-js(#35939)The largest of the three: 38 errors across 11 files, in a layer-1 core library with 20 dependent projects, including the
dotcms-uiadmin app. Six of those dependents are already strict, so this library's loose types were leaking uncertainty into projects that had opted into rigour.Most fixes correct types that were simply wrong, rather than silencing the compiler:
Auth.loginAsUserUsernullwhen nobody is impersonating, and every consumer already guards withauth.loginAsUser || auth.user. NowUser | null.StringUtils.getLinestringstring | null.HttpRequestUtils.getQueryStringParamstringRoutingService.getPortletURLstringMap.get(). Nowstring | undefined.SiteService.switchSiteByIdObservable<Site>of(null)when no site is found. NowObservable<Site | null>; its one consumer already handled null.ResponseView.bodyJsonObjectDotCMSResponse<T>HttpResponse.body, which is nullable. The surroundingtry/catchcould never throw and has been removed.LoginService.urlsmoved fromRecord<string, string>to inference-typed, which resolves all 8TS4111errors at once and gives each endpoint a named property.Two definite-assignment assertions were used, each with a
TODO:LoginService._authandSiteService.selectedSiteare assigned during init but not in the constructor. Modelling them as| undefinedis the truthful type, but their public getters (auth,currentSite) are consumed by already-strict projects, so widening them is a public-API change that belongs in its own issue.No new
any,@ts-ignore, or@ts-expect-erroranywhere in this PR.Blast-radius verification
data-access(a strict consumer) went from 106 type errors to 68, with zero new errors introduced — the honest types upstream remove noise downstream.dotcms-uitypechecks clean apart from a pre-existing missingdotcms-webcomponents/loaderdist.utils(#35940)32 errors across only 3 files, plus 17 more that appeared in the spec files once the flags propagated through
tsconfig.spec.json(baseline there was 0). Both are fixed here — leaving the spec errors would have shipped a regression.The bulk was one constant.
EMPTY_FIELDassignednullto 18 members thatDotCMSContentTypeFielddeclares non-nullable:nullstrictly — consumers use falsy checks such asisNewField's!field.id— so'',0andfalsebehave identically at runtime.clazzhas no zero value (DotCMSClazzis a union of concrete Java class names), soEMPTY_FIELDandEMPTY_SYSTEM_FIELDare nowOmit<DotCMSContentTypeField, 'clazz'>. They are partial templates, not valid fields, and the type now says so. The derivedCOLUMN_FIELD/ROW_FIELD/TAB_FIELDalready supply their ownclazz, so they remain complete.Other fixes:
getFieldsWithoutLayout.filter()did not narrow the optionalrow.columns. A type predicate clears theTS2532and bothTS2769without a cast.ellipsizeTextnull/undefinedat runtime — its own guard and its tests say so — but declaredstring/number. Widened to match, with an explicitlimit == nullcheck so later comparisons narrow.fallbackErrorMessages{ [key: number]: string }, mirroring the identical declaration already inlibs/data-access/.../dot-upload.service.ts.dot-utils.tsDotCMSContentletindex-signature reads ingetImageAssetUrl.dot-asset.service.tspromisesand the twofetchAssetparams.The nine
as unknown ascasts added are all in spec files, on inputs the tests deliberately pass as invalid, matching the idiom those files already used.Blast-radius verification
data-access(strict consumer) went from 68 type errors to 36, zero new.utils-testing(strict) unchanged at its 1 pre-existing error — theOmitdid not break itsEMPTY_SYSTEM_FIELDspread.sdk-create-app(#35938)Two errors, both from flags beyond plain
strict:src/index.ts:393—process.env.DEBUGneeds bracket access undernoPropertyAccessFromIndexSignature(TS4111). It is the onlyprocess.env.*dot access in the project.src/utils/index.ts:41—fetchWithRetrytrippednoImplicitReturns(TS7030). The loop returns on success and throws on the last attempt, but withretries < 1the loop never runs and the function fell through returningundefined. Its only caller (isDotcmsRunning,src/index.ts:506) already guarded withif (res && …), so nothing broke in practice — but the signature was lying. Throwing after the loop closes the gap and narrows the return type toPromise<AxiosResponse>.No build or CI wiring was needed here. The
@nx/esbuild:esbuildexecutor type-checks before bundling (skipTypeCheckdefaults tofalseand is not overridden), and CI already builds this project vianx run-many -t build(build-testincore-web/pom.xml). The same build runs in the SDK release pipeline (cicd_release-sdk.yml→nx run-many --projects='sdk-*'), so the flags are enforced on every release.sdk-types(#35935)libs/sdk/types/tsconfig.jsonhas carriedstrict: trueplus the four extra safety flags since the library was created (#31967), andtsc --noEmitpasses with zero errors. It is also already enforced:tsconfig.lib.jsonsets"declaration": true, so@rollup/plugin-typescriptsits in the Rollup chain and fails the build on a strict violation.So no code change was required. What was missing was documentation, added here to
core-web/CLAUDE.md:## TypeScript Strict Modesection covering the per-project flags, what actually enforces them, and the Vite exception (esbuild skips type checking, which is why the Nx Vite plugin infers a separatetypechecktarget)."strict": truein project tsconfigs. It sat under the Jest config guidance but read as a blanket ban, contradicteddocs/frontend/TYPESCRIPT_STANDARDS.md, and blocked the epic outright. The restriction now points attsconfig.spec.json, which is what it meant.utils-testing(#35944)The six strict flags were already in
tsconfig.json— but completely inert.tsconfig.lib.jsondeclared"types": ["jasmine"], that package is not installed, sotscemittedTS2688: Cannot find type definition file for 'jasmine'and stopped before semantic checking. The project reported exactly one error regardless of what the code did.The reference was stale: nothing uses jasmine, two files use
jest.*, and@types/jestis installed. Switching to"types": ["jest"]removed the abort and 27 spuriousCannot find name 'jest'errors, leaving 5 real ones:clean-up-dialog.tsfixtureparam → typed structurally as{ nativeElement: unknown }, since only that property is touched (no need to pull in Angular'sComponentFixture)dot-page-state.service.mock.ts_lock: boolean = null→boolean | nulldot-page-tools.mock.ts×3tagsarray thatDotPageTooldoes not declare. Verified nothing in the repo reads.tagsoff a page tool, so the dead field was removed rather than added to the model indotcms-modelstsc -p libs/utils-testing/tsconfig.lib.json --noEmitnow exits 0 with no CLI overrides — the check is real rather than short-circuited.Verified across consumers of the touched mocks (
cleanUpDialogin 7 files, page-tools mock in 3):data-access751 tests passed,edit-ema-ui338 passed.dotcms-webcomponents(#35943) — groundwork only, not closedStrict is not enabled here. ~250 errors remain across 38 files, and unlike the other projects this one has no
skip:build, so Stencil type-checks it on every PR — flipping the flag early turns CI red. What landed is the part that is correct on its own.The decorator split, which is the load-bearing decision. Stencil declares runtime-injected members without initializers, colliding with
strictPropertyInitialization(139 of the original 375 errors). The fix cannot be uniform:@Event!EventEmitter@Element!@State!@Prop?Using
!on@Propmade Stencil emit 28 props as required incomponents.d.ts— breaking for any TS/JSX consumer. With?the generated API moves required → optional, which is backward compatible. Measured in the generated file, not assumed.Two traps recorded on the issue
Stencil under-reports. Its build shows ~10 files / ~39 errors per run, not the total. Measured at the same commit: Stencil 39 errors / 10 files vs
tsc250 / 38. Size this work withtsc, not with build output.--skip-nx-cachedoes not clear Stencil's cache. Builds can report green against stale.stenciloutput. This bit me:0117273504annotated a prop, passed a "clean" build, and was actually broken — reverted inf22afce383after verifying twice with.stenciland the Nx cache cleared.That prop (
dot-binary-text-field'svalue) is genuinely contradictory:handleFilePasteassigns aFile, other paths assign strings, and the template feeds it to an<input value>that accepts neither. No annotation describes the current code — the render path has to be fixed first. Left untyped with aTODO(#35943)so it is not re-annotated in isolation.sdk-react(#35945)strict: truewas already present; the five companion flags were not. Adding them surfaced 14 errors, allTS4111— dot access on a type carrying an index signature — resolved with bracket notation. Two origins, same fix:node.attrs, declaredRecord<string, any>in@dotcms/types. That type is deliberately left alone: block editor attributes really are dynamic, and it lives in a layer-0 project whose consumers would all be affected.styles.rowinRow.tsx), whose generated type is also aRecord<string, string>.No behaviour change — bracket access compiles to the same property lookup.
The flags here are genuinely enforced, and that was proved rather than assumed. Reverting one access to dot notation fails the build with
@rollup/plugin-typescript TS4111, confirming TypeScript sits in the Rollup chain. The project carries noskip:tags, so CI builds, lints and tests it on every PR, and the same build runs in the SDK release pipeline.sdk-analytics(#35946)Same starting shape as
sdk-react:strict: truealready present, the five companion flags absent. But this one is not enforced, and that was established by test rather than inference.18 errors in production source, all
TS4111fromnoPropertyAccessFromIndexSignature— dot access onHTMLElement.dataset(DOMStringMap) and on aRecord<string, unknown>of payload properties. Bracket notation throughout, reads and writes alike. Spread acrossdot-analytics.utils.ts(10),dot-analytics.click-tracker.ts(4),dot-analytics.impression-tracker.ts(3),dot-analytics.click.utils.ts(1).29 errors in specs — 15 more of the same mechanical
datasetfix, plus 14 that were pre-existing drift rather than strict-mode fallout. Confirmed pre-existing: they persist identically under--strict false. Two independent gaps had let them accumulate unseen — the inferredtypechecktarget runs onlytsconfig.lib.json, andjest.config.tstransforms viababel-jest, which strips types without checking them.ANALYTICS_CONTENTLET_CLASSno longer exported — renamed toCONTENTLET_CLASSdeviceinsidedataand omitted requiredlocale_idjest.fn()inferringneverformockResolvedValue/mockRejectedValueLocationmock missinghostjest.spyOn(...).mockImplementation()called with no argumentmockInitializeinferred as zero-argresult.custom— not onEnrichedTrackPayloadTS2589excessively deep instantiationThe pageview fixture was the instructive one: with
devicemisplaced andlocale_idmissing, thepageviewmember of theDotCMSEventunion stopped matching, so TypeScript fell through to the impression member and reported a misleading "doc_encodingdoes not exist onDotCMSContentImpressionPageData". One coherent fix cleared four errors. Fixtures were corrected rather than production types widened; no source bug hid behind any of them.So
sdk-analyticsjoinsdotcms-jsandutilsas strict but unenforced. Wiringnx affected -t typecheckintocore-web/pom.xmlwas deliberately left out — it is monorepo-wide and belongs to the epic, not to project 13 of 44. Both gaps are raised on #35932.This also corrects the pattern proposed in #35942 — that every
libs/sdk/*project was already strict and already enforced. That holds for the Rollup-built SDK libs, which type-check through@rollup/plugin-typescript(assdk-reactproved). It does not hold for Vite-built ones:sdk-analyticsinheritedstrictfrom the shared tsconfig lineage but neither the other five flags nor a type-checking build.0 internal dependents — the only references to
@dotcms/analyticsoutside the lib are doc comments inlibs/sdk/uve/src/internal/constants.ts. No blast radius.sdk-angular(#35947)No flags were added — all six were already there, plus Angular's
strictTemplates,strictInjectionParametersandstrictInputAccessModifiers. Re-adding them would have been a cosmetic diff. The real defect was dead config.Both
tsconfig.lib.jsonandtsconfig.spec.jsonreferenced anext/directory that existed and was removed — the references landed on 2025-03-21 (09e879b2ac) and outlived the directory. One of them was fatal:tscaborts on that before semantic checking, sotsconfig.spec.jsonhad never completed a single semantic pass and any error count taken from it was meaningless. The asymmetry is the lesson: a non-matchingincludeglob is harmless, a missingfilesentry is fatal — which is whytsconfig.lib.json, whosenext/references were only ininclude/exclude, kept working.Removed the dangling references from both. Both configs now report 0 own errors; the one remaining error in each is the pre-existing, unrelated
virtual:sdk-versionfromsdk-clientdocumented in thesdk-reactsection above.The spec config coming out clean was predicted, not lucky:
jest-preset-angular@17→ts-jest@29.4.6with diagnostics enabled and transpile-only unset already type-checked all 21 spec files against these exactcompilerOptions— just per-file, never as a whole program. That is the opposite ofsdk-analyticsbelow, wherebabel-jeststripped types and hid 14 errors. Same rollout, two projects, and the test transformer decided whether anything was checked at all.Production source is clean without escape hatches: 0
@ts-ignore/@ts-expect-error, 0 non-null assertions, and 2anys that are the same exported declaration (DynamicComponentEntity = Promise<Type<any>>,lib/models/index.ts:12).Type<any>is idiomatic Angular for dynamically-loaded components and the type is public API, so narrowing it is a separate change, not strict-mode work. 0 internal dependents.CLAUDE.mdnow documents theTS6053masking variant next to the existingTS2688one. Two of the fourteen projects triaged so far were masked this way — #35944 viaTS2688, #35947 viaTS6053— so error counts from the remaining projects should not be trusted until their tsconfigs are checked for this.data-access(#35948)First non-isolated project in the rollout: 27 direct dependents, 6 of them already strict.
All six flags had been in
libs/data-access/tsconfig.jsonfor some time, and they were completely inert. The project has nobuildtarget, so its own tsconfig is never read by anything, and its 27 dependents compile these sources under their own non-strict configs. So 36 errors sat in a layer-3 shared services hub with CI fully green — matching the 106 → 68 → 36 drift measured incidentally in thedotcms-jsandutilssections above.tsconfig.lib.jsontsconfig.spec.jsonProduction source (36)
paginator.service.ts(14) — 8 uninitialised fields given zero values; four header reads take?? ''(identicalNaNoutcome);private setLinks(linksString: string)widened tostring | nullsince its body already didlinksString?.split(',') || []; the file-localinterface Linksgained an index signature because theLink-header parser stores whateverrelthe server sends.dot-page-state.service.ts(12) — six declarations widened because they were simply wrong; the service really does emitnull. The interesting one ishandleSetPageStateFailed, declaredObservable<DotHttpErrorHandled>but ending inmap(() => undefined). Because it genuinely emitsundefined, the caller's= [null, null]destructuring default is reachable and load-bearing, not dead code. Declaring the honest type made the wholeswitchMaptypable; it now destructures explicitly instead of fighting an annotation.if (page)becameif (page && user)— whichforkJoinalready guaranteed.dot-router(5),dot-localstorage(3),dot-content-types-info(2) — nullable getters (previousUrl,storedRedirectUrl),localStoragereads, and a string index narrowed tokeyof.Specs (47)
25 came from three lines. The fake
Routerdeclarednavigate = jest.fn(() => ...), which infers zero parameters, so everytoHaveBeenCalledWith(...)was aTS2554.Two of the rest were real bugs hiding behind disabled suites:
dot-global-message.service.spec.tsimportedDotMessageServicefromdot-alert-confirm.service, which does not export it. The suite isxdescribed, so it never ran.dot-ai.service.ts— a production file — didexport { DotAiProviderConfig }on a type, invalid underisolatedModules. Only the spec config sets that flag, so only it surfaced the error.Also:
dot-page-layout.service.spec.tswas callingsave(id, mockDotLayout()), butsavetakes aDotTemplateDesignerand posts it verbatim — the spec was testing a payload shape production never sends (edit-ema-layout.component.ts:111sends the real one). And thedot-content-drivefixture used anoffsetfield removed fromDotContentDriveSearchRequest, copied from the model's own stale JSDoc example, which is fixed here too.dot-personasnow reuses the existingmockDotPersonafrom@dotcms/utils-testinginstead of hand-rolling 21 fields.No new
any, no@ts-ignore/@ts-expect-erroranywhere in the diff.Blast radius — measured, not assumed
Every strict dependent was counted before and after. Zero new errors, 218 removed, and three went fully clean because they were carrying nothing but this library's leakage:
global-storeportlets-dot-analytics-data-accessportlets-dot-experiments-data-accessimage-editorportlets-dot-analyticsportlets-dot-locales-portletutils-testingThis is the "high leverage" the issue predicted, quantified.
A repo-wide finding:
testnever type-checks specsdata-accessusesjest-preset-angular→ts-jest@29.4.6againsttsconfig.spec.json, which looks like it type-checks. It does not, because that tsconfig setsisolatedModules: true:ts-jest/.../config/config-set.js:229reads TypeScript'sisolatedModulesinto ts-jest's own flag.ts-jest/.../compiler/ts-compiler.js:74builds the language-service host onlyif (!isolatedModules)._doTypeChecking()needs that host forgetSemanticDiagnostics.data-accessis the proof: 84tscerrors alongside 754 passing tests. Sincecore-web/CLAUDE.mdmandatesisolatedModules: truein everytsconfig.spec.json, no project'stesttarget type-checks its specs anywhere in this monorepo — so "tests pass" has never been evidence of spec type-cleanliness. This corrects the justification given in thesdk-angularsection above (that verdict was separately confirmed withtsc -p, so it stands). Removing the flag would enable checking monorepo-wide and is left to the epic.Test plan
dotcms-jspnpm exec tsc -p libs/dotcms-js/tsconfig.lib.json --noEmit— 0 errors (from 38)data-access,global-store,portlets-dot-analytics,portlets-dot-analytics-data-access,portlets-dot-locales-portlet,utils-testingdata-accesstypecheck: 106 → 68 errors, zero newdotcms-uitypecheck clean (one pre-existing unrelated error)dotcms-jslint went from 42 to 41 problems (still tag-excluded)sdk-create-apptsc --noEmitclean on lib and specnx run sdk-create-app:build / :lint / :testgreennode dist/libs/sdk/create-app/index.js --helpworksDEBUGfix makesnx run sdk-create-app:buildfail with TS4111 — confirming the build gate is realsdk-analyticstsc --noEmitclean ontsconfig.lib.json(from 18) andtsconfig.spec.json(from 29)nx run sdk-analytics:typecheck / :lint / :build / :build:standalonegreennx run sdk-analytics:test— 15 suites, 314 tests passed. Since jest never type-checked these specs, this was the real regression check on the fixture editsnx run sdk-analytics:build— this project's build is not a gatesdk-angulartsc -p tsconfig.spec.json --noEmitnow completes a semantic pass at all (previouslyTS6053), 0 own errorstsc -p tsconfig.lib.json --noEmit0 own errorsnx run sdk-angular:lintclean;:buildgreennx run sdk-angular:testunchanged at 21 suites / 234 tests — the guard that no file dropped out of the programnx run sdk-angular:build(TS2322, exit 1) — ngtsc gates this projectdata-accesstsc -p tsconfig.lib.json --noEmit36 → 0;tsc -p tsconfig.spec.json --noEmit84 → 0nx run data-access:lintclean;:testunchanged at 79 suites / 754 testsdotcms-ui820 tests andui2184 tests passnx affected -t buildgreen for all 6 affected projectsBoth
pnpm exec nx format:check --base=origin/maingreenany/@ts-ignore/@ts-expect-error(verified by diff grep)Correction: a verification false negative (review follow-up)
A review comment caught a real regression this PR introduced, and the reason it slipped through matters for how the numbers above should be read.
libs/utils-testing/tsconfig.lib.jsondeclares"types": ["jasmine"], and that package is not installed.tsctherefore emitsTS2688: Cannot find type definition file for 'jasmine'and stops before semantic checking. Sotsc -p libs/utils-testing/tsconfig.lib.json --noEmitreports exactly one error no matter what the code does.The
utilssection originally reported "utils-testingunchanged at 1 pre-existing error" as evidence of no regression. That measurement proved nothing — nothing was being type-checked. Running the same config with--types nodereveals 33 errors, including a genuineTS2741caused by retypingEMPTY_SYSTEM_FIELDtoOmit<DotCMSContentTypeField, 'clazz'>: the mock atdot-content-types.mock.ts:71spreads it and never suppliesclazz.Fixed by giving the mock
clazz: DotCMSClazzes.TEXT; that config is now at 32 errors, all pre-existing and unrelated.Because the mock has ~103 consumers whose tests do run in CI, the runtime-value change was verified rather than assumed —
clazzwentnull(pre-PR) → absent (this PR) →TEXT:FieldUtil.isRow/isColumn/isTabDividercompare for equality and returnfalsefor all three values.!field.clazzorfield.clazz === nullanywhere in the repo.default-value-property7/7;dot-content-types-edit545 passed across 48 suites;data-access751 passed across 79 suites.The
data-accessfigures reported elsewhere in this PR (106 → 68 fordotcms-js, 68 → 36 forutils) are not affected — that project has no unresolvedtypesentry, so those runs were doing real semantic checking.core-web/CLAUDE.mdnow documents this masking behaviour so the next person does not repeat it.Other two comments
sdk-create-app— the throw said "requires at least 1 retry", butretriesis the total attempt count (for (i = 0; i < retries; i++)), soretries = 1is one attempt and zero retries. Reworded to "attempt".CLAUDE.mdverify snippet — hard-codedlibs/<project>/tsconfig.lib.json, which resolves for neither nested projects (libs/sdk/create-app, which has notsconfig.lib.json) nor apps (tsconfig.app.json). Replaced with a<projectRoot>placeholder and both caveats.Notes for reviewers
Three sibling issues in this rollout turned out not to need the work as written, and were resolved separately:
dotcms) and [04/44] Enable TS strict mode in dot-layout-grid #35937 (dot-layout-grid) — closed as not applicable. Both are dead libraries with zero consumers that do not compile today; removal is tracked in Remove dead core-web libraries (libs/dotcms, libs/dot-layout-grid) #36950.typescript-strict-plugin/tsc-strictapproach that was dropped — the bootstrap [00] Setup typescript-strict-plugin baseline + CI gate #35933 closed without the plugin ever landing. Sub-issues still referencingnpx tsc-strictor// @ts-strict-ignorecarry stale acceptance criteria; [06/44] Enable TS strict mode in dotcms-js #35939's were corrected on the issue.Closes #35948
Closes #35947
Closes #35946
Closes #35945
Closes #35944
Closes #35940
Closes #35939
Closes #35938
Closes #35935