Research/upgrade ch5 for solid#276
Open
nbrar-crestron wants to merge 9 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>
Companion analysis to MIGRATION.md, capturing the Crestron#1 critical perf finding from the architecture review: every list-family component (ch5-list, ch5-button-list, ch5-subpage-reference-list, ch5-spinner) renders every item to the DOM regardless of viewport. The document covers: • why it hurts specifically on panel hardware (vs. desktop) • what windowing-with-recycling looks like for CH5 • the four hard parts (variable heights, focus/gesture, signal rebinding, behavioural parity) • proposed Phase 2 / Phase 3 sequencing • a pre-flight checklist of questions to resolve with the team before any code gets written • how this work builds on Phase 1's shared observers, leak fix, and Jest harness Intended as a starting point for team review of what Phase 2 should look like. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Third in the architecture-review doc series, after MIGRATION.md (Phase 1 inventory) and LIST_VIRTUALIZATION.md (Phase 2 case for list family). Compares <ch5-button> per-instance runtime cost against Bootstrap's .btn, targeted at the Crestron TSW-770 panel. Hard numbers, cited to file:line in src/ch5-button/ and src/ch5-common/, with the asymmetry between the two components honestly acknowledged: Bootstrap is mostly CSS; ch5-button is a reactive component bound to the signal bus. Findings: • DOM nodes per button: 1 (Bootstrap) vs. 6–9 (ch5) • Heap objects on init: 0–1 vs. 20–30 • Native listeners: 0–1 delegated vs. 8–10 per instance • Browser observers: 0 vs. 2 per instance on master • Signal subscriptions: 0 vs. 10+ per instance • Memory: < 1 KB vs. 15–25 KB per instance Identifies the three worst hot spots (full classList enumeration on every render; per-instance MutationObserver+ResizeObserver; innerHTML='' rebuilds on label change) and proposes seven improvements ranked by ROI, plus a sequencing plan that lands the biggest wins inside ch5-button-base.ts before the Phase 2 maintainability refactor. Includes an "Open questions for the team" section: visual parity, mode-state churn rate in real projects, signal-subscription coverage, and pressable semantics — all worth resolving before any PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fourth in the architecture-review doc series, after MIGRATION.md,
LIST_VIRTUALIZATION.md, and BUTTON_PERFORMANCE.md.
Picks up the open question from BUTTON_PERFORMANCE.md: can ch5-button
allocate only what its authored markup actually asks for, rather than
spinning up the full 20–30 object / 10+ signal-subscription surface on
every instance?
Distinguishes two complementary techniques:
A. Runtime-lazy composition — a feature registry where each
capability (label, receiveStateLabel, sendEventOnClick, mode,
pressable, …) is a small installable unit triggered by the
presence of its attribute.
B. Build-time pruning — the HTML5 UI Builder already sees every
<ch5-button> in every project; it can emit a manifest of used
capabilities and tree-shake the bundle accordingly.
Includes:
• a concrete TypeScript sketch of the feature-registry pattern,
• a side-by-side pros table covering memory, observers, signal
subs, bundle size, render time, authoring compatibility,
• seven cons / risks with mitigations,
• CH5-specific considerations (toolchain as a unique asset; Lit /
FAST validation of the pattern),
• a falsifiable two-week proof-of-concept plan that ships against
one capability before committing to the full refactor,
• open questions for the team and links back to the rest of the
doc series.
Intended as a design-discussion starting point. No code shipped.
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.