From cfaaf1d25a5fbe99f6254b467ab5fba8c8b86ac8 Mon Sep 17 00:00:00 2001 From: Mgrdich Date: Mon, 6 Jul 2026 14:06:12 -0400 Subject: [PATCH] =?UTF-8?q?refactor:=20PR-44=20audit=20follow-up=20?= =?UTF-8?q?=E2=80=94=20shared=20applyPhaseGuarded,=20select=20throw=20rout?= =?UTF-8?q?ing,=20doc=20consistency?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge cleanup on top of spec 039 (PR #44): - Extract the `$$phase`-guarded `$apply`/`$evalAsync` dispatch into a single shared `src/compiler/apply-phase-guarded.ts` helper. The spec-026 event directives, forms `applyDuringEvent` (input-types), `form` submit, and `select`'s `applyChange` now all route through it with their own cause token instead of duplicating the try/catch. - Fix `select`: `applyChange` previously lacked the try/catch, so a throwing `$parser` during the native change commit escaped the listener without reaching `$exceptionHandler`. It now injects `$exceptionHandler` and routes via `'$compile'`. Adds a regression test in select.test.ts. - De-duplicate `stashController` — remove the local copy in compile.ts and import the shared one from element-slots.ts. - Rename runControllerSeam-widening.test.ts → kebab-case run-controller-seam-widening.test.ts (file-naming convention). - Add inline justifications to the eslint-disable comments in core/utils.ts (isFunction, TypedArray copy cast, dynamic delete). - Doc consistency: update the stale "EXCEPTION_HANDLER_CAUSES stays at 10" comments across compiler/controller/bootstrap error files (the tuple is now 13 after spec 037) and refresh CLAUDE.md + roadmap.md. typecheck + lint clean; 4243 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 6 +- context/product/roadmap.md | 6 +- src/bootstrap/bootstrap-error.ts | 4 +- src/compiler/__tests__/component.test.ts | 2 +- src/compiler/__tests__/require.test.ts | 2 +- ...s => run-controller-seam-widening.test.ts} | 0 src/compiler/__tests__/spec023-parity.test.ts | 2 +- src/compiler/__tests__/spec024-parity.test.ts | 2 +- src/compiler/__tests__/spec025-parity.test.ts | 2 +- src/compiler/__tests__/spec026-parity.test.ts | 2 +- src/compiler/__tests__/spec027-parity.test.ts | 2 +- src/compiler/__tests__/spec028-parity.test.ts | 4 +- src/compiler/__tests__/spec029-parity.test.ts | 4 +- src/compiler/__tests__/spec030-parity.test.ts | 4 +- src/compiler/__tests__/spec031-parity.test.ts | 2 +- .../__tests__/structural-conflict.test.ts | 2 +- .../__tests__/template-errors.test.ts | 2 +- .../__tests__/transclude-errors.test.ts | 2 +- src/compiler/apply-phase-guarded.ts | 55 +++++++++++++++++++ src/compiler/compile-error.ts | 14 ++--- src/compiler/compile-provider.ts | 2 +- src/compiler/compile.ts | 27 +-------- src/compiler/directive-types.ts | 2 +- src/compiler/lifecycle.ts | 2 +- src/compiler/ng-attribute-aliases.ts | 2 +- src/compiler/ng-controller.ts | 2 +- src/compiler/ng-event-directives.ts | 38 +++++-------- src/compiler/ng-if.ts | 2 +- src/compiler/ng-include.ts | 2 +- src/compiler/ng-pluralize.ts | 2 +- src/compiler/ng-ref.ts | 2 +- src/compiler/ng-switch.ts | 2 +- src/compiler/require-resolver.ts | 2 +- .../__tests__/controller-di.test.ts | 4 +- .../__tests__/controller-parity.test.ts | 2 +- src/controller/controller-errors.ts | 2 +- src/core/__tests__/utils.test.ts | 2 +- src/core/utils.ts | 6 +- src/forms/__tests__/select.test.ts | 44 ++++++++++++++- src/forms/form.ts | 16 ++---- src/forms/input-types.ts | 18 ++---- src/forms/select.ts | 21 ++++--- 42 files changed, 185 insertions(+), 136 deletions(-) rename src/compiler/__tests__/{runControllerSeam-widening.test.ts => run-controller-seam-widening.test.ts} (100%) create mode 100644 src/compiler/apply-phase-guarded.ts diff --git a/CLAUDE.md b/CLAUDE.md index 6c65ff3..bea2d04 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -41,7 +41,7 @@ CI (`.github/workflows/ci.yml`) gates on: lint → format:check → typecheck - **No `new Function()` / no `eval()`** — the expression parser uses a **tree-walking interpreter** deliberately. This avoids CSP violations and is part of the project's security posture. Don't "optimize" by generating code strings. - **Digest TTL contract** — configurable via `Scope.create({ ttl: 20 })` (default 10). TTL breach throws with the watch function source in the error to help identify unstable watchers. - **One-time bindings & constant watches** (spec 010) — the parser attaches `literal` / `constant` / `oneTime` flags on AST nodes. Scope wires `oneTimeWatchDelegate` (literal) or `oneTimeLiteralWatchDelegate` for `::expr` expressions; constant expressions use `constantWatchDelegate`. When modifying the watcher wiring, preserve those delegate selections. -- **Module boundary rule** — `parser/*` and `di/*` depend only on `@core` (prefer importing from `@core/index`, not `@core/utils` directly). `core/scope.ts` intentionally depends on `@parser/index` because scopes evaluate expression strings. `compiler/` has no dependencies yet. +- **Module boundary rule** — `parser/*` and `di/*` depend only on `@core` (prefer importing from `@core/index`, not `@core/utils` directly). `core/scope.ts` intentionally depends on `@parser/index` because scopes evaluate expression strings. Two documented exceptions: `di/module.ts` carries `import type`-only references to `@compiler`/`@controller`/`@filter` (erased at build — zero runtime dependency), and `parser/interpreter.ts` carries ONE runtime value import from `@filter` (`FilterLookupError`, spec 016 — the interpreter constructs the error when a filter lookup misses; its `FilterFn`/`FilterService` references are `import type`-only). Don't widen either exception without a spec. - **Error handling in digest** — listener/watch exceptions are logged with `console.error('...', e)` and the digest continues. Don't swallow silently and don't abort the digest loop on a single listener failure. - **Strict mode is frozen after the config phase** — `$sceProvider.enabled(false)` is the only way to disable SCE. Once the injector finishes config, `$sce.isEnabled()` is permanent. Strict-OFF turns both `trustAs*` and `getTrusted*` into total pass-throughs (no wrapper classes are constructed). - **Trusted values are per-context nominal classes** — `TrustedResourceUrl extends TrustedUrl`, so a trusted resource URL is accepted where a trusted URL is expected (not vice-versa). Identity is checked via `instanceof`, not a string-based brand. Do NOT "optimize" to a single branded wrapper — the subtype rule matters for AngularJS parity. @@ -156,12 +156,12 @@ CI (`.github/workflows/ci.yml`) gates on: lint → format:check → typecheck ## Coding conventions -- **TypeScript strict** (`strict: true` + `noUncheckedIndexedAccess`). No `any` — the single existing cast in `src/core/utils.ts:229` is the ceiling, not a precedent. +- **TypeScript strict** (`strict: true` + `noUncheckedIndexedAccess`). No `any` — the single existing cast in `src/core/utils.ts` (the TypedArray-copy `source.constructor as any` inside `copyRecursive`, ~line 277) is the ceiling, not a precedent. - **Every `eslint-disable` comment must carry an inline justification** (`-- reason`). CI enforces lint. - **No explicit return types** when TS inference handles them — let inference do the work. Annotate only on exported public-API boundaries where the declared shape is part of the contract. - **Imports**: use path aliases (`@core`, `@parser`, `@di`, `@compiler`). `no-restricted-imports` blocks `../*` relative climbing. - **File naming**: kebab-case (`scope-watch-delegates.ts`, `ast-flags.ts`). Tests under `src//__tests__/*.test.ts`. -- **File size target**: under 500 lines per source file. Refactor candidates today: `src/core/scope.ts` (827), `src/di/module.ts` (776), `src/di/injector.ts` (734). +- **File size target**: under 500 lines per source file. 17 source files currently exceed it (2026-07 audit); the standout refactor candidates: `src/compiler/compile.ts` (~2300 — 4.6× the target), `src/compiler/compile-provider.ts` (1281), `src/di/module.ts` (1269), `src/compiler/compile-error.ts` (1136), `src/compiler/directive-types.ts` (978), `src/core/scope.ts` (950), `src/compiler/attributes.ts` (846), `src/di/injector.ts` (750), `src/core/ng-module.ts` (676). A dedicated `compile.ts`-split spec is the highest-value refactor. ## Git + spec workflow diff --git a/context/product/roadmap.md b/context/product/roadmap.md index 5929f55..b7549c4 100644 --- a/context/product/roadmap.md +++ b/context/product/roadmap.md @@ -49,7 +49,7 @@ _Complete the essential building blocks that everything else depends on._ _The layer that connects the runtime to templates and the DOM._ -- [ ] **Expressions & Parser** +- [x] **Expressions & Parser** - [x] **Expression Parser:** Implement a full expression parser supporting property access, method calls, operators, literals, and assignments and all the supported features of AngularJS 1.x, integration with scope. - [x] **One-Time Bindings:** Support `::` prefix for expressions that unwatch after stabilization. - [x] **Interpolation:** Implement `$interpolate` service for `{{expression}}` resolution in strings and templates. @@ -59,7 +59,7 @@ _The layer that connects the runtime to templates and the DOM._ - [x] **$interpolate Integration:** Wire the `trustedContext` parameter on `$interpolate` to `$sce.getTrusted(...)` — resolves the `TODO(spec-$sce)` marker in `src/interpolate/interpolate.ts` left by spec 011. - [x] **$sceProvider:** Support config-phase `enabled(value?)` to toggle strict mode. -- [ ] **HTML Sanitization ($sanitize / ngSanitize)** +- [x] **HTML Sanitization ($sanitize / ngSanitize)** - [x] **Separate `ngSanitize` module:** Ship as a dedicated module, NOT part of core `ng`. Mirrors AngularJS 1.x `angular-sanitize.js` packaging so apps that don't need sanitization don't pay for its parser tables or attack surface. New `src/sanitize/` subpath + `@sanitize/*` alias + `./sanitize` in `package.json` exports and `rollup.config.mjs` entries, following the `./sce` / `./interpolate` layout. - [x] **ESM-first `createSanitize` / `sanitize` factory:** Pure `(untrustedHtml: string) => string` pipeline with no DI dependency — usable standalone and via `$sanitize` DI registration. Follows the `createSce` / `sce` precedent. - [x] **`$sanitize` service + `$SanitizeProvider`:** DI-layer thin shim registered on `ngSanitize`; provider owns only the allow-list extensions (see below). `$get` depends on the ESM factory — zero duplicate logic. @@ -67,7 +67,7 @@ _The layer that connects the runtime to templates and the DOM._ - [x] **Attribute allow-list per tag:** Fixed whitelist (`href`, `src`, `alt`, `title`, `class`, `id`, …) with tag-specific constraints (e.g. `target` only on ``). Disallowed attributes (including all `on*` event handlers) are stripped. *(Implementation note: ships as a single global allow-list (`VALID_ATTRS`) rather than per-tag — AngularJS 1.x parity. Per-tag scoping is deferred.)* - [x] **URL-protocol safe-list for `href` / `src`:** Same allow-list regex used by `$compileProvider.aHrefSanitizationTrustedUrlList` — defaults to `/^\s*(https?|s?ftp|mailto|tel|file):/` plus relative URLs. `javascript:` and dangerous `data:` URIs are stripped. Configurable via `$sanitizeProvider.uriPattern(RegExp)`. - [x] **`$sce.getTrustedHtml` fallback integration:** When a value reaches `$sce.getTrustedHtml(...)` WITHOUT being wrapped AND `$sanitize` is available on the injector, delegate to `$sanitize(value)` instead of throwing. Keeps the spec-012 strict-mode contract intact (plain strings still throw when `$sanitize` isn't loaded) and matches AngularJS 1.x `ng-bind-html` behavior. Small coordination edit in `src/sce/sce.ts` gated behind an optional dependency lookup. - - [ ] **`ng-bind-html` directive integration:** Lands with the Directives & DOM Compilation roadmap item below — `ng-bind-html="expr"` evaluates `expr`, runs through `$sce.getTrustedHtml` (which now routes to `$sanitize` when appropriate), and sets `innerHTML`. *(Deferred — depends on `$compile`.)* + - [x] **`ng-bind-html` directive integration:** Lands with the Directives & DOM Compilation roadmap item below — `ng-bind-html="expr"` evaluates `expr`, runs through `$sce.getTrustedHtml` (which now routes to `$sanitize` when appropriate), and sets `innerHTML`. _(spec 023 — shipped as part of the visibility & binding built-ins.)_ - [x] **AngularJS parity tests + documented CVE regressions:** Port test vectors from `angular/angular.js/test/ngSanitize/sanitizeSpec.js`. Include a dedicated mXSS-regression suite covering each historical `ngSanitize` CVE (tag confusion, attribute-context breaks, etc.) so future edits can't regress. - [x] **DOMPurify-compat escape hatch:** Document how to swap the built-in implementation for DOMPurify via a decorator (`.decorator('$sanitize', () => domPurifyBackedImpl)`). No hard dependency; purely a documented pattern so teams with stricter security posture can opt in. *(Documented in `src/sanitize/README.md`.)* diff --git a/src/bootstrap/bootstrap-error.ts b/src/bootstrap/bootstrap-error.ts index 39c3d6b..866ccbf 100644 --- a/src/bootstrap/bootstrap-error.ts +++ b/src/bootstrap/bootstrap-error.ts @@ -8,8 +8,8 @@ * are part of the public contract and locked by tests. * * These are PROGRAMMER errors surfaced directly to the caller — they are NOT - * routed through `$exceptionHandler` (the `EXCEPTION_HANDLER_CAUSES` tuple stays - * at 10). Unregistered string-name modules reuse `getModule`'s existing + * routed through `$exceptionHandler` (the `EXCEPTION_HANDLER_CAUSES` tuple gains no + * bootstrap token). Unregistered string-name modules reuse `getModule`'s existing * `Module not found: ` throw rather than a new class. */ diff --git a/src/compiler/__tests__/component.test.ts b/src/compiler/__tests__/component.test.ts index 182ba23..99717b7 100644 --- a/src/compiler/__tests__/component.test.ts +++ b/src/compiler/__tests__/component.test.ts @@ -19,7 +19,7 @@ * {@link InvalidComponentDefinitionError}; downstream directive * normalization runs lazily at `Directive` provider `$get` time * and routes via `$exceptionHandler('$compile')` through the existing - * factory `try/catch`. `EXCEPTION_HANDLER_CAUSES` stays at 10. + * factory `try/catch`. `EXCEPTION_HANDLER_CAUSES` is unchanged. * * **Controller spelling.** Tests use the canonical array-style * annotation with a trailing function expression that stashes the diff --git a/src/compiler/__tests__/require.test.ts b/src/compiler/__tests__/require.test.ts index 66dfea8..7935db3 100644 --- a/src/compiler/__tests__/require.test.ts +++ b/src/compiler/__tests__/require.test.ts @@ -15,7 +15,7 @@ * BEFORE `$onInit` runs. * * Resolution failure (`MissingRequiredControllerError`) routes via - * `$exceptionHandler('$compile')`; the tuple stays at 10. + * `$exceptionHandler('$compile')`; the tuple is unchanged. * * Both link sites are exercised: the inline (synchronous) link path * AND the `templateUrl` post-template-install link path — same diff --git a/src/compiler/__tests__/runControllerSeam-widening.test.ts b/src/compiler/__tests__/run-controller-seam-widening.test.ts similarity index 100% rename from src/compiler/__tests__/runControllerSeam-widening.test.ts rename to src/compiler/__tests__/run-controller-seam-widening.test.ts diff --git a/src/compiler/__tests__/spec023-parity.test.ts b/src/compiler/__tests__/spec023-parity.test.ts index 93aa307..e696bc3 100644 --- a/src/compiler/__tests__/spec023-parity.test.ts +++ b/src/compiler/__tests__/spec023-parity.test.ts @@ -26,7 +26,7 @@ * * Mirrors the structural precedent set by * `src/compiler/__tests__/spec022-parity.test.ts` (and the - * `EXCEPTION_HANDLER_CAUSES.length === 10` regression guard pattern + * `EXCEPTION_HANDLER_CAUSES.length` regression guard pattern * from there). * * @see context/spec/023-visibility-and-binding-directives/functional-spec.md diff --git a/src/compiler/__tests__/spec024-parity.test.ts b/src/compiler/__tests__/spec024-parity.test.ts index 52d5811..dacb2b0 100644 --- a/src/compiler/__tests__/spec024-parity.test.ts +++ b/src/compiler/__tests__/spec024-parity.test.ts @@ -24,7 +24,7 @@ * * Mirrors the structural precedent set by * `src/compiler/__tests__/spec023-parity.test.ts` (and the - * `EXCEPTION_HANDLER_CAUSES.length === 10` regression guard pattern + * `EXCEPTION_HANDLER_CAUSES.length` regression guard pattern * from there). * * @see context/spec/024-class-and-style-directives/functional-spec.md diff --git a/src/compiler/__tests__/spec025-parity.test.ts b/src/compiler/__tests__/spec025-parity.test.ts index 5536550..14e2d2c 100644 --- a/src/compiler/__tests__/spec025-parity.test.ts +++ b/src/compiler/__tests__/spec025-parity.test.ts @@ -21,7 +21,7 @@ * * Mirrors the structural precedent set by * `src/compiler/__tests__/spec024-parity.test.ts` (and the - * `EXCEPTION_HANDLER_CAUSES.length === 10` regression guard pattern + * `EXCEPTION_HANDLER_CAUSES.length` regression guard pattern * from there). * * @see context/spec/025-attribute-helper-directives/functional-spec.md diff --git a/src/compiler/__tests__/spec026-parity.test.ts b/src/compiler/__tests__/spec026-parity.test.ts index 226c0e0..8694768 100644 --- a/src/compiler/__tests__/spec026-parity.test.ts +++ b/src/compiler/__tests__/spec026-parity.test.ts @@ -32,7 +32,7 @@ * * Mirrors the structural precedent set by * `src/compiler/__tests__/spec025-parity.test.ts` (and the - * `EXCEPTION_HANDLER_CAUSES.length === 10` regression guard pattern + * `EXCEPTION_HANDLER_CAUSES.length` regression guard pattern * from there). * * @see context/spec/026-event-directives/functional-spec.md diff --git a/src/compiler/__tests__/spec027-parity.test.ts b/src/compiler/__tests__/spec027-parity.test.ts index f15dbc5..1b25894 100644 --- a/src/compiler/__tests__/spec027-parity.test.ts +++ b/src/compiler/__tests__/spec027-parity.test.ts @@ -35,7 +35,7 @@ * * Mirrors the structural precedent set by * `src/compiler/__tests__/spec026-parity.test.ts` (and the - * `EXCEPTION_HANDLER_CAUSES.length === 10` regression guard pattern + * `EXCEPTION_HANDLER_CAUSES.length` regression guard pattern * from there). * * @see context/spec/027-structural-flow-control-directives/functional-spec.md diff --git a/src/compiler/__tests__/spec028-parity.test.ts b/src/compiler/__tests__/spec028-parity.test.ts index ada3725..dbb2f09 100644 --- a/src/compiler/__tests__/spec028-parity.test.ts +++ b/src/compiler/__tests__/spec028-parity.test.ts @@ -31,7 +31,7 @@ * own catch routes via the existing `'$compile'` cause token, NOT * `'watchListener'`. * - * Plus the `EXCEPTION_HANDLER_CAUSES.length === 10` regression guard — + * Plus the `EXCEPTION_HANDLER_CAUSES.length` regression guard — * spec 028 introduces FOUR new error classes * (`NgRepeatBadIteratorExpressionError`, `NgRepeatBadIdentifierError`, * `NgRepeatBadAliasError`, `NgRepeatDuplicateKeyError`) but ZERO new @@ -39,7 +39,7 @@ * * Mirrors the structural precedent set by * `src/compiler/__tests__/spec027-parity.test.ts` (and the - * `EXCEPTION_HANDLER_CAUSES.length === 10` regression-guard pattern + * `EXCEPTION_HANDLER_CAUSES.length` regression-guard pattern * established by spec 023 → spec 027). * * @see context/spec/028-ng-repeat/functional-spec.md diff --git a/src/compiler/__tests__/spec029-parity.test.ts b/src/compiler/__tests__/spec029-parity.test.ts index f042cda..8ee8485 100644 --- a/src/compiler/__tests__/spec029-parity.test.ts +++ b/src/compiler/__tests__/spec029-parity.test.ts @@ -30,7 +30,7 @@ * down cleanly inside an `ng-repeat` row and on an `ng-if` subtree * without tripping the spec-027 same-element structural gap. * - * Plus the `EXCEPTION_HANDLER_CAUSES.length === 10` regression guard — + * Plus the `EXCEPTION_HANDLER_CAUSES.length` regression guard — * spec 029 introduces TWO new error classes * (`NgPluralizeNoRuleDefinedError`, `NgPluralizeBadOffsetError`) but * ZERO new cause tokens; both route via the existing `'$compile'` @@ -38,7 +38,7 @@ * * Mirrors the structural precedent set by * `src/compiler/__tests__/spec028-parity.test.ts` (and the - * `EXCEPTION_HANDLER_CAUSES.length === 10` regression-guard pattern + * `EXCEPTION_HANDLER_CAUSES.length` regression-guard pattern * established by spec 023 → spec 028). * * @see context/spec/029-ng-pluralize/functional-spec.md diff --git a/src/compiler/__tests__/spec030-parity.test.ts b/src/compiler/__tests__/spec030-parity.test.ts index e39620e..d3c9d30 100644 --- a/src/compiler/__tests__/spec030-parity.test.ts +++ b/src/compiler/__tests__/spec030-parity.test.ts @@ -24,14 +24,14 @@ * several rows, each carrying an `ng-ref`, compile / digest / render * the expected row count without errors. * - * Plus the `EXCEPTION_HANDLER_CAUSES.length === 10` regression guard — + * Plus the `EXCEPTION_HANDLER_CAUSES.length` regression guard — * spec 030 introduces new error classes (`NgRefBadExpressionError`, * `NgRefNoControllerError`) but ZERO new cause tokens; both route via * the existing `'$compile'` token. * * Mirrors the structural precedent set by * `src/compiler/__tests__/spec029-parity.test.ts` (and the - * `EXCEPTION_HANDLER_CAUSES.length === 10` regression-guard pattern + * `EXCEPTION_HANDLER_CAUSES.length` regression-guard pattern * established by spec 023 → spec 029). * * @see context/spec/030-csp-template-cache-element-overrides/functional-spec.md diff --git a/src/compiler/__tests__/spec031-parity.test.ts b/src/compiler/__tests__/spec031-parity.test.ts index 69b0570..1e23716 100644 --- a/src/compiler/__tests__/spec031-parity.test.ts +++ b/src/compiler/__tests__/spec031-parity.test.ts @@ -25,7 +25,7 @@ * - **Composite** — a page mixing text `{{ }}`, attribute `{{ }}`, * `ng-if`, and `ng-repeat` renders end-to-end and updates on digest. * - * Plus the `EXCEPTION_HANDLER_CAUSES.length === 10` regression guard — + * Plus the `EXCEPTION_HANDLER_CAUSES.length` regression guard — * spec 031 introduces ZERO new cause tokens (the eager-pass catch reuses * the existing `'$compile'` token). * diff --git a/src/compiler/__tests__/structural-conflict.test.ts b/src/compiler/__tests__/structural-conflict.test.ts index cba0ce9..772e810 100644 --- a/src/compiler/__tests__/structural-conflict.test.ts +++ b/src/compiler/__tests__/structural-conflict.test.ts @@ -21,7 +21,7 @@ * - The same error for `ng-if` + `ng-include` and * `ng-repeat` + `ng-switch-when`. * - The canonical nested workaround renders correctly with no error. - * - `EXCEPTION_HANDLER_CAUSES.length === 10` (no new token). + * - `EXCEPTION_HANDLER_CAUSES.length` unchanged (no new token). */ import { afterEach, describe, expect, it } from 'vitest'; diff --git a/src/compiler/__tests__/template-errors.test.ts b/src/compiler/__tests__/template-errors.test.ts index b2227cb..a8f0f3e 100644 --- a/src/compiler/__tests__/template-errors.test.ts +++ b/src/compiler/__tests__/template-errors.test.ts @@ -12,7 +12,7 @@ * `invokeExceptionHandler` recursion guard catches the handler's * throw; template loading does NOT crash; falls back to * `console.error`. - * 2. `EXCEPTION_HANDLER_CAUSES.length === 10` regression — no new cause + * 2. `EXCEPTION_HANDLER_CAUSES.length` regression — no new cause * token introduced by spec 019; `'$compile'` covers every template * error site. * 3. `'$compile' satisfies ExceptionHandlerCause` compile-time check. diff --git a/src/compiler/__tests__/transclude-errors.test.ts b/src/compiler/__tests__/transclude-errors.test.ts index aaa637a..6964471 100644 --- a/src/compiler/__tests__/transclude-errors.test.ts +++ b/src/compiler/__tests__/transclude-errors.test.ts @@ -17,7 +17,7 @@ * the same clone still link; other clones produce normally. * 4. Custom `$exceptionHandler` that itself throws → spec-014 recursion * guard catches; transclusion does not crash. - * 5. `EXCEPTION_HANDLER_CAUSES` length unchanged at 10; `'$compile'` + * 5. `EXCEPTION_HANDLER_CAUSES` length unchanged (no transclude token); `'$compile'` * still included. * 6. `'$compile' satisfies ExceptionHandlerCause` at compile time. */ diff --git a/src/compiler/apply-phase-guarded.ts b/src/compiler/apply-phase-guarded.ts new file mode 100644 index 0000000..b590d48 --- /dev/null +++ b/src/compiler/apply-phase-guarded.ts @@ -0,0 +1,55 @@ +/** + * Shared `$$phase`-guarded `$apply` / `$evalAsync` dispatch. + * + * A native event (or any framework callback firing outside the digest + * machinery) must NOT call `scope.$apply` while a digest is already in + * flight — `$apply` would throw `'$digest already in progress'`. When + * `scope.$$phase` is non-null the runner is queued through + * `scope.$evalAsync` instead; a throw from the drained runner is then + * covered by the digest's standard `'$evalAsync'` catch path. On the + * common no-phase path the runner goes through `scope.$apply`, and + * because this project's `$apply` is `try/finally`-only (no internal + * `try/catch` — see `src/core/scope.ts`), the synchronous throw is + * caught HERE and routed via `$exceptionHandler` under the caller's + * cause token. + * + * Callers and their cause tokens (the asymmetry is test-pinned — see + * `spec026-parity.test.ts`): + * + * - the spec-026 event directives (`ng-event-directives.ts`) pass + * `'eventListener'`; + * - the forms surfaces (`input-types.ts` via `applyDuringEvent`, + * `form.ts` submit, `select.ts` change) pass `'$compile'`. + * + * The `$timeout` / `$interval` services implement the same idea through + * injected `apply` / `rootPhase` seams (pure factories with no `Scope` + * dependency) — deliberately NOT unified with this helper. + * + * Compiler-internal shared helper (the `expression-assign.ts` / + * `element-slots.ts` precedent) — not exported from `@compiler/index`. + */ + +import type { Scope } from '@core/index'; + +import { invokeExceptionHandler, type ExceptionHandler, type ExceptionHandlerCause } from '@exception-handler/index'; + +/** + * Dispatch `run` through the `$$phase`-guarded `$apply` / `$evalAsync` + * seam, routing any synchronous throw via `$exceptionHandler(cause)`. + */ +export function applyPhaseGuarded( + scope: Scope, + exceptionHandler: ExceptionHandler, + cause: ExceptionHandlerCause, + run: () => void, +): void { + try { + if (scope.$$phase !== null) { + scope.$evalAsync(run); + } else { + scope.$apply(run); + } + } catch (err) { + invokeExceptionHandler(exceptionHandler, err, cause); + } +} diff --git a/src/compiler/compile-error.ts b/src/compiler/compile-error.ts index 2fb5954..cfd0ecb 100644 --- a/src/compiler/compile-error.ts +++ b/src/compiler/compile-error.ts @@ -674,7 +674,7 @@ export class TemplateFetchFailedError extends Error { * Routed via `$exceptionHandler('$compile')` from * {@link import('./isolate-bindings').wireIsolateBindings}. No new * `EXCEPTION_HANDLER_CAUSES` token — `'$compile'` is reused; the tuple - * stays at 10. The message names BOTH the missing source attribute and + * is unchanged. The message names BOTH the missing source attribute and * the owning directive / component so the author can locate the unbound * input. * @@ -933,7 +933,7 @@ function describeRepeatItem(item: unknown) { * through `$exceptionHandler('$compile')` — NOT through the digest's * `'watchListener'` path, because the directive captures the throw * before the watcher's caller does. No new `EXCEPTION_HANDLER_CAUSES` - * cause token; the tuple stays at 10. + * cause token; the tuple is unchanged. * * The list does not render until the author resolves the duplicate; * the rows from the previous (valid) state are torn down by the @@ -974,7 +974,7 @@ export class NgRepeatDuplicateKeyError extends Error { * valid authoring choice (offset 0), but a PRESENT offset that cannot * be parsed is always an authoring mistake. * - * No new `EXCEPTION_HANDLER_CAUSES` token — the tuple stays at 10. + * No new `EXCEPTION_HANDLER_CAUSES` token — the tuple is unchanged. * * @example * ```ts @@ -1010,7 +1010,7 @@ export class NgPluralizeBadOffsetError extends Error { * digest, and never for NaN counts (an unusable count blanks the * element silently per FS §2.8). * - * No new `EXCEPTION_HANDLER_CAUSES` token — the tuple stays at 10. + * No new `EXCEPTION_HANDLER_CAUSES` token — the tuple is unchanged. * * @example * ```ts @@ -1040,7 +1040,7 @@ export class NgPluralizeNoRuleDefinedError extends Error { * `$exceptionHandler('$compile')` at link time and goes inert — it * publishes nothing and installs no destroy listener. * - * No new `EXCEPTION_HANDLER_CAUSES` token — the tuple stays at 10. + * No new `EXCEPTION_HANDLER_CAUSES` token — the tuple is unchanged. * * @example * ```ts @@ -1081,7 +1081,7 @@ export class NgRefBadExpressionError extends Error { * `ng-ref-read` attribute) AND the element's tag name so the author can * locate the offending element and the unmatched controller request. * - * No new `EXCEPTION_HANDLER_CAUSES` token — the tuple stays at 10. + * No new `EXCEPTION_HANDLER_CAUSES` token — the tuple is unchanged. * * @example * ```ts @@ -1112,7 +1112,7 @@ export class NgRefNoControllerError extends Error { * grouping pass; the DOM is left UNTOUCHED (no capture, no node removal) * so an unterminated range never leaves a half-grouped tree behind. No * new `EXCEPTION_HANDLER_CAUSES` cause token — `'$compile'` is reused; - * the tuple stays at 10. + * the tuple is unchanged. * * The message names BOTH the unmatched `-start` attribute and the * `-end` attribute that was expected, so the author can locate the diff --git a/src/compiler/compile-provider.ts b/src/compiler/compile-provider.ts index 1c2343f..b86aa2e 100644 --- a/src/compiler/compile-provider.ts +++ b/src/compiler/compile-provider.ts @@ -545,7 +545,7 @@ export class $CompileProvider { * (`InvalidIsolateBindingError`, `InvalidControllerFactoryError`, …) * still route lazily via `$exceptionHandler('$compile')` at provider * `$get` time through the existing factory `try/catch` — - * `EXCEPTION_HANDLER_CAUSES` stays at 10. + * `EXCEPTION_HANDLER_CAUSES` is unchanged. * * Returns `this` for chaining. * diff --git a/src/compiler/compile.ts b/src/compiler/compile.ts index 97f331f..2d8e30b 100644 --- a/src/compiler/compile.ts +++ b/src/compiler/compile.ts @@ -108,9 +108,9 @@ import { collectDirectives } from './directive-collector'; import { isNgManagedElement, NG_BOUND_TRANSCLUDE, - NG_CONTROLLERS, NG_ELEMENT_TRANSCLUDED, NG_SCOPE, + stashController, } from './element-slots'; import type { CompileOptions, CompileService, Directive, Linker, LinkFn, Attributes } from './directive-types'; import { wireIsolateBindings, type NormalizedBindingMap } from './isolate-bindings'; @@ -300,31 +300,6 @@ function isAttributeSourceController(controller: unknown): controller is { __att ); } -/** - * Stash `instance` on `element.$$ngControllers` under `directiveName`. - * Creates the map lazily on first call. Non-enumerable so it does not - * appear in `for..in` traversals. - * - * Slice 4 will add the READ path (the `^` / `^^` ancestor walk for - * `require`); Slice 3 only writes. - */ -function stashController(element: Element, directiveName: string, instance: unknown): void { - let map: Map | undefined; - if (isNgManagedElement(element)) { - map = element[NG_CONTROLLERS]; - } - if (map === undefined) { - map = new Map(); - Object.defineProperty(element, NG_CONTROLLERS, { - value: map, - writable: true, - configurable: true, - enumerable: false, - }); - } - map.set(directiveName, instance); -} - /** * Object-form `require` auto-assignment (spec 022 Slice 4). When a * directive declares `require: { alias: '^name', … }` AND its own diff --git a/src/compiler/directive-types.ts b/src/compiler/directive-types.ts index 8c638b5..f6e5f03 100644 --- a/src/compiler/directive-types.ts +++ b/src/compiler/directive-types.ts @@ -891,7 +891,7 @@ export interface CompileOptions { * * Throws from the controller constructor route via * `invokeExceptionHandler(exceptionHandler, err, '$compile')` — no - * new `EXCEPTION_HANDLER_CAUSES` entry; the tuple stays at 10. + * new `EXCEPTION_HANDLER_CAUSES` entry; the tuple is unchanged. */ readonly controller: ControllerService; /** diff --git a/src/compiler/lifecycle.ts b/src/compiler/lifecycle.ts index c299f9b..894ed20 100644 --- a/src/compiler/lifecycle.ts +++ b/src/compiler/lifecycle.ts @@ -22,7 +22,7 @@ * Slice 2. Failures inside a hook route via * `invokeExceptionHandler(handler, err, '$compile')`; the `'$compile'` * cause token is reused (no new `EXCEPTION_HANDLER_CAUSES` entry — the - * tuple stays at 10). + * tuple is unchanged). * * The `$onChanges` queue is a small per-`$compile`-call (effectively * per-runtime) structure keyed by controller instance. The COMPILER diff --git a/src/compiler/ng-attribute-aliases.ts b/src/compiler/ng-attribute-aliases.ts index 9444268..8092ce8 100644 --- a/src/compiler/ng-attribute-aliases.ts +++ b/src/compiler/ng-attribute-aliases.ts @@ -45,7 +45,7 @@ * - No new error classes. No new `EXCEPTION_HANDLER_CAUSES` token. * Errors thrown from `$observe` listeners (URL pattern) or `$watch` * listeners (boolean pattern) flow through the existing - * `'watchListener'` / `'$evalAsync'` causes — the tuple stays at 10. + * `'watchListener'` / `'$evalAsync'` causes — the tuple is unchanged. * - Priority is LOAD-BEARING and DIFFERS between the two patterns: * URL aliases use priority 99, boolean aliases use priority 100. * Matches AngularJS-canonical exactly. diff --git a/src/compiler/ng-controller.ts b/src/compiler/ng-controller.ts index cc03e43..035021c 100644 --- a/src/compiler/ng-controller.ts +++ b/src/compiler/ng-controller.ts @@ -76,7 +76,7 @@ * was never registered, `InvalidControllerFactoryError` on a malformed * registry entry, etc.) via `$exceptionHandler('$compile')` through * the existing factory `try/catch` in the seam. The rest of the page - * does not crash. `EXCEPTION_HANDLER_CAUSES` stays at 10. + * does not crash. `EXCEPTION_HANDLER_CAUSES` is unchanged. * * The factory is array-form (`[() => ({...})]`) because the project's * `annotate` helper rejects bare functions without `$inject` — this is diff --git a/src/compiler/ng-event-directives.ts b/src/compiler/ng-event-directives.ts index da6b5a4..079f79b 100644 --- a/src/compiler/ng-event-directives.ts +++ b/src/compiler/ng-event-directives.ts @@ -46,7 +46,8 @@ * exception handler. The wrapper preserves the "log and continue" * contract: a buggy handler reports through the configured * `$exceptionHandler` and subsequent events still fire correctly. - * `EXCEPTION_HANDLER_CAUSES.length` stays at 10 — no new token. + * The event directives add no new `EXCEPTION_HANDLER_CAUSES` token — + * they reuse the existing `'eventListener'` entry. * * **Cleanup.** Each link fn registers a `scope.$on('$destroy', …)` * listener that removes the native event listener when the scope @@ -95,8 +96,9 @@ */ import { parse } from '@parser/index'; -import { invokeExceptionHandler, type ExceptionHandler } from '@exception-handler/index'; +import type { ExceptionHandler } from '@exception-handler/index'; +import { applyPhaseGuarded } from './apply-phase-guarded'; import type { DirectiveFactory, DirectiveFactoryReturn, LinkFn } from './directive-types'; /** @@ -162,7 +164,7 @@ function capitalize(name: EventName): string { * `try/catch` around the parsed expression's invocation can route via * `invokeExceptionHandler(..., 'eventListener')`. The cause token is * the existing 6th entry of `EXCEPTION_HANDLER_CAUSES` — no new token - * is introduced (the tuple stays at 10). + * is introduced (the tuple is unchanged). * * The compile fn parses the bound expression ONCE; the link fn * registers `element.addEventListener(eventName, handler)` and a @@ -199,28 +201,14 @@ function createEventDirective(eventName: EventName): DirectiveFactory { const run = () => { parsed(scope, { $event: event }); }; - try { - if (scope.$$phase !== null) { - // Nested-event path — a digest is already in flight - // (e.g. one ng-click fired this synchronously during - // another ng-click's $apply). Queue through - // $evalAsync; the digest's standard '$evalAsync' - // catch path covers any throw from the drained - // expression. - scope.$evalAsync(run); - } else { - // Common path — no phase active. $apply runs `run` - // synchronously and then triggers $root.$digest() so - // any scope mutation propagates. The framework's - // $apply has no internal try/catch, so a throw from - // `run` would otherwise propagate out of - // dispatchEvent — the outer try/catch routes it - // through $exceptionHandler instead. - scope.$apply(run); - } - } catch (err) { - invokeExceptionHandler($exceptionHandler, err, 'eventListener'); - } + // Nested-event path (a digest already in flight — e.g. one + // ng-click fired this synchronously during another + // ng-click's $apply) queues through $evalAsync, so a throw + // from the drained expression routes via the digest's + // standard '$evalAsync' catch path; the common no-phase + // path runs through $apply with a synchronous throw routed + // via 'eventListener'. See `apply-phase-guarded.ts`. + applyPhaseGuarded(scope, $exceptionHandler, 'eventListener', run); }; element.addEventListener(eventName, handler); scope.$on('$destroy', () => { diff --git a/src/compiler/ng-if.ts b/src/compiler/ng-if.ts index e9003c4..cec40aa 100644 --- a/src/compiler/ng-if.ts +++ b/src/compiler/ng-if.ts @@ -78,7 +78,7 @@ * **Errors.** None new. A throwing `$watch` listener routes via the * digest's existing `'watchListener'` cause; `$transclude` errors * route via the existing `'$compile'` cause through the spec-018 - * transclusion runtime. `EXCEPTION_HANDLER_CAUSES` stays at 10. + * transclusion runtime. `EXCEPTION_HANDLER_CAUSES` is unchanged. * * The factory is array-form (`[() => ({...})]`) because the project's * `annotate` helper rejects bare functions without `$inject` — this diff --git a/src/compiler/ng-include.ts b/src/compiler/ng-include.ts index b879409..2615c74 100644 --- a/src/compiler/ng-include.ts +++ b/src/compiler/ng-include.ts @@ -126,7 +126,7 @@ * of the promise chain and is collected by the standard * unhandled-rejection surface; consumers who care about `onload` * errors should wrap them in their own try/catch inside the - * expression. The tuple stays at 10. + * expression. The tuple is unchanged. * * @example Attribute form * ```html diff --git a/src/compiler/ng-pluralize.ts b/src/compiler/ng-pluralize.ts index 52858d2..4c9f01f 100644 --- a/src/compiler/ng-pluralize.ts +++ b/src/compiler/ng-pluralize.ts @@ -104,7 +104,7 @@ * via `$log.debug`; this project ships no `$log` service, so the * report routes through the standard `$exceptionHandler` channel with * the existing `'$compile'` cause token (`EXCEPTION_HANDLER_CAUSES` - * stays at 10). + * is unchanged). * * **`when-…` attribute scan rule (FS §2.7 / Slice 4).** The * enumerable keys of `attrs` are matched against upstream's diff --git a/src/compiler/ng-ref.ts b/src/compiler/ng-ref.ts index 3c1008c..718876e 100644 --- a/src/compiler/ng-ref.ts +++ b/src/compiler/ng-ref.ts @@ -61,7 +61,7 @@ * err, '$compile')` and makes the directive INERT — it publishes * nothing and installs no destroy listener. This is the FS criterion * for `ng-ref="123bad"`. No new `EXCEPTION_HANDLER_CAUSES` token; the - * tuple stays at 10. + * tuple is unchanged. * * **Surrounding-scope publish (upstream parity).** `ngRef` is a * non-isolate directive, so it publishes to the element's SURROUNDING diff --git a/src/compiler/ng-switch.ts b/src/compiler/ng-switch.ts index 3d98a06..f57e8ea 100644 --- a/src/compiler/ng-switch.ts +++ b/src/compiler/ng-switch.ts @@ -88,7 +88,7 @@ * spec-022 mechanism. * * **Errors.** No new error classes. No new `EXCEPTION_HANDLER_CAUSES` - * token. The tuple stays at 10. Every error site reuses existing + * token. The tuple is unchanged. Every error site reuses existing * surfaces: `MissingRequiredControllerError` (children without parent), * `'watchListener'` (a throwing `$watch` listener inside the parent), * `'$compile'` (throws inside `$transclude` invocations). diff --git a/src/compiler/require-resolver.ts b/src/compiler/require-resolver.ts index 27ec5d7..21d3fba 100644 --- a/src/compiler/require-resolver.ts +++ b/src/compiler/require-resolver.ts @@ -16,7 +16,7 @@ * A non-optional miss throws {@link MissingRequiredControllerError}, * which the per-element link site routes via * `$exceptionHandler('$compile')` — no new `EXCEPTION_HANDLER_CAUSES` - * token; the tuple stays at 10. + * token; the tuple is unchanged. * * Pure helpers — no DI access, no exception-handler routing, no element * cleanup. The compiler imports {@link resolveRequireForm} and drives it diff --git a/src/controller/__tests__/controller-di.test.ts b/src/controller/__tests__/controller-di.test.ts index 668ba00..f13e002 100644 --- a/src/controller/__tests__/controller-di.test.ts +++ b/src/controller/__tests__/controller-di.test.ts @@ -18,7 +18,7 @@ * and mirror AngularJS 1.x. Slice 4's `controller-compile.test.ts` covers * the compile-time side. * - * The `EXCEPTION_HANDLER_CAUSES.length === 10` assertion at the bottom is + * The `EXCEPTION_HANDLER_CAUSES.length` assertion at the bottom is * a regression lock — Slice 3 introduces no new cause token. */ @@ -237,7 +237,7 @@ describe('EXCEPTION_HANDLER_CAUSES regression (no new cause token in Slice 3)', it('EXCEPTION_HANDLER_CAUSES.length === 13 (no controller-spec token; grew to 13 in spec 037)', () => { // Spec 020 reuses the existing `'$compile'` cause token (added in // spec 017) for every controller-related error site at link time. - // The tuple stays at 10 entries across Slices 1-5; lock that in + // The tuple gained no controller token; lock the current length in // here so a future drive-by addition surfaces an obvious failure. expect(EXCEPTION_HANDLER_CAUSES.length).toBe(13); }); diff --git a/src/controller/__tests__/controller-parity.test.ts b/src/controller/__tests__/controller-parity.test.ts index d2dfeb7..87a6e41 100644 --- a/src/controller/__tests__/controller-parity.test.ts +++ b/src/controller/__tests__/controller-parity.test.ts @@ -257,7 +257,7 @@ describe('EXCEPTION_HANDLER_CAUSES regression (no new cause token in Slice 5)', it('EXCEPTION_HANDLER_CAUSES.length === 13 (no controller-spec token; grew to 13 in spec 037)', () => { // Spec 020 reuses the existing `'$compile'` cause token (added in // spec 017) for every controller-related error site at link time. - // The tuple stays at 10 entries; lock that in here so a future + // The tuple gained no controller token; lock the current length in here so a future // drive-by addition surfaces an obvious failure. expect(EXCEPTION_HANDLER_CAUSES.length).toBe(13); }); diff --git a/src/controller/controller-errors.ts b/src/controller/controller-errors.ts index 0c7b680..51549af 100644 --- a/src/controller/controller-errors.ts +++ b/src/controller/controller-errors.ts @@ -15,7 +15,7 @@ * time). Compile-time invocations of `$controller(...)` are wrapped by * the spec-020 Slice 4 seam in a `try/catch` that routes through * `$exceptionHandler('$compile')` — no new `EXCEPTION_HANDLER_CAUSES` - * entry is added (the tuple stays at 10). + * entry is added (the tuple is unchanged). */ /** diff --git a/src/core/__tests__/utils.test.ts b/src/core/__tests__/utils.test.ts index afeb7a9..7888b5b 100644 --- a/src/core/__tests__/utils.test.ts +++ b/src/core/__tests__/utils.test.ts @@ -1328,7 +1328,7 @@ describe('copy', () => { describe('noop', () => { it('returns undefined', () => { - // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression + // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression -- asserting noop's void return IS the test; the "confusing" expression is deliberate expect(noop()).toBe(undefined); }); diff --git a/src/core/utils.ts b/src/core/utils.ts index a4be679..496ba03 100644 --- a/src/core/utils.ts +++ b/src/core/utils.ts @@ -88,7 +88,7 @@ export function isBoolean(value: unknown): value is boolean { return typeof value === 'boolean'; } -// eslint-disable-next-line @typescript-eslint/no-unsafe-function-type +// eslint-disable-next-line @typescript-eslint/no-unsafe-function-type -- a type guard must accept ANY callable; the broad `Function` type is the point export function isFunction(value: unknown): value is Function { return typeof value === 'function'; } @@ -273,7 +273,7 @@ function copyRecursive(source: T, destination: T | undefined, visited: Set if (destination !== undefined) { throw new Error('Cannot copy! TypedArray destination is not supported.'); } - // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-call + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-call -- the concrete TypedArray subclass is only knowable at runtime via `source.constructor`; the single sanctioned `any` cast in the codebase (see CLAUDE.md) return new (source.constructor as any)(source.buffer.slice(0)) as T; } @@ -301,7 +301,7 @@ function copyRecursive(source: T, destination: T | undefined, visited: Set // Clear existing own properties on destination if (destination !== undefined) { for (const key of Object.keys(result)) { - // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete -- copy-into-destination must clear pre-existing own keys; the keys are runtime data, not a fixed shape delete result[key]; } } diff --git a/src/forms/__tests__/select.test.ts b/src/forms/__tests__/select.test.ts index d4b96ce..bf9599c 100644 --- a/src/forms/__tests__/select.test.ts +++ b/src/forms/__tests__/select.test.ts @@ -9,13 +9,14 @@ * `change` event. Vectors ported from AngularJS `selectSpec.js`. */ -import { afterEach, describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { asOption, asSelect } from '@compiler/__tests__/dom-guards'; import type { CompileService } from '@compiler/directive-types'; import type { Scope } from '@core/index'; import { bootstrapInjector } from '@bootstrap/index'; import { resetRegistry } from '@di/module'; +import { NgModelControllerImpl } from '@forms/ng-model-controller'; interface Harness { $compile: CompileService; @@ -222,3 +223,44 @@ describe('option — interpolated value="{{…}}" (PR-audit regression)', () => expect(select.value).toBe('b'); }); }); + +// ──────────────────────────────────────────────────────────────────────────── +// Audit regression — a change-commit throw routes via $exceptionHandler +// ──────────────────────────────────────────────────────────────────────────── + +describe('select — change-commit throw routes via $exceptionHandler (audit regression)', () => { + it('a throwing $parser during the native change commit reports instead of escaping the listener', () => { + // The default `$exceptionHandler` is `consoleErrorExceptionHandler`, + // so spying `console.error` is the observable proxy for the routing + // contract (the `ng-model.test.ts` precedent). Before the shared + // `applyPhaseGuarded` adoption, `selectCtrl.applyChange` lacked the + // try/catch and the throw escaped the native `change` listener + // without ever reaching the handler. + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const { $compile, $rootScope } = boot(); + const select = compile( + '', + $compile, + $rootScope, + ); + + const map = (select as unknown as { $$ngControllers?: Map }).$$ngControllers; + const ctrl = map?.get('ngModel'); + if (!(ctrl instanceof NgModelControllerImpl)) { + throw new Error('ngModel controller not found on select element'); + } + ctrl.$parsers.push(() => { + throw new Error('parser boom'); + }); + + select.value = 'green'; + expect(() => { + fireChange(select); + }).not.toThrow(); + expect(consoleSpy).toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); +}); diff --git a/src/forms/form.ts b/src/forms/form.ts index eadcf98..7413d70 100644 --- a/src/forms/form.ts +++ b/src/forms/form.ts @@ -57,11 +57,12 @@ * `injector.get('ngFormDirective')`, NOT exported from the root barrel. */ +import { applyPhaseGuarded } from '@compiler/apply-phase-guarded'; import { buildParentWriter } from '@compiler/expression-assign'; import { stashController } from '@compiler/element-slots'; import type { Attributes, DirectiveFactory, DirectiveFactoryReturn, LinkFn } from '@compiler/directive-types'; import type { ControllerInvokable } from '@controller/controller-types'; -import { invokeExceptionHandler, type ExceptionHandler } from '@exception-handler/index'; +import type { ExceptionHandler } from '@exception-handler/index'; import { parse } from '@parser/index'; import { FormControllerImpl, nullFormCtrl, type FormController } from './form-controller'; @@ -178,18 +179,9 @@ function buildFormFactory(ownName: string): DirectiveFactory { if (!hasAction) { event.preventDefault(); } - const run = () => { + applyPhaseGuarded(scope, $exceptionHandler, '$compile', () => { own.$setSubmitted(); - }; - try { - if (scope.$$phase !== null) { - scope.$evalAsync(run); - } else { - scope.$apply(run); - } - } catch (err) { - invokeExceptionHandler($exceptionHandler, err, '$compile'); - } + }); }; element.addEventListener('submit', onSubmit); diff --git a/src/forms/input-types.ts b/src/forms/input-types.ts index 09ab94f..0112eb7 100644 --- a/src/forms/input-types.ts +++ b/src/forms/input-types.ts @@ -31,8 +31,9 @@ import { asInstanceOf, type Scope } from '@core/index'; +import { applyPhaseGuarded } from '@compiler/apply-phase-guarded'; import type { Attributes } from '@compiler/directive-types'; -import { invokeExceptionHandler, type ExceptionHandler } from '@exception-handler/index'; +import type { ExceptionHandler } from '@exception-handler/index'; import { parse } from '@parser/index'; import { formatDateInput, parseDateInput, type DateInputKind } from './input-date'; @@ -58,20 +59,13 @@ export type InputTypeHandler = (ctx: InputTypeContext) => void; /** * Dispatch a runner through the `$$phase`-guarded `$apply` / `$evalAsync` - * seam, routing any throw via `$exceptionHandler('$compile')`. Mirrors the - * spec-026 event-directive workaround (the project's `$apply` is + * seam, routing any throw via `$exceptionHandler('$compile')`. Thin + * forms-cause binding of the shared {@link applyPhaseGuarded} helper + * (the spec-026 event-directive workaround — the project's `$apply` is * `try/finally`, not `try/catch`). */ export function applyDuringEvent(scope: Scope, exceptionHandler: ExceptionHandler, run: () => void): void { - try { - if (scope.$$phase !== null) { - scope.$evalAsync(run); - } else { - scope.$apply(run); - } - } catch (err) { - invokeExceptionHandler(exceptionHandler, err, '$compile'); - } + applyPhaseGuarded(scope, exceptionHandler, '$compile', run); } /** diff --git a/src/forms/select.ts b/src/forms/select.ts index c3fad10..da91187 100644 --- a/src/forms/select.ts +++ b/src/forms/select.ts @@ -41,10 +41,12 @@ * exported from the root barrel. */ +import { applyPhaseGuarded } from '@compiler/apply-phase-guarded'; import { stashController } from '@compiler/element-slots'; import type { DirectiveFactory, DirectiveFactoryReturn, LinkFn } from '@compiler/directive-types'; import type { ControllerInvokable } from '@controller/controller-types'; import { asInstanceOf } from '@core/index'; +import type { ExceptionHandler } from '@exception-handler/index'; import { NgModelControllerImpl } from './ng-model-controller'; @@ -273,7 +275,7 @@ function asNgModelFromTuple(controllers: unknown): NgModelControllerImpl | null return null; } -function selectFactory(): DirectiveFactoryReturn { +function selectFactory($exceptionHandler: ExceptionHandler): DirectiveFactoryReturn { // Array-annotated so `injector.invoke` resolves `$element` by name. const controller: ControllerInvokable = [ '$element', @@ -304,12 +306,12 @@ function selectFactory(): DirectiveFactoryReturn { } selectCtrl.ngModelCtrl = modelCtrl; + // `$$phase`-guarded dispatch with a throw (e.g. a throwing $parser + // during the change commit) routed via `$exceptionHandler('$compile')` + // — the shared forms/event-directive workaround for `$apply` being + // `try/finally`-only. selectCtrl.applyChange = (run: () => void) => { - if (scope.$$phase !== null) { - scope.$evalAsync(run); - } else { - scope.$apply(run); - } + applyPhaseGuarded(scope, $exceptionHandler, '$compile', run); }; // Model → view: render the selection from the model value. @@ -350,10 +352,11 @@ function selectFactory(): DirectiveFactoryReturn { } /** - * DI-annotated `select` directive. Zero deps — the controller reads only - * `$element`. Registered on `ngModule` via `forms-register.ts`. + * DI-annotated `select` directive. `$exceptionHandler` backs the + * `applyChange` throw routing; the controller reads only `$element`. + * Registered on `ngModule` via `forms-register.ts`. */ -export const selectDirective: DirectiveFactory = [selectFactory]; +export const selectDirective: DirectiveFactory = ['$exceptionHandler', selectFactory]; /** * The `option` directive — plain markup `