Research/upgrade ch5 for solid#276
Open
nbrar-crestron wants to merge 6 commits into
Open
Conversation
Adds Jest 29 + ts-jest + jest-environment-jsdom as a new test runner that lives next to the existing mocha (.spec.ts) and WCT (HTML) suites. New behavioural / before-after tests for the SOLID upgrade go in tests/jest/. npm run test:jest # one-shot npm run test:jest:watch # watch mode npm run test:jest:coverage # with coverage Jest's testMatch picks up only tests/jest/**/*.test.ts and explicitly ignores src/**/*.spec.ts, so the mocha suite is unaffected. tsconfig.umd.json gains an exclude for tests/ — its prior **/*.ts include was greedy enough to pull the new Jest tests into the library build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on guard src/ch5-list/ch5-list.ts:508 had setTimeout(..., 0.5) — almost certainly a typo for 500. A 0.5 ms delay collapses to the next macrotask, defeating the debounce and forcing a synchronous resizeList() on every viewport-change event. Adds tests/jest/perf/short-timeout.test.ts, a regression guard that walks every src/*.ts via the TypeScript compiler API and flags any setTimeout / setInterval call whose delay is a numeric literal in (0, 16) ms. The threshold is one frame at 60Hz. delay === 0 is permitted as the standard "yield to next macrotask" idiom. Supporting helpers: tests/jest/_helpers/source-scanner.ts — file walker + line/col resolver tests/jest/_helpers/ts-call-finder.ts — AST-based call-site finder The AST approach handles nested calls, multi-line arguments, and comments correctly — a regex pass over the same code missed multi-line setTimeout and over-matched on inner literals. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… singleton Adds src/ch5-core/ch5-shared-mutation-observer.ts — a singleton pool that wraps a single browser-level MutationObserver and dispatches per-target callbacks. Modelled on the existing Ch5CoreIntersectionObserver pattern. Refactors src/ch5-common/ch5-mutation-observer.ts to be a thin facade that delegates to the pool. The constructor, observe(), disconnectObserver(), isConnected field, ELEMENTS_MO_EXCEPTION static, and checkElementValidity static are preserved exactly — so the 35+ existing call sites need no changes. Net effect: N components that each used to construct their own MutationObserver now share ONE underlying observer. Proven by tests/jest/perf/mutation-observer-facade.test.ts (50 facade instances → 1 ctor call). Dispatch correctly handles subtree:true configurations by walking up mutation.target's ancestor chain to find a matching registered node. Test coverage: shared-mutation-observer.test.ts 7 tests — pool invariants mutation-observer-facade.test.ts 5 tests — back-compat + isolation Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ility
Adds src/ch5-core/ch5-shared-resize-observer.ts — a singleton pool that
wraps a single browser ResizeObserver and routes entries to per-target
callbacks. Includes feature-detection so it's a no-op on platforms
without native ResizeObserver.
Refactors src/ch5-core/resize-observer.ts. The historical utility
created a new ResizeObserver per call AND returned void — every caller
leaked one observer permanently with no way to disconnect. It now:
• delegates to Ch5SharedResizeObserver (one underlying observer)
• returns a disposer so opt-in callers can finally clean up
The 11 existing callers that ignore the return value still work and
still benefit from pooling. Two direct consumers are migrated to use
the disposer in their removeEventListeners() path:
src/ch5-video-switcher/ch5-video-switcher.ts
src/ch5-signal-level-gauge/ch5-signal-level-gauge.ts
Test coverage (9 tests):
shared-resize-observer.test.ts
• N targets → 1 underlying ResizeObserver
• per-target dispatch
• disposer unregisters exactly that target
• teardown on empty
• graceful no-op when ResizeObserver is unavailable
• legacy utility routes through the pool
• legacy utility returns a working disposer
• legacy utility still works for callers that ignore the disposer
Supporting helper:
tests/jest/_helpers/fake-resize-observer.ts — controllable shim for JSDOM
(JSDOM does not implement ResizeObserver natively)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…init failures
Two long-standing bugs that the test scaffolding now guards against.
1. Language-change subscription leak (src/ch5-common/ch5-common.ts)
The Ch5Common constructor subscribed to the language-change signal and
dropped the returned subscription key on the floor. The subscription
closure captured `this` (the component) AND `this.translatableObjects`,
so every component instance was pinned in memory for the lifetime of
the page — even after removal from the DOM.
Fix: track the returned key in `_languageChangeSubKey` and release it
in `unsubscribeFromSignals()` via `signal.unsubscribe(key)`. Always
released — the `_keepListeningOnSignalsAfterRemoval` flag is for
receive-state bridge subs used for re-attachment; the language sub
must not be kept alive.
Also adds `_baseClassSubscriptions: Subscription[]` as future
infrastructure for any non-bridge RxJS subscriptions a base or
subclass method might register. Drained alongside the language sub.
2. Silent catches in ColorPicker (src/ch5-color-picker/color-picker.ts)
The constructor's try/catch swallowed init failures into `joe = null`
with no log. setColor() then silently no-op'd on the null. QA and
field engineers had nothing to grep for when the picker didn't render.
Fix:
• catches now console.warn with the picker id and the underlying error
• renamed private `joe` → `_picker` (descriptive; nothing external
referenced the old name)
• added `isReady` getter so callers can check before calling
• setColor() returns boolean (existing void-returning callers unaffected)
Test coverage:
language-subscription.test.ts 4 tests — bridge unsubscribe, drain
array, _keepListening flag,
no-op when nothing tracked
color-picker-silent-catch.test.ts 2 tests — init failure warns + isReady,
setColor warns on uninit
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
eslint:
• no-explicit-any: off → warn
• no-console: off → warn
Surfaces ~689 warnings across the existing source so future work can
chip away at them without blocking the build today.
package.json:
• removes the `lint` npm script — it referenced `tslint`, but no
tslint.json exists in the repo. eslint (script `eslint`) is the
real linter.
MIGRATION.md:
• developer-facing walkthrough of every change in this branch
• how to run the new Jest harness
• the new shared observer APIs and their migration path
• the bridge-vs-RxJS subscription pattern in Ch5Common
• the regression-guard tests and how to add new ones
• what is explicitly deferred to later phases and why
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An AI based review.