refactor(highway): flip highway.js to an ES module (R3c)#913
Merged
Conversation
Two lines. index.html: defer -> type="module". highway.js: one explicit assignment.
highway.js can now `import`, which is the whole point — the carve can begin.
━━━ THE ONE THING THE FLIP ACTUALLY BREAKS: window.createHighway ━━━
A top-level `function createHighway()` in a CLASSIC script IMPLICITLY becomes
window.createHighway. In a module it does not — module declarations are module-scoped, and
the name vanishes from the global object the instant the tag grows type="module".
The constitution names window.createHighway as PUBLIC EXTENSION CONTRACT (alongside
window.playSong / showScreen / feedBack). NOTHING IN-TREE CALLS IT. That is exactly why this
would have shipped: the only consumers are third-party plugins rendering their own highway
panel, and I cannot grep those. Green CI, green tests, and a broken plugin API.
Verified by removing the assignment and reloading:
flip WITHOUT an explicit assignment: window.createHighway === undefined <-- gone
flip WITH it: window.createHighway === function
So it is assigned explicitly now — same object, same behaviour, no longer an accident of how
the file happens to be loaded.
━━━ AND A CORRECTION TO #912 ━━━
#912 (merged) rewrote 73 bare `highway.x` -> `window.highway.x` on the stated grounds that
the flip would turn every one of them into a ReferenceError. HAVING NOW ACTUALLY FLIPPED IT,
THAT WAS WRONG. highway.js already did `window.highway = highway`, which puts the name on the
GLOBAL OBJECT — and bare-identifier resolution falls back to the global object whether or not
a lexical global binding exists. Measured on both builds: bare `highway` resolves either way.
#912 is defensible as hygiene and it does not hurt, but it was not a precondition and it fixed
no latent bug. A correction is posted on the PR so its commit message does not mislead. The
real hazard was the factory, not the instance — same class of breakage, wrong name.
ORDERING is unchanged: classic-defer and non-async type="module" share ONE post-parse
execution queue, in document order, so highway.js keeps its position at index.html:1244.
VERIFIED. A/B against origin/main: 15 probes IDENTICAL, zero page errors — window.highway,
window.createHighway, the full API surface, a real song playing, the chart clock advancing,
and the seek->setTime sync. THE PERF GATE PASSES at 1.85ms against its 12ms budget (module
evaluation costs nothing at render time), which is exactly what #910 was built to tell me.
node 1045, pytest 2416, ESLint 0, Codex 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
byrongamatos
added a commit
that referenced
this pull request
Jul 12, 2026
…ts.js (R3c) (#914) 29 constants, 190 lines. highway.js 4,267 -> 4,158. The first real slice, and the one that every later one imports. ━━━ WHY ONLY THE CONSTANTS MAY LIVE AT MODULE SCOPE ━━━ createHighway() is a FACTORY, not a singleton. The constitution publishes window.createHighway precisely so a plugin can build a SECOND highway for its own panel, and highway.js already says so at the top of the closure: // R3c: per-instance mutable state in one object, so extracted renderer/ws // modules can close over it as a factory arg without cross-panel sharing. So hwState — all 79 mutable properties — must NEVER become a module-level singleton: two highways would silently share it, and one panel would drive the other's clock, scale and colour tables. Extracted functions will take it as an ARGUMENT. That is the OPPOSITE of the app.js carve, where a single state container (player-state.js, library-state.js) was exactly right, because there is exactly one app. Same epic, same language, opposite answer — because one is a singleton and the other is a factory. These 29 are pure literals: numbers, strings and colour tables, never reassigned, never mutated. Sharing them across instances is not merely safe, it is what you want — one copy of the shimmer LUT bounds and the string palettes rather than one per panel. Anything with a runtime dependency (document, window, performance, localStorage) stays in the factory; checked, and none of these has one. ESLint now knows static/highway.js is a module. It could not have known before this commit: the flip (#913) changed the SCRIPT TAG, but the file had no import/export yet, so it still parsed as a script and lint stayed green. The first `import` is what makes the config wrong. TESTS. Four source-shape harnesses asserted `const _AUTO_SCALE_MIN = …` etc. lived in highway.js. They now read highway.js AND every static/js/highway-*.js — deliberately, rather than being re-pinned at whichever file currently holds a constant. Re-pinning just breaks again on the next carve, and a source-shape assertion that silently stops finding its target is indistinguishable from one that passes. Bite-tested: renaming two constants away fails them. VERIFIED. A/B against origin/main: 15 probes IDENTICAL, zero page errors. AND THE PERF GATE PASSES AT 1.97ms against its 12ms budget — which is the point of having built it (#910) first: these constants moved from closure scope to module scope, and V8 does not treat those identically. It does here. Now I know rather than hope. node 1045, pytest 2416, ESLint 0, Codex 0. Co-authored-by: Claude Opus 4.8 (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.
Two lines.
index.html:defer→type="module".highway.js: one explicit assignment.highway.js can now
import— which is the whole point. The carve can begin.The one thing the flip actually breaks:
window.createHighwayA top-level
function createHighway()in a classic script implicitly becomeswindow.createHighway. In a module it does not — module declarations are module-scoped, and the name vanishes from the global object the instant the tag growstype="module".The constitution names
window.createHighwayas public extension contract (alongsidewindow.playSong/showScreen/feedBack).Nothing in-tree calls it. That is exactly why this would have shipped: the only consumers are third-party plugins rendering their own highway panel, and I cannot grep those. Green CI, green tests, and a broken plugin API.
Verified by removing the assignment and reloading:
So it's assigned explicitly now — same object, same behaviour, no longer an accident of how the file happens to be loaded.
A correction to #912
#912 (merged) was over-sold, and I want that on the record.
It rewrote 73 bare
highway.x→window.highway.xon the stated grounds that the flip would turn every one of them into aReferenceError.Having now actually flipped it: that was wrong.
highway.jsalready didwindow.highway = highway, which puts the name on the global object — and bare-identifier resolution falls back to the global object whether or not a lexical global binding exists. Measured on both builds: barehighwayresolves either way.#912 is defensible as hygiene and it does no harm, but it was not a precondition and it fixed no latent bug. (Its "six half-converted guards" were bugs my own regex introduced and then repaired.) A correction is posted on that PR so its commit message doesn't mislead the next reader.
The real hazard was the factory, not the instance — same class of breakage, wrong name.
Ordering
Unchanged. Classic-
deferand non-asynctype="module"share one post-parse execution queue, in document order, so highway.js keeps its position atindex.html:1244.Verification
A/B against
origin/main— 15 probes identical, zero page errors:window.highway,window.createHighway, the full API surface, a real song playing, the chart clock advancing, and the seek →setTimesync.The perf gate passes at 1.85ms against its 12ms budget (module evaluation costs nothing at render time) — which is exactly what #910 was built to tell me.
node 1045 · pytest 2416 · ESLint 0 · Codex 0.
🤖 Generated with Claude Code