Skip to content

Fix 22 bugs, migrate to TypeScript, and modernise the toolchain - #251

Open
emshotton wants to merge 4 commits into
bbc:developfrom
emshotton:modernise-toolchain
Open

Fix 22 bugs, migrate to TypeScript, and modernise the toolchain#251
emshotton wants to merge 4 commits into
bbc:developfrom
emshotton:modernise-toolchain

Conversation

@emshotton

Copy link
Copy Markdown
Contributor

Hey! I'm the original author of this library, I was delighted to see it was still being maintained and used after I left the BBC. Looks like it's been a few years since it's been updated, so I thought I'd have a go at revisiting it with Claude Code to see what could be fixed up. This PR does a bunch of modernization, bug fixing, and adding of new tests with only a relatively minor breaking change to the API (VideoContext.exceptions are now Error subclasses).

This ended up being a bit of a monster of a PR, and I'd be happy to split it into more reasonable sized chunks if preferred.

Summary

This PR does three related pieces of work, verified end-to-end at each step:

  1. Fixes 22 bugs found in a full code review, each with a regression test
  2. Converts src/ to strict TypeScript (compiled by Babel, type-checked by tsc)
  3. Modernises the toolchain and packaging — Babel 7, Jest 29, ESLint 8, Prettier 3, TypeDoc, Node 20 CI, and dual UMD + ESM publishing with bundled .d.ts types

Tests grow from 61 to 125 (all passing); tsc --noEmit and ESLint are clean.

Bug fixes (each with a regression test)

API correctness

  • playbackRate/volume setters now match nodes with instanceof MediaNode instead of a fragile constructor-name check, so they work on subclasses
  • audio() now passes the media element cache (previously silently dropped)
  • Deprecated createImageSourceNode forwards its arguments correctly
  • New VideoContext.destroy() releases the update-loop registration and the global debug ref; reset() clears callbacks before the final update so stale UPDATE callbacks don't fire
  • Added missing endOnLastSourceEnd getter; WebGL context creation falls back to experimental-webgl with the supplied context attributes

Render graph

  • getInputsForNode places z-index connections on the port matching their index (previously order-dependent); out-of-range indexes fall back to the first free port
  • Connecting to an unknown named port on an unlimited-input node now throws ConnectException instead of silently mis-connecting
  • Exceptions are real Error subclasses (stack traces, instanceof works)

Source nodes

  • Seeking while a node is waiting now updates its clock, so start(offset) is computed from the seeked time
  • start()/startAt() refuse to re-sequence a destroyed node
  • durationchange fires once when derived from the element, not on every frame for endless sources; media error handlers are detached on unload so they can't fire on dead nodes
  • Ended nodes unload exactly once (MediaNode, and CanvasNode — found while writing coverage in the second commit)
  • ImageNode uploads its static texture once instead of every frame, re-uploads after seeks, and handles user-supplied complete elements without racing onload

Processing nodes

  • _render bound textures with the wrong texture-unit value (enum instead of index) — inputs beyond the first could sample the wrong texture
  • destroy() deleted the wrong texture reference and leaked the vertex buffer

TypeScript & toolchain

  • All 20 source modules + 21 effect/transition definitions converted to strict TS; no any escapes, honest casts are commented
  • All 44 GLSL files inlined as template-literal constants (removes raw-loader and the per-definition package.json stubs)
  • Babel 6 → 7 (@babel/preset-env + preset-typescript), Jest 23 → 29 (jsdom), ESLint 8 + @typescript-eslint, Prettier 3, JSDoc → TypeDoc, husky removed (CI is the gate), CI on Node 20 with a typecheck step
  • Zero runtime dependencies (regenerator-runtime no longer needed)

Packaging

  • main: UMD bundle (unchanged path), now Node-require safe (globalObject: "this")
  • module + exports map: per-module ESM build (tree-shakeable, sideEffects: false)
  • types: generated .d.ts declarations
  • All four outputs (UMD, commonjs2, ESM, types) smoke-tested

Breaking / behavioural changes

  • Browser support now follows browserslist "defaults" — IE11 is dropped
  • Node ≥ 18 required to build; consumers are unaffected beyond the browser targets
  • VideoContext.exceptions are now Error subclasses (message/stack behaviour changes if anyone matched on the old plain objects)

Not included (deliberately)

  • Cypress 4 → 13 migration — the visual-regression suite runs unchanged
  • cypress-image-snapshot replacement

Test plan

  • 125/125 unit + integration tests pass (Jest 29, jsdom)
  • tsc --noEmit clean (strict mode)
  • ESLint clean
  • UMD, commonjs2, ESM and .d.ts outputs built and smoke-tested
  • Cypress visual-regression suite (unchanged; needs a maintainer run against the built bundle)

@emshotton
emshotton force-pushed the modernise-toolchain branch from 1b3c636 to ac20c64 Compare August 17, 2026 05:45
emshotton and others added 4 commits August 16, 2026 22:49
Bug fixes (each with a regression test; 34 tests added, 61 -> 95):
- playbackRate/volume setters now match nodes via instanceof MediaNode
  (minification-safe, covers AudioNode and custom subclasses)
- audio() passes the real element cache instead of a never-assigned field
- ProcessingNode binds Image-property samplers with the 0-based texture
  unit index instead of the TEXTURE0 enum
- RenderGraph places index-connections on the port matching their zIndex
- SourceNode._seek updates the clock before the waiting-state early return
- new VideoContext.destroy() + UpdateablesManager.unregister() fix the
  permanent retention of every context by the module-level update loop
- ProcessingNode.destroy() no longer clobbers its main output texture;
  also deletes the vertex buffer
- ImageNode TYPE corrected from copy-pasted "CanvasNode"; user-supplied
  elements no longer crash _load and become ready; static image textures
  upload once instead of every frame
- durationchange no longer re-fires every frame for endless sources
- snapshot() records true input port indexes; element.onerror cleared on
  unload; destroyed nodes refuse start(); paused nodes unload on end;
  reset() clears callbacks before firing updates; unknown named ports on
  unlimited nodes throw; update loop starts without Worker/visibility API
- exceptions are real Error subclasses; shader failures throw them

TypeScript & toolchain migration:
- all of src/ converted to strict TypeScript; tsc type-checks and emits
  declarations, Babel 7 (+preset-typescript) transpiles
- 44 .frag/.vert shader files inlined as template literals (removes
  raw-loader, per-definition package.json stubs, and the jest glsl mock)
- Jest 29 aligned (drops jest-cli 23 / babel-jest 23; explicit jsdom),
  ESLint 8 + @typescript-eslint, Prettier 3, TypeDoc replaces jsdoc,
  husky removed, CI on Node 20 with a typecheck step
- packaging: UMD (main, unchanged path), per-module ESM (module/exports,
  sideEffects: false), generated .d.ts (types); zero runtime deps;
  browserslist "defaults" (drops IE11)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eated unload in CanvasNode

Adds 30 tests (95 -> 125) covering the video element pool's autoplay
contract, CanvasNode's load/seek/ended lifecycle, and MediaNode's
playbackRate, stretchPaused, buffering, loop, element-cache, URL/
MediaStream source, error, ended and seek branches. Overall statement
coverage rises from 57.8% to 65.7% (branches 48.1% -> 60.8%).

Writing the CanvasNode ended-state test exposed a bug: after passing
its stop time the node called _unload() on every frame, firing the
"destroy" callback repeatedly, because its guard (_element !==
undefined) never becomes false for a user-supplied canvas. Fixed with
the same _loadCalled guard used by MediaNode.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GConf was removed from Ubuntu, so this step now fails on ubuntu-latest
(24.04) before any tests run. Nothing needs it: the unit and
integration suites are Node/jsdom only, and Cypress 4's Electron 8 /
Chromium 80 does not link against GConf.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Chrome 137+ disables the SwiftShader software-WebGL fallback by
default, so on GitHub's GPU-less runners getContext("webgl") returns
null and every visual test fails with "Cannot read properties of
undefined (reading 'createTexture')". Pass --enable-unsafe-swiftshader
via the before:browser:launch hook to restore it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@emshotton
emshotton force-pushed the modernise-toolchain branch from ac20c64 to be6b113 Compare August 17, 2026 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant