From 14c92e40c08d0c7cd6f29a83af1537c009813927 Mon Sep 17 00:00:00 2001 From: "P.Conrad" Date: Sun, 12 Apr 2026 17:47:39 +0000 Subject: [PATCH 01/10] feat: Add TypeScript code review skill - Created a TypeScript code review skill document outlining the review process, core categories, and output format. - Introduced two comprehensive plan for addressing TypeScript code review findings in the TKO monorepo. - Updated tsconfig.json to exclude new directories for skills and plans. --- AGENTS.md | 1 + plans/typescript-code-review-findings-2.md | 409 +++++++++++++++++ plans/typescript-code-review-findings.md | 166 +++++++ skills/typescript-code-review/SKILL.md | 136 ++++++ .../references/review-reference.md | 422 ++++++++++++++++++ .../references/tko-conventions.md | 172 +++++++ tsconfig.json | 4 +- 7 files changed, 1309 insertions(+), 1 deletion(-) create mode 100644 plans/typescript-code-review-findings-2.md create mode 100644 plans/typescript-code-review-findings.md create mode 100644 skills/typescript-code-review/SKILL.md create mode 100644 skills/typescript-code-review/references/review-reference.md create mode 100644 skills/typescript-code-review/references/tko-conventions.md diff --git a/AGENTS.md b/AGENTS.md index f4816ebe9..2cc285939 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -192,6 +192,7 @@ self-contained folder with a `SKILL.md` and optional supporting assets | Skill | Purpose | |-------|---------| | `plan-creation` | Scaffold a `plans/` file with the correct template, classify risk per `AI_COMPLIANCE.md`, and enforce approval gates | +| `typescript-code-review` | Perform comprehensive TypeScript code reviews covering type safety, security, performance, and code quality with actionable feedback | Skills are loaded on-demand when the agent detects a matching task. diff --git a/plans/typescript-code-review-findings-2.md b/plans/typescript-code-review-findings-2.md new file mode 100644 index 000000000..d08fad5b7 --- /dev/null +++ b/plans/typescript-code-review-findings-2.md @@ -0,0 +1,409 @@ +# Plan: TypeScript Code Review — Findings (Round 2) + +**Risk class:** `MEDIUM` + +**Status:** Draft + +## Summary + +A comprehensive TypeScript code review of the full TKO monorepo — 25 packages, +2 builds, and tools — applied the `typescript-code-review` skill against all +production source. The review identified **5 critical bugs**, **15 important +improvements**, and **10 suggestions**. The codebase compiles cleanly (`tsc` +zero errors, `eslint` zero errors) and has strong modular architecture. + +The most severe findings are: + +1. **Proxy `deleteProperty` trap receives wrong argument** — all property + deletions on computed proxies silently corrupt the wrong key +2. **Parser operator precedence inverts JS semantics** — bitwise operators + parse above relational/equality, causing incorrect expression evaluation +3. **`??` (nullish coalescing) behaves identically to `||`** — treats `0`, + `''`, and `false` as nullish +4. **`TextInputLegacyFirefox` override is dead code** — Firefox + autocomplete/drag-drop events are never registered +5. **`style` binding references global `jQuery` instead of `options.jQuery`** + +Additional systemic issues: 17 loose-equality (`==`) comparisons, 4 deprecated +`.substr()` calls, 39 unresolved `FIXME`/`TODO` comments, and deprecated DOM +APIs with extraneous arguments in production source. + +## Goals + +- Fix all 5 confirmed bugs +- Replace all deprecated API usage (`.substr()`, `createEvent`/`initEvent`) +- Fix loose-equality comparisons in production source +- Clean up confirmed dead code + +## Non-Goals + +- Rewriting all `any` types at once (incremental approach preferred) +- Changing runtime behavior or public API surface beyond bug fixes +- Modifying `tools/build.mk` or `tools/karma.conf.js` (shared infra — needs + separate HIGH-risk plan) +- Adding new runtime dependencies +- Re-enabling disabled ESLint rules (separate effort) +- DON'T resolve or triage all FIXME/TODO annotations + +## Current State + +### Baselines + +- `make tsc` — zero errors +- `make eslint` — zero errors +- Loose-equality (`==`) in production source: **17 occurrences** +- Deprecated `.substr()` in production source: **4 occurrences** +- Unresolved `FIXME`/`TODO`/`HACK`/`XXX` in production source: **39 occurrences** + +### Overlap with Existing Plan + +The previous plan (`typescript-code-review-findings.md`) identified some of the +same issues (`.substr()` in `attr.ts`, `AttributeMustacheProvider` bug, FIXMEs). +This plan provides a comprehensive superset with verified new critical findings. +The previous plan's Phase 1 quick wins (steps 1–3) should be subsumed by this +plan's Phase 1. + +--- + +## Detailed Findings + +### Critical Issues 🔴 + +#### 1. Proxy `deleteProperty` trap receives wrong argument +**File**: `packages/computed/src/proxy.ts:57–60` +- **Issue**: The `deleteProperty` trap declares only one parameter `property`, + but the Proxy spec requires `(target, property)`. The first argument is the + target object, not the property name. All deletions on computed proxies + silently fail or corrupt the wrong key. +- **Current**: + ```ts + deleteProperty(property) { + delete mirror[property as any] + return delete object[property as any] + } + ``` +- **Recommended**: + ```ts + deleteProperty(_target, property) { + delete mirror[property as any] + return delete object[property as any] + } + ``` + +#### 2. Parser operator precedence inverts JS semantics for bitwise operators +**File**: `packages/utils.parser/src/operators.ts:172–192` +- **Issue**: Bitwise operators `|`(12), `^`(11), `&`(10) have higher precedence + than relational (11) and equality (10) operators — the exact inverse of + JavaScript. Expression `a < b | c` parses as `a < (b | c)` instead of + `(a < b) | c`. `^` collides with `<` at 11; `&` collides with `===` at 10. +- **Current**: + ```ts + operators['|'].precedence = 12 + operators['^'].precedence = 11 + operators['&'].precedence = 10 + operators['<'].precedence = 11 + operators['==='].precedence = 10 + ``` +- **Recommended** (match JS/MDN precedence): + ```ts + operators['<'].precedence = 12 // relational + operators['<='].precedence = 12 + operators['>'].precedence = 12 + operators['>='].precedence = 12 + operators['=='].precedence = 11 // equality + operators['!='].precedence = 11 + operators['==='].precedence = 11 + operators['!=='].precedence = 11 + operators['&'].precedence = 10 // bitwise AND + operators['^'].precedence = 9 // bitwise XOR + operators['|'].precedence = 8 // bitwise OR + ``` + +#### 3. `??` nullish coalescing behaves identically to `||` +**File**: `packages/utils.parser/src/operators.ts:199` +- **Issue**: `earlyOut` for `??` is `a => a`, which returns falsy for `0`, `''`, + and `false`. The RHS is evaluated unnecessarily and `??` becomes a duplicate + of `||`. If the RHS has side effects, they fire incorrectly. +- **Current**: `operators['??'].earlyOut = a => a` +- **Recommended**: `operators['??'].earlyOut = a => a !== null && a !== undefined` + +#### 4. `TextInputLegacyFirefox` overrides non-existent method (dead code) +**File**: `packages/binding.core/src/textInput.ts:132–143` +- **Issue**: Overrides `eventsIndicatingValueChange()`, but the parent class + `TextInput` calls `eventsIndicatingSyncValueChange()` and + `eventsIndicatingDeferValueChange()` — never `eventsIndicatingValueChange()`. + The Firefox-specific `DOMAutoComplete`, `dragdrop`, `drop` events are never + registered. +- **Current**: `eventsIndicatingValueChange(): string[] {` +- **Recommended**: `override eventsIndicatingSyncValueChange(): string[] {` + +#### 5. `style` binding references global `jQuery` instead of `options.jQuery` +**File**: `packages/binding.core/src/style.ts:16–17` +- **Issue**: Guards with `options.jQuery` but calls bare global `jQuery(element)`. + In module environments where jQuery is not a global, this throws + `ReferenceError`. +- **Current**: `if (options.jQuery) { jQuery(element).css(styleName, styleValue) }` +- **Recommended**: `if (options.jQuery) { options.jQuery(element).css(styleName, styleValue) }` + +--- + +### Important Improvements 🟡 + +#### 6. `subscribable.when()` — subscription leak on unsatisfied condition +**File**: `packages/observable/src/subscribable.ts:191–197` +- **Issue**: Promise never rejects. If the test condition is never satisfied, the + subscription lives forever and the Promise never settles — a memory leak. +- **Recommended**: Add a disposal mechanism (e.g., accept an `AbortSignal`, or + reject when the observable is disposed). + +#### 7. Duplicate `bindingKey` in error message +**File**: `packages/bind/src/applyBindings.ts:510–516` +- **Issue**: Error message includes `spec.bindingKey` twice, producing messages + like `Unable to process binding "text" in binding "text"`. +- **Recommended**: Remove the duplicated segment or replace the second with the + binding expression text. + +#### 8. `value.isInput()` type guard checks wrong element +**File**: `packages/binding.core/src/value.ts:53–54` +- **Issue**: Type guard narrows the `element` parameter but checks + `this.$element` instead. Works by coincidence since callers pass + `this.$element`. +- **Recommended**: `return tagNameLower(element) === 'input'` + +#### 9. Deprecated `createEvent`/`initEvent` with extraneous arguments +**File**: `packages/utils/src/dom/event.ts:83–96` +- **Issue**: Uses deprecated `document.createEvent()` and `initEvent()`. + `initEvent` accepts 3 arguments but 15 are passed (remnant from + `initMouseEvent` signature) — the extra 12 are silently ignored. +- **Recommended**: Replace with `new Event(eventType, { bubbles: true, cancelable: true })`. + +#### 10. Deprecated `.substr()` usage (4 occurrences) + +| File | Line | +|------|------| +| `packages/binding.core/src/attr.ts` | 15 | +| `packages/utils.parser/src/preparse.ts` | 94 | +| `packages/filter.punches/src/index.ts` | 54 | +| `packages/filter.punches/src/index.ts` | 57 | + +- **Recommended**: Replace with `.substring()` (identical semantics for + non-negative indices). + +#### 11. Duplicate import alias in computed +**File**: `packages/computed/src/computed.ts:12–15` +- **Issue**: `options` is imported both directly and as `options as koOptions`. + Dead alias creates confusion. +- **Recommended**: Remove one import; use a single name consistently. + +#### 12. Loose equality (`==`) instead of strict (`===`) — 17 occurrences +Across `packages/observable`, `packages/binding.core`, `packages/utils`, +`packages/binding.template`, `packages/filter.punches`. Most compare strings +or numbers where `===` is both safer and idiomatic. + +#### 13. Dead code: `dataStore` variable in utils +**File**: `packages/utils/src/dom/data.ts:8` +- **Issue**: `const dataStore = {}` declared but never referenced. Leftover from + prior implementation. +- **Recommended**: Remove. + +#### 14. Deprecated `clonePlainObjectDeep` still exported +**File**: `packages/utils/src/object.ts:60–77` +- **Issue**: Annotated `@deprecated Function is unused` but still exported. +- **Recommended**: Remove after confirming no consumers via `knip`. + +#### 15. `AttributeMustacheProvider.getBindingAccessors` returns `false` +**File**: `packages/provider.mustache/src/AttributeMustacheProvider.ts:99–100` +- **Issue**: Base `Provider.getBindingAccessors` returns an object. This override + returns `false` for non-Element nodes — a type-contract violation. +- **Recommended**: Return `Object.create(null)` for consistency. + +#### 16. `var` re-declaration shadows parameter in Parser Node +**File**: `packages/utils.parser/src/Node.ts:55–56` +- **Issue**: `var node: Node = this` re-declares the `node` parameter via `var` + hoisting, discarding it. Confusing and fragile. +- **Recommended**: Rename the parameter to `_node` and use `const node: Node = this`. + +#### 17. `Parser` imported as value but only used as type cast +**File**: `packages/provider.component/src/ComponentProvider.ts:11` +- **Issue**: `Parser` is imported as a value, then cast with `as any` to call + `new (Parser as any)(...)`. The `as any` hides constructor type errors. +- **Recommended**: Fix the `Parser` constructor signature to accept the correct + arguments, removing the need for `as any`. + +#### 18. JsxObserver subscription ignores callback argument +**File**: `packages/utils.jsx/src/JsxObserver.ts:369` +- **Issue**: Subscription callback receives `attr` (new value) but re-passes + `value` (the observable) to `setNodeAttribute`. Works because + `setNodeAttribute` calls `unwrap`, but the callback arg is wasted. +- **Current**: `value.subscribe(attr => this.setNodeAttribute(node, name, value))` +- **Recommended**: `value.subscribe(attr => this.setNodeAttribute(node, name, attr))` + +#### 19. NativeProvider redundant null-guard +**File**: `packages/provider.native/src/NativeProvider.ts:23–26` +- **Issue**: `|| {}` fallback is dead code — early return already handles falsy. +- **Recommended**: Remove the `|| {}`. + +#### 20. `repackage.mjs` swallows write errors silently +**File**: `tools/repackage.mjs:48–49` +- **Issue**: `.catch(console.error)` logs but exits 0 on failure. CI won't + catch broken repackaging. +- **Recommended**: `await` the write and let rejections propagate. + +--- + +### Suggestions 🔵 + +#### 21. Unused `SubscriptionCallback` import +**File**: `packages/bind/src/bindingEvent.ts:3` + +#### 22. Deprecated `event.returnValue = false` fallback +**File**: `packages/binding.core/src/submit.ts:17` +- Legacy IE property; `preventDefault()` is already called in the preceding branch. + +#### 23. `readElseChain` returns `false` where object expected +**File**: `packages/binding.if/src/else.ts:36` +- Returns `false` but callers access `.elseChainSatisfied`. Works by accident. + +#### 24. Redundant `nodeType` check after `instanceof Element` +**File**: `packages/provider.component/src/ComponentProvider.ts:55` + +#### 25. AMD require call lacks error callback +**File**: `packages/utils.component/src/loaders.ts:277` + +#### 26. `repackage.mjs` relative path fragility +**File**: `tools/repackage.mjs:7` +- `../../lerna.json` assumes CWD depth. Derive from `import.meta.url`. + +--- + +### Positive Observations ✅ + +- **Zero `tsc` errors** — the full production source compiles cleanly +- **Zero `eslint` errors** — linting passes across all packages +- **Consistent architecture** — factory-function-as-constructor, Symbol keys, + centralized error handling via `options.onError` are used uniformly +- **Proper `import type`** — the vast majority of type-only imports correctly + use `import type` as required by `verbatimModuleSyntax` +- **Strong modularity** — 25 packages with clear boundaries, barrel exports, + and zero runtime dependencies +- **LifeCycle disposal pattern** — modern binding handlers consistently use + `LifeCycle.anchorTo()` for automatic subscription cleanup +- **Well-structured tests** — Mocha/Chai/Sinon with ~89% statement coverage + +--- + +## Steps + +### Phase 1: Critical Bug Fixes (HIGH priority, behavior-changing) + +1. **Fix Proxy `deleteProperty` trap** — Add missing `_target` parameter in + `packages/computed/src/proxy.ts:57`. Verify with proxy-related tests in + `packages/computed/spec/`. + +2. **Fix parser operator precedence** — Reorder precedence values for bitwise, + relational, and equality operators in + `packages/utils.parser/src/operators.ts:172–192`. Must match JS semantics + exactly. Run full parser test suite. + +3. **Fix `??` earlyOut semantics** — Change `a => a` to + `a => a !== null && a !== undefined` in + `packages/utils.parser/src/operators.ts:199`. Verify `??` correctly + preserves `0`, `''`, `false`. + +4. **Fix `TextInputLegacyFirefox` override** — Rename + `eventsIndicatingValueChange()` to `eventsIndicatingSyncValueChange()` in + `packages/binding.core/src/textInput.ts:132`. + +5. **Fix `style` binding jQuery reference** — Change `jQuery(element)` to + `options.jQuery(element)` in `packages/binding.core/src/style.ts:17`. + +### Phase 2: Important Fixes (MEDIUM priority, no behavior change unless noted) + +6. **Fix `value.isInput()` type guard** — Change `this.$element` to `element` + in `packages/binding.core/src/value.ts:54`. + +7. **Fix duplicate error message** — Remove duplicated `spec.bindingKey` + segment in `packages/bind/src/applyBindings.ts:510–516`. + +8. **Replace deprecated `.substr()`** — 4 files (see finding #10). + +9. **Replace deprecated `createEvent`/`initEvent`** — Use `new Event()` in + `packages/utils/src/dom/event.ts:83–96`. + +10. **Fix loose equality** — Replace 17 `==`/`!=` with `===`/`!==` across + 6 packages (see finding #12). + +11. **Remove dead code** — `dataStore` in `utils/dom/data.ts`, + `clonePlainObjectDeep` in `utils/object.ts`, duplicate `options` alias + in `computed.ts`. + +12. **Fix `AttributeMustacheProvider` return type** — Return `Object.create(null)` + instead of `false` in `provider.mustache/src/AttributeMustacheProvider.ts:99`. + +13. **Fix JsxObserver subscription callback** — Pass `attr` instead of `value` + in `utils.jsx/src/JsxObserver.ts:369`. + +14. **Fix `repackage.mjs` error handling** — `await` the `writeFile` call + in `tools/repackage.mjs:48`. + +### Phase 3: Cleanup (LOW priority) + +15. **Triage TODO/FIXME annotations** — Review all 39 annotations; fix, convert + to tracking issues, or remove stale ones. + +16. **Apply remaining suggestions** — Findings #21–#26. + +--- + +## Verification + +- `make tsc` — zero errors (must remain green after each step) +- `make test-headless` — all tests pass after each step +- `make eslint` — no new errors introduced +- `make format` — formatting check passes + +### Per-Phase Test Strategy + +- **Phase 1**: Run full test suite (`make test-headless`). For findings #2/#3 + (parser), add targeted test cases for `a < b | c` and `x ?? 0` expressions. + For finding #1 (proxy), test `delete proxy.key`. +- **Phase 2**: Run package-specific tests for each changed package, then full + suite at phase end. +- **Phase 3**: No behavior change expected; full suite once at phase end. + +## AI Evidence + +- Risk class: `MEDIUM` — behavior-changing bug fixes in binding/parser/proxy + logic; no CI/CD, release, or shared tooling modification +- Changes and steps: See Steps section (3 phases, 16 steps) +- Tools/commands: `tsc`, `eslint`, subagent code review across all packages + and builds, manual source verification of all 10 highest-severity findings +- Validation: `tsc` and `eslint` pass with zero errors; all 10 critical/important + findings confirmed against actual source code at reported line numbers +- Follow-up owner: Maintainer review required before Phase 1 implementation + (behavior-changing fixes) + +# LATER TASK (Don't do this now) + +#### 1. Unresolved TODO: class refactoring in subscribable +**File**: `packages/observable/src/subscribable.ts:68` + +#### 2. Unresolved TODO: downcast in observable +**File**: `packages/observable/src/observable.ts:209` + +#### 3. Unresolved TODO: dangerous `this` in static method +**File**: `packages/bind/src/BindingHandler.ts:97` + +#### 4. Unresolved TODOs across multiple packages +**Files**: `packages/utils/src/array.ts:97`, +`packages/utils/src/dom/html.ts:59,146`, +`packages/utils/src/dom/selectExtensions.ts:43`, +`packages/binding.template/src/templating.ts:40`, +`packages/binding.template/src/templateEngine.ts:63`, +`packages/binding.foreach/src/foreach.ts:581`, +`packages/binding.if/src/ConditionalBindingHandler.ts:11`, +`packages/provider/src/Provider.ts:59`, +`packages/utils.parser/src/operators.ts:82–84`, +`builds/reference/src/common.ts:5` +- **Total**: 39 FIXME/TODO/HACK/XXX annotations in production source. +- **Recommended**: Triage all: fix, convert to issues, or remove if stale. diff --git a/plans/typescript-code-review-findings.md b/plans/typescript-code-review-findings.md new file mode 100644 index 000000000..7fdfee8fc --- /dev/null +++ b/plans/typescript-code-review-findings.md @@ -0,0 +1,166 @@ +# Plan: TypeScript Code Review — Address Findings + +**Risk class:** `MEDIUM` + +**Status:** Draft + +## Summary + +A comprehensive TypeScript code review of the full TKO monorepo (25+ packages, +2 builds, tools) identified 6 critical issues, 8 important improvements, and +7 suggestions. The codebase compiles cleanly (`tsc` zero errors on production +source) and has solid modular architecture, but pervasive `any` usage — enabled +by globally disabled ESLint rules — undermines TypeScript's value. One confirmed +bug (wrong binding handler lookup) was found, along with deprecated API usage, +untyped DOM parameters, and unresolved FIXMEs in production code. + +## Goals + +- Replace deprecated `.substr()` with `.substring()` / `.slice()` +- Gradually re-enable disabled ESLint type-safety rules +- Improve type annotations in the highest-impact locations (Parser, Provider, + build render functions) +- Harden tooling scripts with proper error handling + +## Non-Goals + +- Fix the confirmed bug in `AttributeMustacheProvider.getPossibleDirectBinding` +- Resolve or document all FIXME annotations in production source +- Rewriting all `any` types at once (too large; incremental approach preferred) +- Changing runtime behavior or public API surface +- Modifying `tools/build.mk` or `tools/karma.conf.js` (shared infrastructure — needs separate HIGH-risk plan) +- Adding new runtime dependencies + +## Current State + +### TypeScript Compiler + +- `tsc` passes with zero errors on production source +- Skill example files (`skills/typescript-code-review/examples/`) cause `tsc` + failures — they are not excluded from tsconfig + + +### Confirmed Bug (Github-Issue #235) + +`packages/provider.mustache/src/AttributeMustacheProvider.ts` line 83: + +```typescript +getPossibleDirectBinding(attrName: string) { + const bindingName = this.ATTRIBUTES_BINDING_MAP[attrName] + return bindingName && this.bindingHandlers.get(attrName) //FIXME this.bindingHandlers.get(bindingName) ? +} +``` + +Looks up binding handler by `attrName` instead of `bindingName`. The FIXME +comment confirms this is a known issue. + +### Unresolved FIXMEs (5 occurrences) + +| File | Line | Description | +|------|------|-------------| +| `packages/provider.mustache/src/AttributeMustacheProvider.ts` | 83 | Wrong binding handler lookup key | +| `packages/provider.attr/src/AttributeProvider.ts` | 45 | Duplicates `Identifier.prototype.lookup_value` | +| `packages/binding.foreach/src/foreach.ts` | 74 | Expensive `cloneNode` — consider iterating | +| `packages/binding.if/src/ifUnless.ts` | 37 | `needsRefresh` condition incomplete | +| `packages/utils.parser/src/preparse.ts` | (charCodes) | Magic numbers instead of named constants | + +## Steps + +### Phase 1: Quick Wins (LOW risk, no behavior change) + +1. **Replace `.substr()` with `.substring()`** in `packages/binding.core/src/attr.ts:15`. + +2. **Replace magic number** `9007199254740991` with `Number.MAX_SAFE_INTEGER` + in `packages/binding.foreach/src/foreach.ts:35`. + +3. **Exclude skill examples from tsc** — Add `skills` to the `exclude` array + in `tsconfig.json` so `make tsc` stays green. + +### Phase 2: Type Improvements (MEDIUM risk) + +9. **Type the Parser core** — Change `ch: any` → `ch: string`, + `at: any` → `at: number`, `text: any` → `text: string` in + `packages/utils.parser/src/Parser.ts`. + +10. **Add `LegacyProvider` constructor types** — Type `providerObject` and + `parentProvider` parameters in `packages/provider/src/Provider.ts:116`. + +11. **Type `jsx` parameter** in build render functions — Replace `any` with + proper JSX element type in `builds/reference/src/index.ts` and + `builds/knockout/src/index.ts`. + +12. **Define `ComponentDefinition` interface** — Replace `any` params in + `packages/binding.component/src/componentBinding.ts`. + +13. **Add named charCode constants** in `packages/utils.parser/src/preparse.ts`. + +### Phase 3: Robustness (LOW risk) + +14. **Add error handling to tooling scripts** — Wrap `JSON.parse` in + try-catch in `tools/release-version.cjs`; make `writeFile` failures + fatal in `tools/repackage.mjs`. + +15. **Fix DOM mutation during iteration** — Collect attributes to remove + in `AttributeMustacheProvider.bindingObjects` before yielding, then + remove after iteration. + +16. **Resolve remaining FIXMEs** — Investigate each, fix or document + retention reason with a tracking issue number. + +## Verification + +- `make tsc` — zero errors (currently passes; must remain green) +- `make test-headless` — all tests pass after each step +- `make eslint` — no new errors introduced; warning count tracked +- `make format` — formatting check passes +- Manual verification of `AttributeMustacheProvider` bug fix via + mustache provider test suite + +## AI Evidence + +- Risk class: `MEDIUM` — behavior changes in binding/provider logic; + no CI/CD, release, or shared tooling modification (Phase 2+ ESLint + changes are config-only) +- Changes and steps: See Steps section above (4 phases, 16 steps) +- Tools/commands: `tsc`, `eslint`, `grep`, subagent code review across + all packages, manual source verification +- Validation: `tsc` passes with zero production errors; all findings + verified against actual source code +- Follow-up owner: Maintainer review required before Phase 2+ + + +# Later Steps (NOT in this task) + +## ESLint Configuration - Current State + +The following type-safety rules are globally disabled in `eslint.config.js`: + +``` +@typescript-eslint/no-explicit-any: off +@typescript-eslint/no-unused-vars: off +@typescript-eslint/no-unsafe-function-type: off +prefer-const: off +prefer-spread: off +no-useless-escape: off +``` + +The entire `builds/` directory is excluded from linting. + +### DON'T DO THIS NOW: ESLint Rule Re-enablement (MEDIUM risk) + +1. **Fix confirmed bug** — Change `this.bindingHandlers.get(attrName)` to + `this.bindingHandlers.get(bindingName)` in `AttributeMustacheProvider.ts:83`. + Verify with existing tests (`make test-headless` in `packages/provider.mustache`). + +2. **Re-enable `prefer-const` as `warn`** — Run `make eslint` to assess + violation count; auto-fix with `make eslint-fix`. + +3. **Re-enable `@typescript-eslint/no-unused-vars` as `warn`** with + `argsIgnorePattern: '^_'` — identify dead code across all packages. + +4. **Include `builds/` in ESLint scope** — Remove `builds/**/*` from + `ignores` in `eslint.config.js`. Fix any violations found. + +5. **Re-enable `no-explicit-any` as `warn`** per-package, starting with + smaller packages (`lifecycle`, `filter.punches`, `builder`). Track + violation count reduction over time. \ No newline at end of file diff --git a/skills/typescript-code-review/SKILL.md b/skills/typescript-code-review/SKILL.md new file mode 100644 index 000000000..69a12b99f --- /dev/null +++ b/skills/typescript-code-review/SKILL.md @@ -0,0 +1,136 @@ +--- +name: typescript-code-review +description: Perform comprehensive code reviews for LLM-based linting of TypeScript projects. Analyzing type safety, best practices, clientside security and code quality. Result are actionable and ranked findings. +--- + +# TypeScript Code Review Skill + +## Review Process + +### 1. Initial Assessment +- Understand purpose, scope, TS version, and `tsconfig.json` settings +- Check for relevant docs, comments, and existing patterns + +### 2. Core Review Categories + +#### Type Safety +- Verify `strict: true` and adherence; flag implicit `any` +- Type guards, narrowing, and exhaustiveness checking for unions +- Flag unnecessary type assertions (`as`, `!`); prefer `satisfies` or `instanceof` +- Proper `?.` and `??` usage; explicit return types on functions +- Generics constrained appropriately; discriminated unions with `type` field + +#### Code Quality +- **Naming**: camelCase vars/fns, PascalCase types/classes; `is`/`has` for booleans; `get` for sync accessors, `fetch` for async IO; `is` for type predicates +- **Functions**: max ~50 lines, max 3 params (use options object for more), no boolean flag params, prefer pure functions +- **Constants**: no magic numbers/strings; use named constants +- **Equality**: `===`/`!==` only (no `==`) +- **Errors**: throw `Error` objects only; handle promise rejections +- **Immutability**: prefer `const`, spread over mutation +- **Enums**: prefer union types or `as const` lookup tables (enums emit runtime code) +- **Defaults**: use ES6 parameter defaults, destructuring, spread + +#### Code Organization +- Prefer `function` declarations at file scope (hoisting, stack traces); arrows for callbacks +- Guard clauses / early returns over deep nesting (max 3-4 levels) +- Import order: external libs → workspace packages → relative → type-only +- `import type` mandatory with `verbatimModuleSyntax` +- File order: constants → types → setup → functions → exports +- `interface` for object shapes; `type` for unions/intersections/mapped types +- String unions for fixed value sets + +#### Modern TS Features +- `?.`, `??`, template literal types, utility types (`Partial`, `Pick`, `Omit`, `Record`) +- `as const` for literal types and lookup tables; `satisfies` for validation +- Type predicates for custom guards + +#### Security +- Validate/sanitize user input at system boundaries; no `innerHTML` with untrusted data; no `eval` +- No hardcoded secrets, tokens, or API keys — use environment variables +- Strip sensitive fields before exposing data: `Omit` +- Don't log sensitive data (passwords, tokens, PII) +- Types as security layer: union types for allowed values, type guards at boundaries +- `npm audit` dependencies; minimize count; review new packages before adding + +#### Testing & Maintainability +- Note missing tests for critical paths; flag circular dependencies +- Use `import type` for type-only imports; verify JSDoc on public APIs + +### 3. Output Format + +```markdown +## Summary +[Overview: quality, main concerns, highlights] + +## Critical Issues 🔴 +[Must fix: type errors, security vulns, bugs] + +## Important Improvements 🟡 +[Maintainability, performance, anti-patterns] + +## Suggestions 🔵 +[Nice-to-have: style, optimizations] + +## Positive Observations ✅ +[Good patterns to reinforce] + +## Detailed Findings +### [Category] +**File**: `path/file.ts:line` +- **Issue**: [Description] +- **Current**: `[code]` +- **Recommended**: `[improved code]` +- **Why**: [Reasoning] +``` + +### 4. Guidelines +- Constructive, specific, with code examples. Severity: 🔴 critical → 🟡 important → 🔵 suggestion +- Priority: security/bugs → anti-patterns → style +- Respect existing patterns; note tradeoffs; reference tsconfig + +### 5. References + +Grep references for specific topics: +- `references/review-reference.md` — type safety checklist, anti-patterns with bad/good examples +- `references/tko-conventions.md` — TKO monorepo-specific patterns and overrides + +### 6. tsconfig Review + +Recommended strict settings: `strict`, `noUncheckedIndexedAccess`, `noImplicitOverride`, +`noPropertyAccessFromIndexSignature`, `exactOptionalPropertyTypes`, +`noFallthroughCasesInSwitch`, `noImplicitReturns`, `noUnusedLocals`, `noUnusedParameters`. + +### 7. TKO Monorepo Conventions + +When reviewing TKO code, apply these overrides: + +**Architecture** (do NOT flag): +- Factory-function-as-constructor with `Object.setPrototypeOf` / `.fn` prototypes — core pattern +- Module-level side effects (prototype wiring, Symbol polyfills) — essential +- Pervasive `any` — accepted tech debt (`no-explicit-any: off`, `noImplicitAny: false`). Flag only in net-new code +- `prefer-const` is off — `let` for never-reassigned vars is fine +- No enums, no `readonly`, minimal `as const` in existing code + +**DO flag**: +- Missing `import type` (mandatory: `verbatimModuleSyntax: true`) +- Missing DOM disposal (`disposeWhenNodeIsRemoved`, `addDisposeCallback`, `LifeCycle.anchorTo()`) +- Violations of centralized error handling (`options.onError` pattern) + +**Testing**: Mocha/Chai/Sinon (not Jasmine); Karma + Electron; tests in `packages/*/spec/`. +**Zero runtime deps**: never suggest external packages for core `@tko/*` packages. + +See `references/tko-conventions.md` for full details. + +### 8. Review Workflow + +1. Scan critical issues (type errors, security, bugs) +2. Review architecture (modules, boundaries, separation of concerns) +3. Deep-dive logic (correctness, edge cases, error handling) +4. Check types (accuracy, safety, TS feature usage) +5. Style/consistency (naming, formatting, patterns) +6. Testing/docs (coverage, clarity) + +## When to Use + +Activate when user asks for code review, feedback on TS implementation, +checking for issues/bugs, or ensuring best practices. diff --git a/skills/typescript-code-review/references/review-reference.md b/skills/typescript-code-review/references/review-reference.md new file mode 100644 index 000000000..3dba05eb9 --- /dev/null +++ b/skills/typescript-code-review/references/review-reference.md @@ -0,0 +1,422 @@ +# TypeScript Review Reference + +Combined type-safety checklist and anti-pattern catalog. Grep for specific topics. + +--- + +## tsconfig — Strict Settings + +```json +{ + "strict": true, "noUncheckedIndexedAccess": true, "noImplicitOverride": true, + "noPropertyAccessFromIndexSignature": true, "exactOptionalPropertyTypes": true, + "noFallthroughCasesInSwitch": true, "noImplicitReturns": true, + "noUnusedLocals": true, "noUnusedParameters": true +} +``` + +--- + +## Type Annotations & `any` + +### ❌ `any` as escape hatch → `unknown` + type guard +```typescript +// Bad +function process(data: any) { return data.value * 2 } +// Good +function process(data: unknown): number { + if (isData(data)) return data.value * 2 + throw new Error('Invalid data') +} +function isData(d: unknown): d is { value: number } { + return typeof d === 'object' && d !== null && 'value' in d + && typeof (d as any).value === 'number' +} +``` + +### ❌ Excessive type assertions → validate with guards +```typescript +// Bad +const user = data as User +// Good +function isUser(d: unknown): d is User { + return typeof d === 'object' && d !== null && 'name' in d +} +if (isUser(data)) { /* data is User */ } +``` + +### ❌ Over-specifying inferred types +```typescript +// Bad: const name: string = 'Alice' +// Good: const name = 'Alice' — still annotate function params/returns +``` + +### ✅ Explicit return types on all functions +```typescript +// Bad +function total(items: Item[]) { return items.reduce((s, i) => s + i.price, 0) } +// Good +function total(items: Item[]): number { return items.reduce((s, i) => s + i.price, 0) } +``` + +--- + +## Null / Undefined Handling + +### ✅ Optional chaining (`?.`) over manual null chains +```typescript +// Bad: const street = user && user.address && user.address.street +// Good: const street = user?.address?.street +``` + +### ✅ Nullish coalescing (`??`) over logical OR +```typescript +// Bad: const name = user.name || 'Guest' — replaces '', 0, false +// Good: const name = user.name ?? 'Guest' — only null/undefined +``` + +### ❌ Non-null assertion (`!`) → handle the null case +```typescript +// Bad: const el = document.getElementById('x')! +// Good +const el = document.getElementById('x') +if (!el) throw new Error('Element not found') +``` + +--- + +## Enums, Unions & Literal Types + +### ❌ Regular enums → union types or `as const` lookup tables +```typescript +// Bad: enum Status { Pending, Approved } — emits runtime code +// Good — union type +type Status = 'pending' | 'approved' +// Good — lookup table (provides both values and types) +const Status = { Pending: 'PENDING', Approved: 'APPROVED' } as const +type Status = typeof Status[keyof typeof Status] +``` + +### ❌ Missing discriminated union → add `type` field +```typescript +// Bad: type Result = { data: string } | { error: string } +// Good +type Result = + | { type: 'ok'; data: string } + | { type: 'err'; error: string } +``` + +### ✅ Exhaustiveness checking in switch +```typescript +function handle(r: Result): string { + switch (r.type) { + case 'ok': return r.data + case 'err': throw new Error(r.error) + default: const _: never = r; throw new Error('Unhandled') + } +} +``` + +### ✅ String unions for fixed value sets +```typescript +// Bad: function setTheme(theme: string) {} +// Good: function setTheme(theme: 'light' | 'dark' | 'system') {} +``` + +### ❌ Missing `as const` for literal config +```typescript +// Bad — loses literal types +const CONFIG = { apiUrl: 'https://api.example.com', timeout: 5000 } +// Good — preserves literal types, readonly +const CONFIG = { apiUrl: 'https://api.example.com', timeout: 5000 } as const +``` + +--- + +## Generics + +### ✅ Constrain generics +```typescript +// Bad: function get(obj: T, key: string) { return obj[key] } +// Good +function get, K extends keyof T>(obj: T, key: K): T[K] { + return obj[key] +} +``` + +### ✅ Use generic defaults +```typescript +interface Response { data: T; status: number } +``` + +--- + +## Object Types & Utility Types + +### ✅ `interface` for object shapes; `type` for unions/intersections +```typescript +// Good — interface for objects +interface User { id: string; name: string; email: string } +// Good — type for unions and mapped types +type Status = 'active' | 'inactive' +type WithTimestamp = T & { createdAt: Date } +``` + +### ✅ Use utility types: `Pick`, `Omit`, `Partial`, `Required`, `Readonly`, `Record` +```typescript +type PublicUser = Omit +type UserUpdate = Partial> +``` + +--- + +## Arrays & Tuples + +### ✅ Type arrays explicitly +```typescript +// Bad: const items = [] — inferred as any[] +// Good: const items: Item[] = [] +``` + +### ✅ Tuples for fixed-length +```typescript +type Point = [x: number, y: number] +``` + +### ✅ Safe indexing with `noUncheckedIndexedAccess` +```typescript +const first = items[0] // Type: Item | undefined — handle the undefined +if (first !== undefined) { use(first) } +``` + +--- + +## Assertions & Narrowing + +### ❌ `as T` assertions → use `instanceof`, `in`, or custom type guards +```typescript +// Bad: const el = document.getElementById('x') as HTMLInputElement +// Good +const el = document.getElementById('x') +if (el instanceof HTMLInputElement) { el.value = 'text' } +``` + +### ✅ `satisfies` (TS 4.9+) — validates structure, preserves literal types +```typescript +const config = { + apiUrl: 'https://api.example.com', + timeout: 5000 +} satisfies Record +// config.apiUrl is 'https://api.example.com', not string +``` + +--- + +## Functions + +### ❌ Too many parameters → options object +```typescript +// Bad: function create(id: string, name: string, email: string, age: number) {} +// Good +interface CreateParams { id: string; name: string; email: string; age: number } +function create(params: CreateParams) {} +``` + +### ❌ Boolean flag parameters → separate functions +```typescript +// Bad: function getUsers(includeInactive: boolean) {} +// Good +function getAllUsers() {} +function getActiveUsers() {} +``` + +### ❌ Arrow functions at file level → function declarations +```typescript +// Bad: const process = (items: Item[]) => items.map(transform) +// Good: function process(items: Item[]) { return items.map(transform) } +// Why: hoisting + better stack traces; arrows for callbacks only +``` + +### ❌ Manual undefined checks → default parameters +```typescript +// Bad: const t = timeout !== undefined ? timeout : 5000 +// Good: function fn(timeout = 5000) {} +``` + +### ❌ Optional before required params +```typescript +// Bad: function create(name?: string, id: string) {} +// Good: function create(id: string, name?: string) {} +``` + +--- + +## Arrays & Objects — Immutability + +### ❌ Mutating arrays → spread +```typescript +// Bad: items.push(newItem); return items +// Good: return [...items, newItem] +``` + +### ❌ `forEach` for transforms → `map`/`filter`/`reduce` +```typescript +// Bad +const names: string[] = [] +users.forEach(u => names.push(u.name)) +// Good +const names = users.map(u => u.name) +``` + +### ❌ `Object.assign` mutation → spread +```typescript +// Bad: Object.assign(user, updates); return user +// Good: return { ...user, ...updates } +``` + +### ❌ `delete` operator → destructuring rest +```typescript +// Bad: delete result.password; return result +// Good: const { password, ...rest } = result; return rest +``` + +--- + +## Classes + +### ❌ Classes for plain data → interface + factory +```typescript +// Bad +class User { constructor(public id: string, public name: string) {} } +// Good — when there's no behavior +interface User { id: string; name: string } +function createUser(id: string, name: string): User { return { id, name } } +``` + +### ✅ Parameter properties for classes with behavior +```typescript +class Service { + constructor(private readonly logger: Logger, private config: Config) {} +} +``` + +--- + +## Imports / Exports + +### ❌ Missing `import type` (build error with `verbatimModuleSyntax`) +```typescript +// Bad: import { User } from './types' — if User is type-only +// Good: import type { User } from './types' +``` + +### ❌ Barrel exports with side effects → import from specific modules +```typescript +// Bad: import { oneFunction } from './modules' — loads all re-exports +// Good: import { oneFunction } from './modules/specific' +``` + +### ❌ Circular dependencies → extract shared code to third module + +--- + +## Error Handling + +### ❌ Catch without typing +```typescript +// Bad: catch (e) { console.log(e.message) } +// Good +catch (error) { + if (error instanceof Error) console.log(error.message) + else console.log('Unknown error') +} +``` + +### ❌ Throwing strings → always `throw new Error(msg)` +```typescript +// Bad: throw 'User not found' +// Good: throw new Error('User not found') +``` + +--- + +## Structure & Design + +### ❌ Deep nesting → guard clauses / early returns +```typescript +// Bad +if (user) { if (active) { if (email) { send(email) } } } +// Good +if (!user) return 'No user' +if (!active) return 'Inactive' +if (!email) return 'No email' +send(email) +``` + +### ❌ `==` → always `===` +```typescript +// Bad: if (count == '0') {} — type coercion: 0 == '' is true +// Good: if (count === 0) {} +``` + +### ❌ Unencapsulated mutable globals → wrap in closure or class +```typescript +// Bad +let current: User | null = null +export function set(u: User) { current = u } +// Good +function createStore() { + let current: User | null = null + return { set(u: User) { current = u }, get() { return current } } +} +export const userStore = createStore() +``` + +--- + +## Testing + +### ❌ `any` in tests → use proper types for mocks +```typescript +// Bad: const mock: any = { name: 'Alice' } +// Good: const mock: User = { id: '1', name: 'Alice', email: 'a@b.com' } +``` + +### ✅ `@ts-expect-error` to verify type rejections +```typescript +// @ts-expect-error — should not accept number +createUser(123) +``` + +--- + +## Review Checklist + +### Type Safety +- [ ] Explicit return types on functions +- [ ] No implicit `any`; `unknown` + guards for unknown data +- [ ] `?.` and `??` for null handling; no `!` assertions +- [ ] Type guards for union narrowing; exhaustiveness in switches +- [ ] Constrained generics; generic defaults where appropriate +- [ ] Safe array indexing (`noUncheckedIndexedAccess`) +- [ ] `satisfies` over `as` assertions where possible +- [ ] `as const` for literal/lookup types; no regular enums +- [ ] `interface` for objects, `type` for unions/mapped +- [ ] `import type` for type-only imports +- [ ] String unions for fixed value sets +- [ ] Bool vars prefixed `is`/`has`/`can`/`should` + +### Code Quality +- [ ] Function declarations at file scope; arrows for callbacks only +- [ ] Max ~50 lines / 3 params per function; options object for more +- [ ] No boolean flag params; no magic numbers/strings +- [ ] `===` only; `Error` objects only; no unhandled rejections +- [ ] Immutable ops (spread, not mutation); `const` default +- [ ] Guard clauses over deep nesting +- [ ] No circular dependencies; proper import grouping + +### Security +- [ ] External input validated at boundaries; no `innerHTML` with untrusted data +- [ ] No `eval`; no hardcoded secrets +- [ ] Sensitive fields stripped; no PII in logs +- [ ] Dependencies audited and minimal diff --git a/skills/typescript-code-review/references/tko-conventions.md b/skills/typescript-code-review/references/tko-conventions.md new file mode 100644 index 000000000..55af2f2ed --- /dev/null +++ b/skills/typescript-code-review/references/tko-conventions.md @@ -0,0 +1,172 @@ +# TKO Monorepo Conventions + +Review guidelines specific to the TKO (Technical Knockout) monorepo. TKO is a +TypeScript MVVM framework for data binding and templating — the next generation +of Knockout.js. + +## Core Architecture + +### Factory-Function-as-Constructor Pattern + +TKO's core primitives (`observable`, `computed`, `subscribable`) use factory +functions that return callable function-objects, not class instances. Prototypes +are wired up via `Object.setPrototypeOf`. This is currently acceptable for reasons of +compatibility with Knockout. + +```typescript +// This is idiomatic TKO — NOT an anti-pattern +export function observable(initialValue) { + function obs() { /* ... */ } + Object.setPrototypeOf(obs, observable.fn) + obs[LATEST_VALUE] = initialValue + return obs +} + +observable.fn = { /* shared methods */ } +Object.setPrototypeOf(observable.fn, subscribable.fn) +``` + +**Do NOT flag**: `Object.setPrototypeOf`, `.fn` prototype objects, factory functions +returning function-objects. + +### Module-Level Side Effects + +Some modules intentionally execute code at import time: +- Prototype chain wiring (`Object.setPrototypeOf` calls) +- `Symbol.observable` polyfill in subscribable +- Prototype extension via `.fn` objects + +**Do NOT flag** these as "avoid side effects." They are essential architecture. + +### Symbol-Based Unique Keys + +TKO uses `Symbol()` and `createSymbolOrString()` for unique keys on objects +rather than string literals or enums: + +```typescript +const LATEST_VALUE = Symbol('LatestValue') +const computedState = Symbol('ComputedState') +const SUBSCRIBABLE_SYM = Symbol('Subscribable') +``` + +This is the preferred discriminator pattern. + +--- + +## TypeScript Configuration + +Key settings to be aware of: + +| Setting | Value | Implication | +|---------|-------|-------------| +| `strict` | `true` | Strict mode enabled | +| `verbatimModuleSyntax` | `true` | `import type` is mandatory for type-only imports | +| `noEmit` | `true` | esbuild handles compilation; `tsc` only type-checks | +| `target` | `ES2020` | ES2020 output target | + + +### `import type` Enforcement + +`verbatimModuleSyntax: true` means type-only imports **must** use `import type`. +Cross-package type imports always use this form: + +```typescript +// Correct +import type { Observable } from '@tko/observable' + +// Wrong — will cause build errors +import { Observable } from '@tko/observable' // if Observable is type-only +``` + +--- + +## DOM & Reactive Patterns + +### Disposal is Critical + +Every subscription or computed tied to a DOM node must have disposal wired up. +Missing disposal causes memory leaks. + +```typescript +// LifeCycle-based binding — disposal is automatic via anchorTo() +class myBinding extends BindingHandler { + constructor(params) { + super(params) + this.computed(() => { + // auto-disposes when DOM node is removed + }) + } +} + +// Manual subscription — must wire up disposal +const sub = someObservable.subscribe(callback) +addDisposeCallback(element, () => sub.dispose()) +``` + +**Always check**: Is there a `disposeWhenNodeIsRemoved`, `addDisposeCallback`, or +`LifeCycle.anchorTo()` for every subscription created in a binding handler? + +### Centralized Error Handling + +Errors flow through `options.onError` — a global error handler. Functions are +wrapped with `catchFunctionErrors()`. Per-site `try/catch` is used sparingly +and only in critical paths (computed evaluation, binding init). + +Do not suggest adding try/catch to every function. Respect the delegation pattern. + +### Binding Handler Styles + +Both patterns are valid in the codebase: + +```typescript +// Modern: class-based +class value extends BindingHandler { + constructor(params) { super(params) } + get controlsDescendants() { return false } +} + +// Legacy: plain object with init/update +const textInput = { + init(element, valueAccessor) { /* ... */ }, + update(element, valueAccessor) { /* ... */ } +} +``` + +--- + +## Naming Conventions + +| Category | Convention | Examples | +|----------|-----------|----------| +| Files (general) | camelCase | `observable.ts`, `bindingContext.ts` | +| Files (classes) | PascalCase | `BindingHandler.ts`, `LifeCycle.ts` | +| Variables/functions | camelCase | `arrayForEach`, `registerDependency` | +| Classes | PascalCase | `BindingHandler`, `Subscription` | +| Interfaces/Types | PascalCase (no `I` prefix) | `Observable`, `ComputedOptions` | +| Binding handlers | lowercase matching binding name | `value`, `textInput`, `css` | +| Symbols/constants | UPPER_SNAKE_CASE | `LATEST_VALUE`, `DISPOSED_STATE` | +| Private members | underscore prefix | `_subscriptions`, `_isDisposed` | + +--- + +## Package Conventions + +- Each package: `src/`, `spec/`, `types/`, `dist/`, `index.ts`, `Makefile` +- Inter-package deps use `@tko/package-name` (npm workspaces) +- **Zero runtime dependencies** — never add external packages to core +- **Named exports** are strongly preferred (~281 inline vs ~29 default) +- **Barrel files** (`index.ts`) re-export via `export *` and `export type { ... }` + +## Testing + +- **New tests**: Mocha + Chai + Sinon (not Jasmine) +- **Runner**: Karma with Electron (default), Chrome Headless, Firefox Headless +- **Test location**: `packages/*/spec/` +- Coverage target: ~89% statements, ~83% branches + +## Code Style + +- Prettier: no semicolons, single quotes, trailing commas: none, 120 char width +- ESLint: typescript-eslint flat config +- 2-space indentation for JS/TS, tabs for Makefiles +- Run `make format-fix && make eslint-fix` before committing diff --git a/tsconfig.json b/tsconfig.json index cb89b7326..d549e08bd 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -42,6 +42,8 @@ "builds", "docs", "tools", - "tko.io" + "tko.io", + "skills", + "plans" ] } From 39716a64380e823f050cdd90fbce3948df7a4e7b Mon Sep 17 00:00:00 2001 From: phillipc Date: Mon, 20 Apr 2026 22:18:08 +0200 Subject: [PATCH 02/10] chore: update testing and code style guidelines in tko-conventions.md --- .../typescript-code-review/references/tko-conventions.md | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/skills/typescript-code-review/references/tko-conventions.md b/skills/typescript-code-review/references/tko-conventions.md index 55af2f2ed..227f7b192 100644 --- a/skills/typescript-code-review/references/tko-conventions.md +++ b/skills/typescript-code-review/references/tko-conventions.md @@ -160,13 +160,10 @@ const textInput = { ## Testing - **New tests**: Mocha + Chai + Sinon (not Jasmine) -- **Runner**: Karma with Electron (default), Chrome Headless, Firefox Headless +- **Runner**: Vitest + Playwright (chromium/firefox/webkit) - **Test location**: `packages/*/spec/` - Coverage target: ~89% statements, ~83% branches ## Code Style -- Prettier: no semicolons, single quotes, trailing commas: none, 120 char width -- ESLint: typescript-eslint flat config -- 2-space indentation for JS/TS, tabs for Makefiles -- Run `make format-fix && make eslint-fix` before committing +- Run `bun run format` before committing From 9d3dca26e47ab2d010eb095bd9f80fb3e1f17dc1 Mon Sep 17 00:00:00 2001 From: phillipc Date: Mon, 20 Apr 2026 22:46:23 +0200 Subject: [PATCH 03/10] chore: update TypeScript code review findings and skill --- plans/typescript-code-review-findings-2.md | 55 +++++++++++++--------- plans/typescript-code-review-findings.md | 26 +++++++--- skills/typescript-code-review/SKILL.md | 2 +- 3 files changed, 53 insertions(+), 30 deletions(-) diff --git a/plans/typescript-code-review-findings-2.md b/plans/typescript-code-review-findings-2.md index d08fad5b7..75851370c 100644 --- a/plans/typescript-code-review-findings-2.md +++ b/plans/typescript-code-review-findings-2.md @@ -4,6 +4,12 @@ **Status:** Draft +## Status Update (2026-04-20) + +- Source basis: local `git log` + GitHub API (`knockout/tko`). +- Marked as done based on code + commits: Findings/Steps 1, 2, 5, 8, 9 (including `bdac39c2`, `3e7a37ae`, `5d6603d7`, `9659cf21`). +- Obsolete: references to `tools/repackage.mjs` (file no longer exists). + ## Summary A comprehensive TypeScript code review of the full TKO monorepo — 25 packages, @@ -15,17 +21,17 @@ zero errors, `eslint` zero errors) and has strong modular architecture. The most severe findings are: 1. **Proxy `deleteProperty` trap receives wrong argument** — all property - deletions on computed proxies silently corrupt the wrong key + deletions on computed proxies silently corrupt the wrong key ✅ **Done** 2. **Parser operator precedence inverts JS semantics** — bitwise operators - parse above relational/equality, causing incorrect expression evaluation + parse above relational/equality, causing incorrect expression evaluation ✅ **Done** 3. **`??` (nullish coalescing) behaves identically to `||`** — treats `0`, `''`, and `false` as nullish 4. **`TextInputLegacyFirefox` override is dead code** — Firefox autocomplete/drag-drop events are never registered -5. **`style` binding references global `jQuery` instead of `options.jQuery`** +5. **`style` binding references global `jQuery` instead of `options.jQuery`** ✅ **Done** Additional systemic issues: 17 loose-equality (`==`) comparisons, 4 deprecated -`.substr()` calls, 39 unresolved `FIXME`/`TODO` comments, and deprecated DOM +`.substr()` calls ✅ **Done**, 39 unresolved `FIXME`/`TODO` comments, and deprecated DOM APIs with extraneous arguments in production source. ## Goals @@ -52,7 +58,8 @@ APIs with extraneous arguments in production source. - `make tsc` — zero errors - `make eslint` — zero errors - Loose-equality (`==`) in production source: **17 occurrences** -- Deprecated `.substr()` in production source: **4 occurrences** +- ~~Deprecated `.substr()` in production source: **4 occurrences**~~ ✅ **Done** + (Current state: 0 matches in `packages/*/src` + `builds/*/src`) - Unresolved `FIXME`/`TODO`/`HACK`/`XXX` in production source: **39 occurrences** ### Overlap with Existing Plan @@ -69,7 +76,7 @@ plan's Phase 1. ### Critical Issues 🔴 -#### 1. Proxy `deleteProperty` trap receives wrong argument +#### 1. Proxy `deleteProperty` trap receives wrong argument ✅ Done **File**: `packages/computed/src/proxy.ts:57–60` - **Issue**: The `deleteProperty` trap declares only one parameter `property`, but the Proxy spec requires `(target, property)`. The first argument is the @@ -90,7 +97,7 @@ plan's Phase 1. } ``` -#### 2. Parser operator precedence inverts JS semantics for bitwise operators +#### 2. Parser operator precedence inverts JS semantics for bitwise operators ✅ Done **File**: `packages/utils.parser/src/operators.ts:172–192` - **Issue**: Bitwise operators `|`(12), `^`(11), `&`(10) have higher precedence than relational (11) and equality (10) operators — the exact inverse of @@ -137,7 +144,7 @@ plan's Phase 1. - **Current**: `eventsIndicatingValueChange(): string[] {` - **Recommended**: `override eventsIndicatingSyncValueChange(): string[] {` -#### 5. `style` binding references global `jQuery` instead of `options.jQuery` +#### 5. `style` binding references global `jQuery` instead of `options.jQuery` ✅ Done **File**: `packages/binding.core/src/style.ts:16–17` - **Issue**: Guards with `options.jQuery` but calls bare global `jQuery(element)`. In module environments where jQuery is not a global, this throws @@ -170,14 +177,14 @@ plan's Phase 1. `this.$element`. - **Recommended**: `return tagNameLower(element) === 'input'` -#### 9. Deprecated `createEvent`/`initEvent` with extraneous arguments +#### 9. Deprecated `createEvent`/`initEvent` with extraneous arguments ✅ Done **File**: `packages/utils/src/dom/event.ts:83–96` - **Issue**: Uses deprecated `document.createEvent()` and `initEvent()`. `initEvent` accepts 3 arguments but 15 are passed (remnant from `initMouseEvent` signature) — the extra 12 are silently ignored. - **Recommended**: Replace with `new Event(eventType, { bubbles: true, cancelable: true })`. -#### 10. Deprecated `.substr()` usage (4 occurrences) +#### 10. Deprecated `.substr()` usage (4 occurrences) ✅ Done | File | Line | |------|------| @@ -217,7 +224,7 @@ or numbers where `===` is both safer and idiomatic. returns `false` for non-Element nodes — a type-contract violation. - **Recommended**: Return `Object.create(null)` for consistency. -#### 16. `var` re-declaration shadows parameter in Parser Node +#### 16. `var` re-declaration shadows parameter in Parser Node ✅ Done **File**: `packages/utils.parser/src/Node.ts:55–56` - **Issue**: `var node: Node = this` re-declares the `node` parameter via `var` hoisting, discarding it. Confusing and fragile. @@ -243,7 +250,7 @@ or numbers where `===` is both safer and idiomatic. - **Issue**: `|| {}` fallback is dead code — early return already handles falsy. - **Recommended**: Remove the `|| {}`. -#### 20. `repackage.mjs` swallows write errors silently +#### 20. `repackage.mjs` swallows write errors silently ⚠️ Obsolete **File**: `tools/repackage.mjs:48–49` - **Issue**: `.catch(console.error)` logs but exits 0 on failure. CI won't catch broken repackaging. @@ -270,7 +277,7 @@ or numbers where `===` is both safer and idiomatic. #### 25. AMD require call lacks error callback **File**: `packages/utils.component/src/loaders.ts:277` -#### 26. `repackage.mjs` relative path fragility +#### 26. `repackage.mjs` relative path fragility ⚠️ Obsolete **File**: `tools/repackage.mjs:7` - `../../lerna.json` assumes CWD depth. Derive from `import.meta.url`. @@ -296,14 +303,14 @@ or numbers where `===` is both safer and idiomatic. ### Phase 1: Critical Bug Fixes (HIGH priority, behavior-changing) -1. **Fix Proxy `deleteProperty` trap** — Add missing `_target` parameter in +1. ✅ **Fix Proxy `deleteProperty` trap** — Add missing `_target` parameter in `packages/computed/src/proxy.ts:57`. Verify with proxy-related tests in - `packages/computed/spec/`. + `packages/computed/spec/`. Done via commit `bdac39c2`. -2. **Fix parser operator precedence** — Reorder precedence values for bitwise, +2. ✅ **Fix parser operator precedence** — Reorder precedence values for bitwise, relational, and equality operators in `packages/utils.parser/src/operators.ts:172–192`. Must match JS semantics - exactly. Run full parser test suite. + exactly. Run full parser test suite. Done via commit `3e7a37ae`. 3. **Fix `??` earlyOut semantics** — Change `a => a` to `a => a !== null && a !== undefined` in @@ -314,8 +321,9 @@ or numbers where `===` is both safer and idiomatic. `eventsIndicatingValueChange()` to `eventsIndicatingSyncValueChange()` in `packages/binding.core/src/textInput.ts:132`. -5. **Fix `style` binding jQuery reference** — Change `jQuery(element)` to +5. ✅ **Fix `style` binding jQuery reference** — Change `jQuery(element)` to `options.jQuery(element)` in `packages/binding.core/src/style.ts:17`. + Done via commit `5d6603d7`. ### Phase 2: Important Fixes (MEDIUM priority, no behavior change unless noted) @@ -325,10 +333,12 @@ or numbers where `===` is both safer and idiomatic. 7. **Fix duplicate error message** — Remove duplicated `spec.bindingKey` segment in `packages/bind/src/applyBindings.ts:510–516`. -8. **Replace deprecated `.substr()`** — 4 files (see finding #10). +8. ✅ **Replace deprecated `.substr()`** — 4 files (see finding #10). + Done via commit `9659cf21`. -9. **Replace deprecated `createEvent`/`initEvent`** — Use `new Event()` in +9. ✅ **Replace deprecated `createEvent`/`initEvent`** — Use `new Event()` in `packages/utils/src/dom/event.ts:83–96`. + Done (current code uses `new MouseEvent`/`new KeyboardEvent`/`new Event`). 10. **Fix loose equality** — Replace 17 `==`/`!=` with `===`/`!==` across 6 packages (see finding #12). @@ -343,8 +353,9 @@ or numbers where `===` is both safer and idiomatic. 13. **Fix JsxObserver subscription callback** — Pass `attr` instead of `value` in `utils.jsx/src/JsxObserver.ts:369`. -14. **Fix `repackage.mjs` error handling** — `await` the `writeFile` call - in `tools/repackage.mjs:48`. +14. ✅ **Fix `repackage.mjs` error handling** — `await` the `writeFile` call + in `tools/repackage.mjs:48`. ⚠️ **Obsolete/dropped**: `tools/repackage.mjs` + is not present in the current repo state. ### Phase 3: Cleanup (LOW priority) diff --git a/plans/typescript-code-review-findings.md b/plans/typescript-code-review-findings.md index 7fdfee8fc..d6f56bf49 100644 --- a/plans/typescript-code-review-findings.md +++ b/plans/typescript-code-review-findings.md @@ -4,6 +4,13 @@ **Status:** Draft +## Status Update (2026-04-20) + +- Source basis: local `git log` + GitHub API (`knockout/tko`), including + Issue [#235](https://github.com/knockout/tko/issues/235) (still `open`). +- Marked as done: Steps 1, 2, 3. +- Marked as obsolete: part of Step 14 (`tools/repackage.mjs` no longer exists). + ## Summary A comprehensive TypeScript code review of the full TKO monorepo (25+ packages, @@ -36,8 +43,9 @@ untyped DOM parameters, and unresolved FIXMEs in production code. ### TypeScript Compiler - `tsc` passes with zero errors on production source -- Skill example files (`skills/typescript-code-review/examples/`) cause `tsc` - failures — they are not excluded from tsconfig +- ~~Skill example files (`skills/typescript-code-review/examples/`) cause `tsc` + failures — they are not excluded from tsconfig~~ ✅ **Done** (`skills` is now + excluded in `tsconfig.json`) ### Confirmed Bug (Github-Issue #235) @@ -68,13 +76,16 @@ comment confirms this is a known issue. ### Phase 1: Quick Wins (LOW risk, no behavior change) -1. **Replace `.substr()` with `.substring()`** in `packages/binding.core/src/attr.ts:15`. +1. ✅ **Replace `.substr()` with `.substring()`** in `packages/binding.core/src/attr.ts:15`. + Done via commit `9659cf21`. -2. **Replace magic number** `9007199254740991` with `Number.MAX_SAFE_INTEGER` +2. ✅ **Replace magic number** `9007199254740991` with `Number.MAX_SAFE_INTEGER` in `packages/binding.foreach/src/foreach.ts:35`. + Done via commit `1b8a062f`. -3. **Exclude skill examples from tsc** — Add `skills` to the `exclude` array +3. ✅ **Exclude skill examples from tsc** — Add `skills` to the `exclude` array in `tsconfig.json` so `make tsc` stays green. + Done (see current `tsconfig.json`). ### Phase 2: Type Improvements (MEDIUM risk) @@ -98,7 +109,8 @@ comment confirms this is a known issue. 14. **Add error handling to tooling scripts** — Wrap `JSON.parse` in try-catch in `tools/release-version.cjs`; make `writeFile` failures - fatal in `tools/repackage.mjs`. + fatal in `tools/repackage.mjs`. ⚠️ **Partially obsolete**: `tools/repackage.mjs` + is no longer present in the current repo state. 15. **Fix DOM mutation during iteration** — Collect attributes to remove in `AttributeMustacheProvider.bindingObjects` before yielding, then @@ -163,4 +175,4 @@ The entire `builds/` directory is excluded from linting. 5. **Re-enable `no-explicit-any` as `warn`** per-package, starting with smaller packages (`lifecycle`, `filter.punches`, `builder`). Track - violation count reduction over time. \ No newline at end of file + violation count reduction over time. diff --git a/skills/typescript-code-review/SKILL.md b/skills/typescript-code-review/SKILL.md index 69a12b99f..8f1749187 100644 --- a/skills/typescript-code-review/SKILL.md +++ b/skills/typescript-code-review/SKILL.md @@ -116,7 +116,7 @@ When reviewing TKO code, apply these overrides: - Missing DOM disposal (`disposeWhenNodeIsRemoved`, `addDisposeCallback`, `LifeCycle.anchorTo()`) - Violations of centralized error handling (`options.onError` pattern) -**Testing**: Mocha/Chai/Sinon (not Jasmine); Karma + Electron; tests in `packages/*/spec/`. +**Testing**: Mocha/Chai/Sinon (not Jasmine); vitest; tests in `packages/*/spec/`. **Zero runtime deps**: never suggest external packages for core `@tko/*` packages. See `references/tko-conventions.md` for full details. From 6386af0e24fb3ad043cde041fb4337f04f82ed6e Mon Sep 17 00:00:00 2001 From: phillipc Date: Tue, 21 Apr 2026 15:25:06 +0200 Subject: [PATCH 04/10] chore: update TypeScript code review findings for rounds 1-4, archive round 1, and add new findings for rounds 3 and 4 --- plans/typescript-code-review-findings-2.md | 438 +++------------------ plans/typescript-code-review-findings-3.md | 54 +++ plans/typescript-code-review-findings-4.md | 202 ++++++++++ plans/typescript-code-review-findings.md | 186 ++------- 4 files changed, 329 insertions(+), 551 deletions(-) create mode 100644 plans/typescript-code-review-findings-3.md create mode 100644 plans/typescript-code-review-findings-4.md diff --git a/plans/typescript-code-review-findings-2.md b/plans/typescript-code-review-findings-2.md index 75851370c..91c53e82a 100644 --- a/plans/typescript-code-review-findings-2.md +++ b/plans/typescript-code-review-findings-2.md @@ -1,420 +1,84 @@ -# Plan: TypeScript Code Review — Findings (Round 2) +# Plan: TypeScript Code Review — Findings (Round 2, Deduplicated) **Risk class:** `MEDIUM` -**Status:** Draft +**Status:** Active Backlog -## Status Update (2026-04-20) +## Status Update (2026-04-21) -- Source basis: local `git log` + GitHub API (`knockout/tko`). -- Marked as done based on code + commits: Findings/Steps 1, 2, 5, 8, 9 (including `bdac39c2`, `3e7a37ae`, `5d6603d7`, `9659cf21`). -- Obsolete: references to `tools/repackage.mjs` (file no longer exists). +- Updated to current toolchain (Bun + Biome). +- Removed completed/obsolete items and cross-round duplicates. +- This file now keeps only Round-2-specific backlog items not promoted as canonical findings in rounds 3–4. -## Summary - -A comprehensive TypeScript code review of the full TKO monorepo — 25 packages, -2 builds, and tools — applied the `typescript-code-review` skill against all -production source. The review identified **5 critical bugs**, **15 important -improvements**, and **10 suggestions**. The codebase compiles cleanly (`tsc` -zero errors, `eslint` zero errors) and has strong modular architecture. - -The most severe findings are: - -1. **Proxy `deleteProperty` trap receives wrong argument** — all property - deletions on computed proxies silently corrupt the wrong key ✅ **Done** -2. **Parser operator precedence inverts JS semantics** — bitwise operators - parse above relational/equality, causing incorrect expression evaluation ✅ **Done** -3. **`??` (nullish coalescing) behaves identically to `||`** — treats `0`, - `''`, and `false` as nullish -4. **`TextInputLegacyFirefox` override is dead code** — Firefox - autocomplete/drag-drop events are never registered -5. **`style` binding references global `jQuery` instead of `options.jQuery`** ✅ **Done** - -Additional systemic issues: 17 loose-equality (`==`) comparisons, 4 deprecated -`.substr()` calls ✅ **Done**, 39 unresolved `FIXME`/`TODO` comments, and deprecated DOM -APIs with extraneous arguments in production source. - -## Goals - -- Fix all 5 confirmed bugs -- Replace all deprecated API usage (`.substr()`, `createEvent`/`initEvent`) -- Fix loose-equality comparisons in production source -- Clean up confirmed dead code - -## Non-Goals - -- Rewriting all `any` types at once (incremental approach preferred) -- Changing runtime behavior or public API surface beyond bug fixes -- Modifying `tools/build.mk` or `tools/karma.conf.js` (shared infra — needs - separate HIGH-risk plan) -- Adding new runtime dependencies -- Re-enabling disabled ESLint rules (separate effort) -- DON'T resolve or triage all FIXME/TODO annotations - -## Current State - -### Baselines - -- `make tsc` — zero errors -- `make eslint` — zero errors -- Loose-equality (`==`) in production source: **17 occurrences** -- ~~Deprecated `.substr()` in production source: **4 occurrences**~~ ✅ **Done** - (Current state: 0 matches in `packages/*/src` + `builds/*/src`) -- Unresolved `FIXME`/`TODO`/`HACK`/`XXX` in production source: **39 occurrences** - -### Overlap with Existing Plan - -The previous plan (`typescript-code-review-findings.md`) identified some of the -same issues (`.substr()` in `attr.ts`, `AttributeMustacheProvider` bug, FIXMEs). -This plan provides a comprehensive superset with verified new critical findings. -The previous plan's Phase 1 quick wins (steps 1–3) should be subsumed by this -plan's Phase 1. - ---- - -## Detailed Findings +## Stack Baseline (Current) -### Critical Issues 🔴 +- Type-check: `bun run tsc` +- Lint/format: `bun run check` +- Tests: `bun run test` +- Unused analysis: `bun run knip` -#### 1. Proxy `deleteProperty` trap receives wrong argument ✅ Done -**File**: `packages/computed/src/proxy.ts:57–60` -- **Issue**: The `deleteProperty` trap declares only one parameter `property`, - but the Proxy spec requires `(target, property)`. The first argument is the - target object, not the property name. All deletions on computed proxies - silently fail or corrupt the wrong key. -- **Current**: - ```ts - deleteProperty(property) { - delete mirror[property as any] - return delete object[property as any] - } - ``` -- **Recommended**: - ```ts - deleteProperty(_target, property) { - delete mirror[property as any] - return delete object[property as any] - } - ``` - -#### 2. Parser operator precedence inverts JS semantics for bitwise operators ✅ Done -**File**: `packages/utils.parser/src/operators.ts:172–192` -- **Issue**: Bitwise operators `|`(12), `^`(11), `&`(10) have higher precedence - than relational (11) and equality (10) operators — the exact inverse of - JavaScript. Expression `a < b | c` parses as `a < (b | c)` instead of - `(a < b) | c`. `^` collides with `<` at 11; `&` collides with `===` at 10. -- **Current**: - ```ts - operators['|'].precedence = 12 - operators['^'].precedence = 11 - operators['&'].precedence = 10 - operators['<'].precedence = 11 - operators['==='].precedence = 10 - ``` -- **Recommended** (match JS/MDN precedence): - ```ts - operators['<'].precedence = 12 // relational - operators['<='].precedence = 12 - operators['>'].precedence = 12 - operators['>='].precedence = 12 - operators['=='].precedence = 11 // equality - operators['!='].precedence = 11 - operators['==='].precedence = 11 - operators['!=='].precedence = 11 - operators['&'].precedence = 10 // bitwise AND - operators['^'].precedence = 9 // bitwise XOR - operators['|'].precedence = 8 // bitwise OR - ``` - -#### 3. `??` nullish coalescing behaves identically to `||` -**File**: `packages/utils.parser/src/operators.ts:199` -- **Issue**: `earlyOut` for `??` is `a => a`, which returns falsy for `0`, `''`, - and `false`. The RHS is evaluated unnecessarily and `??` becomes a duplicate - of `||`. If the RHS has side effects, they fire incorrectly. -- **Current**: `operators['??'].earlyOut = a => a` -- **Recommended**: `operators['??'].earlyOut = a => a !== null && a !== undefined` - -#### 4. `TextInputLegacyFirefox` overrides non-existent method (dead code) -**File**: `packages/binding.core/src/textInput.ts:132–143` -- **Issue**: Overrides `eventsIndicatingValueChange()`, but the parent class - `TextInput` calls `eventsIndicatingSyncValueChange()` and - `eventsIndicatingDeferValueChange()` — never `eventsIndicatingValueChange()`. - The Firefox-specific `DOMAutoComplete`, `dragdrop`, `drop` events are never - registered. -- **Current**: `eventsIndicatingValueChange(): string[] {` -- **Recommended**: `override eventsIndicatingSyncValueChange(): string[] {` +## Summary -#### 5. `style` binding references global `jQuery` instead of `options.jQuery` ✅ Done -**File**: `packages/binding.core/src/style.ts:16–17` -- **Issue**: Guards with `options.jQuery` but calls bare global `jQuery(element)`. - In module environments where jQuery is not a global, this throws - `ReferenceError`. -- **Current**: `if (options.jQuery) { jQuery(element).css(styleName, styleValue) }` -- **Recommended**: `if (options.jQuery) { options.jQuery(element).css(styleName, styleValue) }` +Round 2 currently tracks medium/low-risk cleanup and consistency work that is still relevant, +but not duplicated in newer findings plans. ---- +## Remaining Round-2 Findings ### Important Improvements 🟡 -#### 6. `subscribable.when()` — subscription leak on unsatisfied condition -**File**: `packages/observable/src/subscribable.ts:191–197` -- **Issue**: Promise never rejects. If the test condition is never satisfied, the - subscription lives forever and the Promise never settles — a memory leak. -- **Recommended**: Add a disposal mechanism (e.g., accept an `AbortSignal`, or - reject when the observable is disposed). - -#### 7. Duplicate `bindingKey` in error message -**File**: `packages/bind/src/applyBindings.ts:510–516` -- **Issue**: Error message includes `spec.bindingKey` twice, producing messages - like `Unable to process binding "text" in binding "text"`. -- **Recommended**: Remove the duplicated segment or replace the second with the - binding expression text. - -#### 8. `value.isInput()` type guard checks wrong element -**File**: `packages/binding.core/src/value.ts:53–54` -- **Issue**: Type guard narrows the `element` parameter but checks - `this.$element` instead. Works by coincidence since callers pass - `this.$element`. -- **Recommended**: `return tagNameLower(element) === 'input'` - -#### 9. Deprecated `createEvent`/`initEvent` with extraneous arguments ✅ Done -**File**: `packages/utils/src/dom/event.ts:83–96` -- **Issue**: Uses deprecated `document.createEvent()` and `initEvent()`. - `initEvent` accepts 3 arguments but 15 are passed (remnant from - `initMouseEvent` signature) — the extra 12 are silently ignored. -- **Recommended**: Replace with `new Event(eventType, { bubbles: true, cancelable: true })`. - -#### 10. Deprecated `.substr()` usage (4 occurrences) ✅ Done +1. `subscribable.when()` may keep subscription alive forever when condition never becomes true. +File: `packages/observable/src/subscribable.ts` -| File | Line | -|------|------| -| `packages/binding.core/src/attr.ts` | 15 | -| `packages/utils.parser/src/preparse.ts` | 94 | -| `packages/filter.punches/src/index.ts` | 54 | -| `packages/filter.punches/src/index.ts` | 57 | +2. Duplicate `bindingKey` in apply-bindings error message. +File: `packages/bind/src/applyBindings.ts` -- **Recommended**: Replace with `.substring()` (identical semantics for - non-negative indices). +3. `value.isInput()` checks `this.$element` instead of the parameter. +File: `packages/binding.core/src/value.ts` -#### 11. Duplicate import alias in computed -**File**: `packages/computed/src/computed.ts:12–15` -- **Issue**: `options` is imported both directly and as `options as koOptions`. - Dead alias creates confusion. -- **Recommended**: Remove one import; use a single name consistently. +4. Loose equality debt (`==`/`!=`) still needs intentional triage and selective replacement. +Scope: multiple packages -#### 12. Loose equality (`==`) instead of strict (`===`) — 17 occurrences -Across `packages/observable`, `packages/binding.core`, `packages/utils`, -`packages/binding.template`, `packages/filter.punches`. Most compare strings -or numbers where `===` is both safer and idiomatic. +5. Deprecated `clonePlainObjectDeep` remains exported. +File: `packages/utils/src/object.ts` -#### 13. Dead code: `dataStore` variable in utils -**File**: `packages/utils/src/dom/data.ts:8` -- **Issue**: `const dataStore = {}` declared but never referenced. Leftover from - prior implementation. -- **Recommended**: Remove. +6. `AttributeMustacheProvider.getBindingAccessors` returns `false` for non-element nodes (type-contract mismatch). +File: `packages/provider.mustache/src/AttributeMustacheProvider.ts` -#### 14. Deprecated `clonePlainObjectDeep` still exported -**File**: `packages/utils/src/object.ts:60–77` -- **Issue**: Annotated `@deprecated Function is unused` but still exported. -- **Recommended**: Remove after confirming no consumers via `knip`. +7. `Parser` imported as value and downcast via `as any` in component provider path. +File: `packages/provider.component/src/ComponentProvider.ts` -#### 15. `AttributeMustacheProvider.getBindingAccessors` returns `false` -**File**: `packages/provider.mustache/src/AttributeMustacheProvider.ts:99–100` -- **Issue**: Base `Provider.getBindingAccessors` returns an object. This override - returns `false` for non-Element nodes — a type-contract violation. -- **Recommended**: Return `Object.create(null)` for consistency. +8. JsxObserver subscription callback ignores callback value. +File: `packages/utils.jsx/src/JsxObserver.ts` -#### 16. `var` re-declaration shadows parameter in Parser Node ✅ Done -**File**: `packages/utils.parser/src/Node.ts:55–56` -- **Issue**: `var node: Node = this` re-declares the `node` parameter via `var` - hoisting, discarding it. Confusing and fragile. -- **Recommended**: Rename the parameter to `_node` and use `const node: Node = this`. - -#### 17. `Parser` imported as value but only used as type cast -**File**: `packages/provider.component/src/ComponentProvider.ts:11` -- **Issue**: `Parser` is imported as a value, then cast with `as any` to call - `new (Parser as any)(...)`. The `as any` hides constructor type errors. -- **Recommended**: Fix the `Parser` constructor signature to accept the correct - arguments, removing the need for `as any`. - -#### 18. JsxObserver subscription ignores callback argument -**File**: `packages/utils.jsx/src/JsxObserver.ts:369` -- **Issue**: Subscription callback receives `attr` (new value) but re-passes - `value` (the observable) to `setNodeAttribute`. Works because - `setNodeAttribute` calls `unwrap`, but the callback arg is wasted. -- **Current**: `value.subscribe(attr => this.setNodeAttribute(node, name, value))` -- **Recommended**: `value.subscribe(attr => this.setNodeAttribute(node, name, attr))` - -#### 19. NativeProvider redundant null-guard -**File**: `packages/provider.native/src/NativeProvider.ts:23–26` -- **Issue**: `|| {}` fallback is dead code — early return already handles falsy. -- **Recommended**: Remove the `|| {}`. - -#### 20. `repackage.mjs` swallows write errors silently ⚠️ Obsolete -**File**: `tools/repackage.mjs:48–49` -- **Issue**: `.catch(console.error)` logs but exits 0 on failure. CI won't - catch broken repackaging. -- **Recommended**: `await` the write and let rejections propagate. - ---- +9. `NativeProvider` redundant null fallback. +File: `packages/provider.native/src/NativeProvider.ts` ### Suggestions 🔵 -#### 21. Unused `SubscriptionCallback` import -**File**: `packages/bind/src/bindingEvent.ts:3` - -#### 22. Deprecated `event.returnValue = false` fallback -**File**: `packages/binding.core/src/submit.ts:17` -- Legacy IE property; `preventDefault()` is already called in the preceding branch. - -#### 23. `readElseChain` returns `false` where object expected -**File**: `packages/binding.if/src/else.ts:36` -- Returns `false` but callers access `.elseChainSatisfied`. Works by accident. +10. Unused `SubscriptionCallback` import. +File: `packages/bind/src/bindingEvent.ts` -#### 24. Redundant `nodeType` check after `instanceof Element` -**File**: `packages/provider.component/src/ComponentProvider.ts:55` +11. Legacy `event.returnValue` fallback. +File: `packages/binding.core/src/submit.ts` -#### 25. AMD require call lacks error callback -**File**: `packages/utils.component/src/loaders.ts:277` +12. `readElseChain()` returns `false` where object shape is expected. +File: `packages/binding.if/src/else.ts` -#### 26. `repackage.mjs` relative path fragility ⚠️ Obsolete -**File**: `tools/repackage.mjs:7` -- `../../lerna.json` assumes CWD depth. Derive from `import.meta.url`. +13. Redundant `nodeType` check after `instanceof Element`. +File: `packages/provider.component/src/ComponentProvider.ts` ---- +14. AMD `require` call has no explicit error callback path. +File: `packages/utils.component/src/loaders.ts` -### Positive Observations ✅ +## De-duplication Rules -- **Zero `tsc` errors** — the full production source compiles cleanly -- **Zero `eslint` errors** — linting passes across all packages -- **Consistent architecture** — factory-function-as-constructor, Symbol keys, - centralized error handling via `options.onError` are used uniformly -- **Proper `import type`** — the vast majority of type-only imports correctly - use `import type` as required by `verbatimModuleSyntax` -- **Strong modularity** — 25 packages with clear boundaries, barrel exports, - and zero runtime dependencies -- **LifeCycle disposal pattern** — modern binding handlers consistently use - `LifeCycle.anchorTo()` for automatic subscription cleanup -- **Well-structured tests** — Mocha/Chai/Sinon with ~89% statement coverage - ---- - -## Steps - -### Phase 1: Critical Bug Fixes (HIGH priority, behavior-changing) - -1. ✅ **Fix Proxy `deleteProperty` trap** — Add missing `_target` parameter in - `packages/computed/src/proxy.ts:57`. Verify with proxy-related tests in - `packages/computed/spec/`. Done via commit `bdac39c2`. - -2. ✅ **Fix parser operator precedence** — Reorder precedence values for bitwise, - relational, and equality operators in - `packages/utils.parser/src/operators.ts:172–192`. Must match JS semantics - exactly. Run full parser test suite. Done via commit `3e7a37ae`. - -3. **Fix `??` earlyOut semantics** — Change `a => a` to - `a => a !== null && a !== undefined` in - `packages/utils.parser/src/operators.ts:199`. Verify `??` correctly - preserves `0`, `''`, `false`. - -4. **Fix `TextInputLegacyFirefox` override** — Rename - `eventsIndicatingValueChange()` to `eventsIndicatingSyncValueChange()` in - `packages/binding.core/src/textInput.ts:132`. - -5. ✅ **Fix `style` binding jQuery reference** — Change `jQuery(element)` to - `options.jQuery(element)` in `packages/binding.core/src/style.ts:17`. - Done via commit `5d6603d7`. - -### Phase 2: Important Fixes (MEDIUM priority, no behavior change unless noted) - -6. **Fix `value.isInput()` type guard** — Change `this.$element` to `element` - in `packages/binding.core/src/value.ts:54`. - -7. **Fix duplicate error message** — Remove duplicated `spec.bindingKey` - segment in `packages/bind/src/applyBindings.ts:510–516`. - -8. ✅ **Replace deprecated `.substr()`** — 4 files (see finding #10). - Done via commit `9659cf21`. - -9. ✅ **Replace deprecated `createEvent`/`initEvent`** — Use `new Event()` in - `packages/utils/src/dom/event.ts:83–96`. - Done (current code uses `new MouseEvent`/`new KeyboardEvent`/`new Event`). - -10. **Fix loose equality** — Replace 17 `==`/`!=` with `===`/`!==` across - 6 packages (see finding #12). - -11. **Remove dead code** — `dataStore` in `utils/dom/data.ts`, - `clonePlainObjectDeep` in `utils/object.ts`, duplicate `options` alias - in `computed.ts`. - -12. **Fix `AttributeMustacheProvider` return type** — Return `Object.create(null)` - instead of `false` in `provider.mustache/src/AttributeMustacheProvider.ts:99`. - -13. **Fix JsxObserver subscription callback** — Pass `attr` instead of `value` - in `utils.jsx/src/JsxObserver.ts:369`. - -14. ✅ **Fix `repackage.mjs` error handling** — `await` the `writeFile` call - in `tools/repackage.mjs:48`. ⚠️ **Obsolete/dropped**: `tools/repackage.mjs` - is not present in the current repo state. - -### Phase 3: Cleanup (LOW priority) - -15. **Triage TODO/FIXME annotations** — Review all 39 annotations; fix, convert - to tracking issues, or remove stale ones. - -16. **Apply remaining suggestions** — Findings #21–#26. - ---- +- Findings tracked in rounds 3 or 4 must not be re-added here. +- Fixed findings stay in git history, not in the active list. +- Obsolete paths (for example removed files) are excluded. ## Verification -- `make tsc` — zero errors (must remain green after each step) -- `make test-headless` — all tests pass after each step -- `make eslint` — no new errors introduced -- `make format` — formatting check passes - -### Per-Phase Test Strategy - -- **Phase 1**: Run full test suite (`make test-headless`). For findings #2/#3 - (parser), add targeted test cases for `a < b | c` and `x ?? 0` expressions. - For finding #1 (proxy), test `delete proxy.key`. -- **Phase 2**: Run package-specific tests for each changed package, then full - suite at phase end. -- **Phase 3**: No behavior change expected; full suite once at phase end. - -## AI Evidence - -- Risk class: `MEDIUM` — behavior-changing bug fixes in binding/parser/proxy - logic; no CI/CD, release, or shared tooling modification -- Changes and steps: See Steps section (3 phases, 16 steps) -- Tools/commands: `tsc`, `eslint`, subagent code review across all packages - and builds, manual source verification of all 10 highest-severity findings -- Validation: `tsc` and `eslint` pass with zero errors; all 10 critical/important - findings confirmed against actual source code at reported line numbers -- Follow-up owner: Maintainer review required before Phase 1 implementation - (behavior-changing fixes) - -# LATER TASK (Don't do this now) - -#### 1. Unresolved TODO: class refactoring in subscribable -**File**: `packages/observable/src/subscribable.ts:68` - -#### 2. Unresolved TODO: downcast in observable -**File**: `packages/observable/src/observable.ts:209` - -#### 3. Unresolved TODO: dangerous `this` in static method -**File**: `packages/bind/src/BindingHandler.ts:97` - -#### 4. Unresolved TODOs across multiple packages -**Files**: `packages/utils/src/array.ts:97`, -`packages/utils/src/dom/html.ts:59,146`, -`packages/utils/src/dom/selectExtensions.ts:43`, -`packages/binding.template/src/templating.ts:40`, -`packages/binding.template/src/templateEngine.ts:63`, -`packages/binding.foreach/src/foreach.ts:581`, -`packages/binding.if/src/ConditionalBindingHandler.ts:11`, -`packages/provider/src/Provider.ts:59`, -`packages/utils.parser/src/operators.ts:82–84`, -`builds/reference/src/common.ts:5` -- **Total**: 39 FIXME/TODO/HACK/XXX annotations in production source. -- **Recommended**: Triage all: fix, convert to issues, or remove if stale. +- `bun run tsc` +- `bun run check` +- `bun run test` +- `bun run knip` diff --git a/plans/typescript-code-review-findings-3.md b/plans/typescript-code-review-findings-3.md new file mode 100644 index 000000000..c51ee6cd3 --- /dev/null +++ b/plans/typescript-code-review-findings-3.md @@ -0,0 +1,54 @@ +# Plan: TypeScript Code Review — Findings (Round 3, Deduplicated) + +**Risk class:** `MEDIUM` + +**Status:** Active Backlog + +## Status Update (2026-04-21) + +- Updated to current software stack terminology. +- Kept only canonical Round-3 findings. +- Removed overlap duplicates with rounds 1, 2, and 4. + +## Stack Baseline (Current) + +- Type-check: `bun run tsc` +- Lint/format: `bun run check` +- Tests: `bun run test` + +## Summary + +Round 3 focuses on parser-semantics parity and runtime portability concerns. +It tracks one critical and two important findings that remain canonical for this round. + +## Detailed Findings + +### Critical Issues 🔴 + +1. `LifeCycle.__addEventListener` removes listeners with mismatched signature. +File: `packages/lifecycle/src/LifeCycle.ts` + +- Register uses `addEventListener(..., options)` but dispose used `removeEventListener(... )` without options. +- Capture listeners can remain attached after disposal. + +### Important Improvements 🟡 + +2. Parser accepts invalid JavaScript mixing of `??` with `||` / `&&` without required parentheses. +File: `packages/utils.parser/src/operators.ts` + +- Native JS requires explicit grouping for these mixes. +- Current parser accepts and evaluates such expressions. + +3. Runtime paths use ambient `document/window` instead of configured options. +Files: +- `packages/utils.component/src/loaders.ts` +- `packages/binding.foreach/src/foreach.ts` + +- Should consistently use `options.document` / `options.global` for portability. + +## Verification + +- `bun run tsc` +- `bun run check` +- `bun run test` +- Targeted parser and lifecycle regression tests for the above paths diff --git a/plans/typescript-code-review-findings-4.md b/plans/typescript-code-review-findings-4.md new file mode 100644 index 000000000..8e1783f88 --- /dev/null +++ b/plans/typescript-code-review-findings-4.md @@ -0,0 +1,202 @@ +# Plan: TypeScript Code Review — Findings (Round 4) + +**Risk class:** `MEDIUM` + +**Status:** Active Backlog + +## Status Update (2026-04-21) + +- Updated to current toolchain language (Bun + Biome). +- Deduplicated against rounds 1–3. +- This file keeps only Round-4-canonical findings. + +## Stack Baseline (Current) + +- Type-check: `bun run tsc` +- Lint/format: `bun run check` +- Tests: `bun run test` +- Unused analysis: `bun run knip` + +## Summary + +Round 4 tracks **2 critical bugs**, **8 important improvements**, and **9 suggestions** +that are not duplicated as canonical findings in rounds 1–3. + +## Prior Rounds + +For historical context and previously cataloged findings, see: + +1. `plans/typescript-code-review-findings.md` +2. `plans/typescript-code-review-findings-2.md` +3. `plans/typescript-code-review-findings-3.md` + +--- + +## Detailed Findings + +### Critical Issues 🔴 + +#### 1. `notifyNextChange` closure variable never reset — sticky spurious notifications +**File**: `packages/observable/src/observable.ts:212` +- **Issue**: In `subscribable.fn.limit`, the chained assignment + `self._notifyNextChange = didUpdate = ignoreBeforeChange = false` writes to + the instance property `self._notifyNextChange` — which is never read anywhere + in the codebase — instead of resetting the closure variable `notifyNextChange`. + Once `_notifyNextChangeIfValueIsDifferent()` sets `notifyNextChange = true` + (line 238), it is **never** reset to `false`. Every subsequent call to + `finish()` will see `shouldNotify === true` regardless of value equality, + causing spurious notifications for any rate-limited or deferred observable. +- **Verified**: `notifyNextChange` is a closure-scoped `let` (line 194). + `self._notifyNextChange` is never read. Confirmed bug. +- **Current**: + ```ts + self._notifyNextChange = didUpdate = ignoreBeforeChange = false + ``` +- **Recommended**: + ```ts + notifyNextChange = didUpdate = ignoreBeforeChange = false + ``` + +#### 2. Missing `getOwnPropertyDescriptor` Proxy trap breaks `Object.keys()`, `JSON.stringify()` +**File**: `packages/computed/src/proxy.ts:47–84` +- **Issue**: The Proxy defines an `ownKeys` trap returning keys from the source + `object`, but no `getOwnPropertyDescriptor` trap. The proxy target is an empty + `function () {}`. When `Object.keys(proxy)` is called, the engine calls + `ownKeys` (returns real keys), then calls `getOwnPropertyDescriptor` for each — + which falls through to the empty function target, returning `undefined`. All + keys are filtered out. +- **Impact**: `Object.keys(proxy)` returns `[]`, `JSON.stringify(proxy)` returns + `undefined`, and `{...proxy}` produces `{}`. +- **Verified**: Confirmed — no `getOwnPropertyDescriptor` trap exists in the + handler. +- **Recommended**: + ```ts + getOwnPropertyDescriptor(_target, prop) { + return Object.getOwnPropertyDescriptor(object, prop) + ?? Reflect.getOwnPropertyDescriptor(_target, prop) + } + ``` + +--- + +### Important Improvements 🟡 + +#### 3. Base `Provider.preprocessNode` returns `[node]`, short-circuiting MultiProvider +**File**: `packages/provider/src/Provider.ts:53–55` +- **Issue**: The base `Provider.preprocessNode` returns `[node]` (truthy) instead + of `null`. In `MultiProvider.preprocessNode`, the first provider returning + truthy wins — subsequent providers are never consulted. Any provider inheriting + the default claims every node, preventing later providers from preprocessing. +- **Current**: `return [node]` +- **Recommended**: `return null` + +#### 4. `Text.textNodeReplacement` ignores `textNode` parameter, uses global `document` +**File**: `packages/provider.mustache/src/mustacheParser.ts:67–70` +- **Issue**: `Text.textNodeReplacement()` declares zero parameters, but callers + pass `textNode`. The argument is silently ignored. Uses global + `document.createTextNode(...)` while sibling `Expression.textNodeReplacement` + correctly uses `textNode.ownerDocument`. Causes cross-document DOM adoption + errors. +- **Recommended**: Accept `textNode` parameter, use `textNode.ownerDocument`. + +#### 5. `VirtualProvider.preprocessNode` removes node unconditionally when `parent` is null +**File**: `packages/provider.virtual/src/VirtualProvider.ts:21–31` +- **Issue**: Replacement insertions use `parent?.insertBefore(...)` (no-op when + null), but `node.remove()` always executes. Content silently lost. +- **Recommended**: Early-return `null` when `!parent`. + +#### 6. `slotBinding.ts` `getSlot()` returns `Node[]` typed as `Node` +**File**: `packages/binding.component/src/slotBinding.ts:76–86` +- **Issue**: `getSlot` declares return type `Node`, but the default-slot fallback + returns a filtered `Node[]`. +- **Recommended**: Correct return type to `Node | Node[]` or normalize. + +#### 7. `foreach.ts` `removeNodes` — null `parentNode` dereference +**File**: `packages/binding.foreach/src/foreach.ts:~413` +- **Issue**: `removeFn` reads `nodes[0].parentNode` into `parent`, then calls + `parent.removeChild()`. If nodes are already detached, throws `TypeError`. +- **Recommended**: Use `parent?.removeChild(nodes[i])`. + +#### 8. `foreach.ts` `makeTemplateNode` bypasses HTML sanitization +**File**: `packages/binding.foreach/src/foreach.ts:~60` +- **Issue**: For `