Skip to content

refactor: delegate font source resolution to the parser - #1653

Merged
obiot merged 2 commits into
melonjs:masterfrom
ICOM725:refactor/fontface-src-resolution
Sep 17, 2026
Merged

obiot merged 2 commits into
melonjs:masterfrom
ICOM725:refactor/fontface-src-resolution

Conversation

@ICOM725

@ICOM725 ICOM725 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Description

Move CSS font source handling out of the generic loader and into the fontface parser. The loader delegates source normalization and base-URL exclusion to optional parser hooks; the fontface parser owns url() unwrapping and local() handling.

Keep the existing matching rules and immutable asset descriptors. Tests register the font rules under a different parser name and cover bare paths, quoted/unquoted URLs, local fonts, data URIs and retries of the same frozen manifest entry.

Type of change

  • Bug fix
  • New feature
  • Documentation update
  • Performance improvement
  • Refactoring (no functional changes)

Checklist

  • I have read the Contributing Guide
  • My code follows the existing code style (pnpm lint passes)
  • I have tested my changes locally (pnpm test passes)
  • I have added tests that cover my changes (if applicable)
  • The build succeeds (pnpm build)

Targeted verification: six loader/font spec files pass (98 tests), with Playwright using installed Chrome instead of Chromium Headless Shell through an external config override. Four of the seven new parser-alias cases failed before the change. pnpm lint, pnpm build and git diff --check pass.

The full suite was not run for this worktree. Full runs on the same upstream baseline with the independent Trigger change encountered browser-import/iframe failures and a stalled WebGPU spec; those runs are not reported as passing here.

Related issues

Fixes #1648.

ICOM725 and others added 2 commits September 17, 2026 12:34
Builds on the delegation this PR introduced. The hooks were optional
properties bolted onto one parser function, so `parsers.get()` returned a bare
function for fourteen types and a decorated one for the fifteenth — `load()`
still had to know some types are special, which is the thing moving CSS out of
it was meant to stop.

`setParser` now normalizes every registration into the same record —
`{ parse, normalizeSrc, needsBaseURL }` — filling both hooks with defaults when
a type does not declare them. `load()` calls them unconditionally and never
names an asset type. A parser that needs neither writes nothing; the fourteen
that do are untouched.

Renamed, because the old names described what the loader should DO rather than
what the value IS:

- `resolveSrc` -> `normalizeSrc`. It resolved nothing — resolution is the base
  URL going on afterwards. It turns a type-specific descriptor into a bare path
- `skipBaseURL` -> `needsBaseURL`, inverted. "Skip a step" is an instruction to
  the caller and read as a double negative at the call site; the positive form
  has a truthful default (yes, prefix it) and reads as a sentence

The rename exposed a layering bug I then had to fix. `data:` and `skipBaseURL`
were the same predicate — "this src needs no base URL" — one hardcoded, one
delegated. Folding them naively means a type that overrides the hook REPLACES
the data-URI rule, so a data-URI font would get a base URL prepended. `data:`
is decided by `load()` for every type, before the hook is consulted, so an
overriding parser declares only its own exceptions and cannot forget that one.

The contract is an `AssetParser` typedef next to `Asset`, matching how the
loader documents itself. `setParser(type, fn)` is unchanged for callers.

Tests: the contributor's seven cases plus twenty-three adversarial ones. The
ones that earn their place are the ordering traps — a src that normalizes INTO
a data URI, and one that normalizes into a form its own `needsBaseURL` then
rejects — both of which resolve to a 404 if the exclusions are tested against
the original src rather than the normalized one. Also pinned: a plain parser is
handed `url(...)` and `local(...)` verbatim, since those mean nothing to a type
that has not said so.

Each piece verified by reverting it: folding `data:` into the default fails 5,
running `normalizeSrc` after the prefix fails 10, applying it to an array src
fails 1. Full suite 287 files / 6951 tests; the twelve loader and asset specs
green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NGvtaUNATVCVxD2qcbiY4t
@obiot
obiot force-pushed the refactor/fontface-src-resolution branch from 972efa4 to 0b7566c Compare September 17, 2026 04:54
@obiot

obiot commented Sep 17, 2026

Copy link
Copy Markdown
Member

Thanks for this — the direction is right and the delegation is the part worth keeping. I've pushed two changes on top of your commit (rebased onto master, which had moved ~20 commits): one to the shape of the hooks, one to their names.

The shape

Your patch attaches resolveSrc / skipBaseURL as properties on preloadFontFace, which is what #1648 proposed. In practice that means parsers.get() returns a bare function for fourteen types and a decorated one for the fifteenth, so load() still has to probe for optional properties — it knows some types are special, just implicitly rather than by name. That is the thing moving CSS out of load() was meant to stop.

setParser now normalizes every registration into the same record:

parsers.set(type, {
    parse: parserFn,
    normalizeSrc: parserFn.normalizeSrc ?? keepSrc,
    needsBaseURL: parserFn.needsBaseURL ?? alwaysRelative,
});

A type declares hooks exactly as you had it — on the parser function, in the parser's own file — and the fourteen that need neither write nothing. load() calls both unconditionally and never names a type. The contract is an AssetParser typedef next to Asset, matching how the loader documents itself elsewhere. setParser(type, fn) is unchanged for callers.

The names

Both old names described what the loader should do rather than what the value is:

  • resolveSrcnormalizeSrc. It resolved nothing — resolution is the base URL going on afterwards. What it does is turn a type-specific descriptor into a bare path
  • skipBaseURLneedsBaseURL, inverted. "Skip a step" is an instruction to the caller, and read as a double negative at the call site. The positive form has a truthful default (yes, prefix it) and reads as a sentence: normalize the src, then prefix it if it needs a base URL

A bug the rename exposed

data: and skipBaseURL were the same predicate — "this src needs no base URL" — one hardcoded, one delegated. Folding them naively means a type that overrides the hook replaces the data-URI rule, so a data-URI font gets a base URL prepended. load() now decides data: for every type before consulting the hook, so an overriding parser declares only its own exceptions and cannot forget that one. Your two data-URI cases would have caught it.

Tests

Your seven cases are kept (renamed to the new hooks) and there are 23 more in loader-parser-contract.spec.js. The ones that earn their place are the ordering traps — a src that normalizes into a data URI, and one that normalizes into a form its own needsBaseURL then rejects — both of which resolve to a 404 if the exclusions are tested against the original src rather than the normalized one. Also pinned: a parser declaring no hooks is handed url(...) and local(...) verbatim, since those mean nothing to a type that has not said so.

Each piece verified by reverting it: folding data: into the default fails 5, running normalizeSrc after the prefix fails 10, applying it to an array src fails 1.

Your checklist noted the full suite was not run — it is green now: 287 files / 6951 tests, pnpm lint clean, and the four specs #1648 listed as must-stay-green among them.

@ICOM725

ICOM725 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up! Having setParser fill in the defaults makes the loader simpler, and needsBaseURL reads much better at the call site. Keeping the data-URI check in the loader makes sense too, so individual parsers can't accidentally bypass it.

@obiot
obiot merged commit 8cd744d into melonjs:master Sep 17, 2026
3 checks passed
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.

Loader: fontface url() handling lives in the generic src-resolution path

2 participants