ReviewOS

stacks/very-happy-dom

page.on('console' | 'response' | 'error') never fire: the emitters have no callers

#1602
Closed glennmichael123 opened this 8 days ago ·
8 days ago

Problem

BrowserPage advertises six page events:

export type PageEventType = 'console' | 'request' | 'response' | 'error' | 'load' | 'domcontentloaded'

Three of them can never fire. _emitConsole (BrowserPage.ts:730), _emitResponse (:746) and _emitError (:754) are declared and have zero callers anywhere in src/. _emitRequest (:738) has one caller at :914 and works, as do load and domcontentloaded, which BrowserFrame._commit emits.

const page = new Browser().newPage()
const fired: string[] = []
for (const event of ['console', 'response', 'error', 'load', 'domcontentloaded'] as const)
  page.on(event, () => fired.push(event))

page.mainFrame.window.console.log('hello from the page')
page.mainFrame.window.document.body.innerHTML = '<div></div>'

console.log(fired) // []  — the console.log printed, but nothing was emitted

Why it matters

page.on() accepts these names without complaint and returns normally, so the failure is silent. Registering a handler looks like it worked; it simply never runs.

The cost lands on the waiting patterns these events exist for. Code that awaits a console message or a response before asserting does not fail — it hangs until a timeout, and reports as a timeout rather than as a missing feature, which sends whoever is debugging it looking in the wrong place.

console is the one people reach for first, to assert a page logged no errors. A test doing that passes unconditionally today: no events, no errors, nothing checked.

Proposed scope

Either wire them up or stop advertising them. Wiring is the better answer, and each has an obvious source:

  • response — BrowserFrame._navigate already has the main-resource Response, and RouteRegistry sees every intercepted request's response.
  • console — the Window's console is already a distinct object; the page can forward through it the way _emitRequest is already forwarded.
  • error — page-level errors surfacing from evaluate and from navigation failures.

If any cannot be sourced meaningfully, remove it from PageEventType so page.on('console', ...) is a type error rather than a silent no-op. A missing feature that announces itself is better than one that pretends.

Acceptance

  • The reproduction above reports every event it registered for, or the unsupported names no longer type-check.
  • A test asserts each advertised event actually fires, so a future handler cannot quietly lose its only caller — which is how this happened.
  • request, load and domcontentloaded keep working.

Found while assessing whether wildloop's Playwright specs could port here: several await a response event before asserting, and would have hung rather than failed.

8 days ago

Fixed in 029bb19. CI green. All three are wired rather than removed, since each had a real source.

The reproduction from the issue now reports console where it reported [].

  • response — _navigate already held the Response; it emits before committing the document. Non-200s are reported too, not only the happy path.
  • error — the same method already had two failure paths, a fetch that throws and the offline guard. Both emit before rethrowing, so a listener sees the failure even though the caller still gets it.
  • console — the frame window's console defaulted to the host's, so page output went to stdout and nowhere else. It now forwards through a per-frame wrapper and still reaches the console underneath, which the existing test asserts.

test/browser-page-events.test.ts carries the guard the issue asked for: a test that registers for every name in PageEventType and fails unless all six fire. That is what was missing, and how three emitters came to sit there with no callers.

Two things changed beyond the literal ask

A response built by route.fulfill() carries no url, so a listener could not tell which request it answered — and goto()'s return value had the same hole. The landed url is filled in when it is missing; a real fetch reports its own and is untouched.

One existing expectation changed deliberately. browser-frame-window.test.ts asserted frame.window.console === browser.console. Forwarding makes that false, because page events are per page while the browser's console is shared between pages. It now asserts the relationship — the frame's console forwards to the browser's — and still asserts the output arrives, which was the test's actual point.

And one defect in my own test, caught before it shipped

The first version enabled request interception and routed **/* without tearing either down. Both are installed process-wide, so it answered other test files' fetches and failed six unrelated network tests a long way from here — the kind of thing that reads as flakiness for everyone afterwards. It now scopes its routes to its own origin and unroutes in afterEach. Suite is 1635 passing, from 1627 before.

Sign in to comment on this issue.