Experiment - Enable TypeScript strict mode#3640
Draft
tegan-temporal wants to merge 27 commits into
Draft
Conversation
Set up incremental strict-mode adoption: - tsconfig.strict.json is now a ratchet (strict:true with noImplicitAny and strictNullChecks still disabled) so remaining flags are gated. - Add check:strict script. Fix the 32 residual errors (useUnknownInCatchVariables + strictFunctionTypes): - request-from-api: type handleError option as ErrorCallback. - paginated callbacks: type token param as NextPageToken. - narrow unknown catch vars before accessing .message / passing as Error. - widen contravariant component handler params (Event, value unions). Note: 2 pre-existing errors remain in event-link.ts (link.workflow does not exist on ILink) — these also fail the base `pnpm check` and are out of scope for strict mode.
Add explicit types across utilities, models, holocene, components, services, stores, and vendor code so noImplicitAny passes: - proper domain types for test fixtures and callback params - keyof/index-signature typing for dynamic property access - Svelte 5 typed props, event params, and snippet params - ambient module declaration for the untyped oidc-provider dev dep Enable noImplicitAny in the strict ratchet (tsconfig.strict.json). Remaining: strictNullChecks (still disabled) and the 2 pre-existing event-link.ts errors (tracked separately).
The `if (link.workflow)` block referenced a `workflow` field that does not exist on the proto ILink type (which only has workflowEvent, batchJob, activity, nexusOperation). It was hallucinated draft code from #3620 ("WIP unreviewed changes driven by codex"); the real workflow-link case is already handled by the link.workflowEvent branch above. This block was unreachable and failed `pnpm check`.
The noImplicitAny pass annotated extracted play/focus consts with StoryContext<ComponentProps<...>>, which is narrower than the context the play prop provides and failed under strictFunctionTypes. Type the reusable play consts structurally (canvasElement + step) so they are a valid supertype of the play context; inline the date-picker focus helper. Ratchet (strict:true, strictNullChecks:false) is now clean.
Handle null/undefined across the codebase so strictNullChecks passes,
fixing at use sites with guards, optional chaining, and nullish
defaults rather than non-null assertions (only a handful of `!` in
tests/provably-safe spots). Notable:
- models: proto (all-optional) reads normalized to domain types.
- services/utilities: documented REST base64 vs proto Uint8Array casts
only at genuine boundaries; local Replace<> types for paginated token.
- WorkflowExecution.pendingWorkflowTask/defaultWorkflowTaskTimeout made
optional (they are genuinely absent at runtime) instead of defaulting
to {} — preserves falsy behavior for `{#if}` guards.
- getValueForFirstKey returns undefined (not '') to match behavior.
- remove obsolete event-link workflow-variant tests (dead branch).
Flip strictNullChecks on in the strict ratchet (tsconfig.strict.json is
now full `strict: true`). Full unit suite passes; base check stays clean.
Flip `strict: true` in tsconfig.json so `pnpm check` (already run in CI via lint-and-test.yml) enforces the full strict flag set. All prior phases cleared the errors, so this is a clean flip. Remove the interim check:strict script; tsconfig.strict.json is retained for the Danger count-strict-errors tracker (now reporting 0, serving as a guard).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
# Conflicts: # src/lib/components/search-attributes-form/search-attributes-form-content.svelte
tegan-temporal
changed the title
tegan-temporal
changed the title
The Danger strict tracker uses standard svelte-check (svelte2tsx + tsc); `pnpm check` uses --tsgo, which didn't flag these. Reconcile Svelte-4 $$Props (extends HTML*Attributes, so props are nullable) with the narrower export-let declarations: - button: target accepts `string | null | undefined` - accordion: `export let icon` typed IconName (was implicitly `null`) - checkbox: override id/disabled to non-null in $$Props - radio-input: override disabled to non-null in RadioInputProps Both standard svelte-check and tsgo now report 0.
range-input: make min/max/id required (component produces NaN without min/max; all callers already pass them); type step as number | undefined. checkbox: type group as T[] | undefined to match $$Props and the internal group !== undefined guards. Removes the 'undefined as unknown as X' casts.
Remove comments that restate the adjacent cast (structurally-identical zod/domain types, search-attribute shapes, Memo narrowing) and shorten the two verbose schedule-spec blocks. Keep only comments stating external facts a reader can't derive from the code: base64-vs-Uint8Array wire encoding, opaque-JSON REST boundary, the null filter sentinel, the single-attribute-key invariant, requestFromAPI's undefined contract, and the OIDC .d.ts shim.
# Conflicts: # src/lib/services/standalone-nexus-operations.ts
Now that strict mode is fully enabled and the codebase is strict-clean,
the Danger strict-mode check fails (was: warned) when a changed file
introduces a TypeScript error under the strict config. Reframe the
message from migration-tracking ("% of total") to enforcement. Infra
failures (script/parse errors) stay as warnings so a tooling hiccup
doesn't block all PRs.
The check-types CI job (`pnpm check`, strict tsconfig) continues to
hard-block on tsgo-detected errors; Danger (standard compiler) covers
the errors tsgo under-reports.
svelte-check's machine-verbose output uses 0-indexed LSP positions. count-strict-errors passed them straight through, so Danger's inline annotations landed one line above the actual error, and the lines-in-diff gate (built from git's 1-indexed hunk headers) compared against 0-indexed lines. Convert start line/character to 1-indexed at the source. Verified with a probe: an error on line 3 now reports 3.
Drop 10 unused imports flagged by eslint no-unused-vars across the touched areas (page, Badge, pauseLiveUpdates, Payload, base, afterEach, PotentiallyDecodable, UserMetadata, isRawPayloads, SearchAttributeTypeOption). Also remove two unused `const namespace` literals in event-history test files edited by this branch. Left pre-existing unused locals in files this change didn't touch (and the load `parent()` call, which has a side effect) alone. Type-checks (tsgo + standard) and full test suite stay green.
tegan-temporal
commented
Jul 8, 2026
|
|
||
| <!-- Info Alert --> | ||
| <Alert intent="info" class="text-sm"> | ||
| <Icon name="info" slot="icon" /> |
Contributor
Author
There was a problem hiding this comment.
Alert does not have an icon slot... this wasn't being rendered to begin with. It just became noticeable due to the strictness change
An earlier commit removed the `if (link.workflow)` handler believing it was dead/hallucinated code. That was wrong: `temporal.api.common.v1.ILink` has a real `workflow` oneof variant (Link.IWorkflow: namespace, workflowId, runId, reason) — see @temporalio/proto@1.19.0 root.d.ts. The removal was made while local node_modules were stale at proto 1.17.2 (where the field didn't yet exist), so it type-checked but silently regressed: events carrying link.workflow fell through to unknownLink() and rendered as non-clickable "Link" text instead of a Workflow ID link. Restore the branch and its two covering tests. Verified: strict check (tsgo + standard) clean, event-link tests pass against proto 1.19.0.
- activity-options-update-drawer: default backoffCoefficient to 2 (server default) instead of 0 — Temporal rejects < 1, and the full field mask always submits it. (finding 2) - getFirstWholeNumberUnit: add overloads so passing a defaultUnit returns a non-undefined label; narrow initialTimeoutUnit to DefaultUnits and drop 4 now-dead `?? 'second(s)'` fallbacks. (finding 4) - dangerfile: match changed files by normalized path equality instead of fragile bidirectional substring (missed regressions / false-blocked suffix collisions). (finding 5) - timeline-graph-row: guard divide-by-zero so an undefined workflowDistance yields 0, not Infinity, as an SVG coordinate. (finding 7) - rename MostRecentWOrkflowVersionStamp -> MostRecentWorkflowVersionStamp typo (now load-bearing imported surface). (finding 9) Deferred: #3 (requestFromAPI -> Promise<T|undefined> is a 128-call-site refactor, its own PR), #6 (documented zod input/output boundary), #8 (pre-existing on main, not this change).
On the notifyOnError path requestFromAPI resolves without a body, so the
declared Promise<T> was a lie (previously papered over with
`undefined as unknown as T`). Type it as Promise<T | undefined> and
return undefined honestly.
Handle the surfaced undefined at the service boundary (19 files in
src/lib/services/) so each exported service keeps its existing return
contract and the change doesn't cascade into component callers: list
responses default to `[]`, object responses to `{}` / typed empties,
etc. Happy-path behavior is unchanged (fallbacks only trigger on the
handled-error path, which previously crashed callers on property
access). Four internal helpers (query-service fetchQuery,
workflow-activities requestWithActivityFallback, the two poll* fns) were
widened to `| undefined` instead of defaulted — their callers already
guard undefined, and defaulting poll* would have broken the long-poll
empty-response skip.
getFormSpecInitialData parsed partial seeds and cast the result to FormSpecSchema, claiming a fully-formed spec it did not produce. Split the bare object out of the refined schema (formSpecObject) and parse seeds through it so zod injects the field defaults, yielding a genuine FormSpecSchema with no cast. This matches the shape submit-time validation already produces, so final data is unchanged.
Every caller now passes a default, so the no-default behaviors (returning undefined on no match, falling back to the last unit on zero) were dead. Requiring defaultUnit collapses the three overload signatures into one that returns a plain UnitLabelT, and the schedule policy drawer passes 'second(s)' directly instead of coalescing the result (behavior-identical, since seconds was already the last unit).
getFormSpecFromSpec built partial input specs and cast the array to FormSpecSchema[]. Parse each spec through the bare object schema (parseFormSpec) so zod injects the field defaults, yielding a genuine FormSpecSchema with no cast. Because parsing now fills the sibling field (a frozen interval spec gains a default calendar), realign the frozen type label to distinguish calendar vs interval by the interval value rather than calendar presence, matching how summarize.ts already discriminates.
backoffCoefficient/maximumAttempts bind to number inputs, whose DOM value is a string, so typing them as string throughout matches reality and the RetryPolicyInput props. The request builder already coerces via Number(...) guarded by truthiness, so the form no longer double-converts and the StandaloneActivityFormData cast is gone. Adds coverage for the builder's string->number coercion and empty-field omission.
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
1 participant
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 & motivation 💭
Enables TypeScript
strict: truein the basetsconfig.json, sopnpm check(already run in CI vialint-and-test.yml) now enforces the full strict flag set. The repo previously built withstrict: false.Rolled out as an incremental ratchet — one flag group at a time, each driven to zero before enabling the next (~930 errors total):
useUnknownInCatchVariables,strictFunctionTypes, …noImplicitAnyplaytypingstrictNullChecks?./ nullish defaults at use sitesstrict: truein base tsconfigFixes were made at use sites (guards, optional chaining, nullish defaults) rather than
!assertions — only a handful of!in tests / provably-safe spots. Documentedas unknown asonly at genuine boundaries (REST base64stringvs protoUint8Array; zod input/output). No exported signatures widened except a few deliberate, verified cases. Later commits fold in code-review feedback (tighter types instead of casts, prop types that were non-null but defaultedundefined, dead-code and comment cleanup).tsconfig.strict.json+scripts/count-strict-errors.ts(the Danger tracker) are retained and now report 0 — a regression guard.Testing 🧪
How was this tested 👻
pnpm check(tsgo, the CI gate) → 0 errorssvelte-checkagainsttsconfig.strict.json(standard compiler, what the Danger strict tracker uses) → 0 errors — the two compilers diverge slightly, so both are verifiedUnit (
vitest) → 2226 passing, 0 failing (behavior verified, not just types — e.g. a real truthiness regression was caught by a snapshot and fixed in code)Integration (Playwright) → 258 passing, 1 skipped
E2E (Playwright) → 28 passing, 5 skipped, 1 flaky (
schedules.specempty-list → create-form nav; passed on retry)Manual testing — not done
E2E / integration suites run (green)
Unit tests added / updated
Reviewer notes / risk 🔎
Most of the diff is type-only, but
strictNullCheckstouched runtime in places. Hotspots worth a closer look:workflow-execution.ts,simplify-attributes.ts) —null→undefined/''.WorkflowExecution.pendingWorkflowTask/defaultWorkflowTaskTimeoutwere made optional (genuinely absent at runtime) instead of defaulting to{}, which would have flipped{#if}truthiness.bind:valuegetter/setter rewrites (spec-type-interval, …). (Note: the one flaky e2e is on the schedules page — passed on retry, but worth a glance.){#if}guards in templates/snippets (pending activity/task heartbeat,workflow-nexus-links).persisted-search-parameter.ts— minor change to what's persisted (fixes a latent bug).Out of scope / follow-up
This PR does not address the repo's pre-existing lint warnings (~168 svelte-check + ~118 eslint, largely
state_referenced_locally,require-each-key, unused vars). They exist onmaintoday and are best handled in a separate cleanup PR to keep this one focused on the strict flip.Checklists
Merge Checklist
main(up to date)Issue(s) closed