refactor(core-web): enable TS strict mode in utils, dotcms-js and sdk-create-app + document the rollout - #36957
refactor(core-web): enable TS strict mode in utils, dotcms-js and sdk-create-app + document the rollout#36957nicobytes wants to merge 13 commits into
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 4m 23s —— View job ReviewI reviewed the full diff against New IssuesNone. Resolved
Notes (non-blocking, verified safe)
Nice touch documenting the · |
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>
Two factual corrections to the specs for #35941 and #35942, and one observation, all surfaced by a follow-up review. 1. Published version was wrong. Both specs quoted the version from the local `package.json` (`@dotcms/uve` 1.1.1, `@dotcms/client` 1.2.0) as if that were what ships. It is not: the SDK release action rewrites the version to the dotCMS release tag under ADR-0019 date lockstep. npm `latest` for both is 26.8.7-1 (197 and 262 published versions respectively). Both specs now say so explicitly, and the corresponding GitHub issue comments have been edited. 2. `sdk-client` has 6 dependents, not 4, and 5 of them are strict rather than 3. The Nx graph query used for the original count missed `sdk-experiments` and `sdk-create-app`. The lone non-strict consumer is `portlets-edit-ema-portlet`, which reaches into `@dotcms/client/internal`. 3. New observation, out of scope for the rollout: `build:js` in both `sdk-client` and `sdk-uve` emits an artifact that is committed to git (`html/js/editor-js/sdk-editor.js` and `ext/uve/dot-uve.js`), but that target is invoked by neither `core-web/pom.xml` nor any workflow. If the source changes and nobody runs it by hand, the committed file drifts out of sync and nothing notices. The verdicts for both issues are unchanged — both projects remain already strict-compliant and enforced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
Four steps of the strict-mode rollout (epic #35932):
utilsstrict+ 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.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 realBoth
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 #35940
Closes #35939
Closes #35938
Closes #35935