ReviewOS

stacks/very-happy-dom

waitForSelector returns null on timeout, where Playwright and our own waitForFunction throw

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

Problem

waitForSelector answers a timeout with null:

// BrowserPage.ts:208
while (Date.now() - startTime < timeout) {
  const element = this.mainFrame.document.querySelector(selector)
  if (element) { ... return element }
  await new Promise(resolve => setTimeout(resolve, 50))
}

return null   // <- Playwright throws TimeoutError here

Its immediate neighbour does throw:

// BrowserPage.ts:234
throw new Error(`waitForFunction timed out after ${timeout}ms`)

Confirmed:

await page.waitForSelector('.never-exists', { timeout: 200 })  // null

Why it matters

The two functions sit twenty lines apart and disagree about what a timeout is, so whichever one a reader learned first is wrong about the other.

The null is the worse half. The idiomatic line is:

const row = await page.waitForSelector('.row')
await row.click()

When the row never arrives, that reports TypeError: null is not an object (evaluating 'row.click') — pointing at the click, with no mention of the selector, the timeout, or the wait. The actual fact, ".row never appeared within 30s", is nowhere in the message. Same shape as #1602: a silent answer that sends whoever is debugging it to the wrong line.

Playwright returns an element or throws TimeoutError, and its { state: 'detached' } case returns null legitimately — which is exactly the distinction null here destroys.

Proposed scope

  • Throw on timeout, with the selector, the timeout and the state waited for in the message.
  • Add the state option (attached / detached / visible / hidden) so a deliberate "it is gone" wait has a way to say so. visible is already half-there as the visible?: boolean option and should fold into state.
  • Playwright supports { state: 'detached' } resolving to null; keep that as the only null return.
  • A distinguishable error type (TimeoutError) so callers can catch a timeout without string-matching.

This is a breaking change for anyone relying on the null, so it belongs with the other locator-waiting work rather than as a silent patch.

Acceptance

  • waitForSelector on a selector that never matches rejects, and the message contains the selector and the timeout.
  • waitForSelector(sel, { state: 'detached' }) resolves once the element is removed.
  • waitForFunction keeps throwing, and both errors are the same type.

Found while assessing whether wildloop's Playwright specs could port here.

Sign in to comment on this issue.