[ogawa0071]

Summary

Lets the library run outside a browser (Node.js 20.13+, for latency, download and upload) by removing the hard dependency on the window global.

Changes

• Resolve relative API URLs against globalThis.location instead of window.location (BandwidthEngine, LoadNetworkEngine, utils/authorization). In a browser globalThis is window, so behavior there is unchanged. Outside a browser location is undefined, so absolute API URLs (every default) now resolve instead of throwing a ReferenceError on window. The authorization helper keeps withholding the token for relative URLs there.
• Report a relative API URL outside a browser through onError: with no location, MeasurementEngine checks downloadApiUrl / uploadApiUrl when it's created. A relative one is reported through onError when the test starts, and the test ends, without logging the results, before any request is made. The report is deferred to a microtask, so with autoStart an onError set right after construction still gets it; if the test is paused first, it waits for play(). In a browser the check doesn't run, and relative URLs still resolve against the page origin.
• Tolerate a resource timing entry without nextHopProtocol (Node's fetch leaves it out): it is treated as an unknown HTTP version (0) instead of throwing.
• README notes the Node.js support: 20.13+ on a best-effort basis for latency, download and upload. Earlier versions don't report request timings for fetch (feat: Add resource timing entries for connection, request and response nodejs/undici#2481), so every sample is rejected. Packet loss still needs a browser (RTCPeerConnection), and API URLs must be absolute outside a browser.
• Tests stub fetch and the page origin through globalThis / location instead of window, matching what the source now reads. The new tests/unit/outsideBrowser.test.ts runs BandwidthEngine and SpeedTest with no location. It covers absolute URLs, a timing entry without nextHopProtocol, and a relative URL reported through onError, including when the test is paused right after construction. It also checks that, with location stubbed, a relative URL still resolves against the page origin.

No Node.js-only APIs are introduced (no fs, Buffer, process or require()), so the browser-only constraint in AGENTS.md still holds; this only drops the assumption that window exists.

Testing

• pnpm test:unit: 260 passed
• pnpm lint (oxlint, oxfmt check, tsc): passes
• pnpm build: passes
• pnpm test:e2e: 72 passed (Chromium + WebKit)
• The unit tests run the library in Node, but with fetch and performance stubbed, so no automated test makes real requests from Node.js

🤖 Generated with Claude Code

[clanky-the-clanker]

This pull request's reviews are posted as pull request reviews: see the review of b1e85bb4.

[andre-j3sus]

Thanks for this! I built the branch and ran latency, download and upload against speed.cloudflare.com on Node 26, and it completes with sensible results. Browser behavior is unchanged since globalThis is window there.

A few things before we merge:

1. Rebase: this conflicts with main since #176 landed. The window.location.origin in BandwidthEngine moved into a new URL helper, so that's where the change needs to go now.
2. Tests: right now the suite would still pass if either fix were reverted. Could you add:
   • one engine test that runs with absolute URLs and no location stub, so the "no DOM" path is actually exercised beyond the auth helper;
   • one fake timing entry without nextHopProtocol. Node leaves that field out, and every current fixture sets it to 'h2'.
3. README: it still says the library relies on the browser PerformanceResourceTiming API. A short note would help: Node 18+ is supported on a best-effort basis for latency, download and upload, packet loss still needs a browser (RTCPeerConnection), and API URLs must be absolute outside a browser.

[ogawa0071]

Thanks for testing it on Node 26! I've addressed the review:

1. Rebase: rebased onto main. The base URL change is now in #buildRequest, and the nextHopProtocol change in #calibrateServerTimeDelta. The window stubs that came with feat: add parallel bandwidth measurements and multiple hosts #176 (parallelBatch, parallelResults, and the e2e parallelBandwidth spy) now use location / globalThis like the rest.

2. Tests: tests/unit/outsideBrowser.test.ts adds:

   • BandwidthEngine with absolute URLs and no location stub
   • a timing entry without nextHopProtocol
   • a relative API URL outside a browser, reported through onError (see the inline thread)
   • a relative API URL in a browser, still resolved against the page origin

   Reverting either fix makes the matching test fail.

3. README: added the Node.js note: Node 18+ on a best-effort basis for latency, download and upload, packet loss still needs a browser (RTCPeerConnection), and API URLs must be absolute outside a browser.

pnpm test:unit and pnpm lint pass locally; I haven't run the e2e suite locally.

[andre-j3sus]

Thanks for the quick turnaround, this looks good! I re-ran it on Node 26: absolute URLs work, and a relative downloadApiUrl now ends cleanly through onError with no crash.

One correction on the README: Node 18+ doesn't actually work. Before undici 6.1.0 (nodejs/undici#2481), Node's fetch reported requestStart and responseStart as 0 (nodejs/undici#2515). So hasDetailedTiming rejects every sample, with a misleading Timing-Allow-Origin error, and the summary comes back as zeros. I checked raw timing entries across versions:

Node	undici	Works
18.20.8	5.29.0	❌
20.12.2	5.28.4	❌
20.13.0	6.13.0	✅
22.0.0	6.11.1	✅

So the minimum is Node 20.13. Suggested wording is inline.

Could you also update the PR description so it covers the second commit? It should mention the up-front relative URL check reported through onError and the README note. It also still says 197 tests and "no automated test that runs the full library under Node.js".

Optional, nice to have: a test that calls pause() right after construction with a relative URL and checks that onError doesn't fire until play().

[ogawa0071]

Thanks for re-running it and tracking down the Node versions!

1. README: applied your wording (Node.js 20.13+) in dd7b12d.
2. PR description: updated to cover the second commit: the up-front relative URL check reported through onError, the README note and the new tests. The test count is current, and the Testing section now says the unit tests run the library in Node with fetch and performance stubbed, so none makes real requests.
3. Pause test: added "waits for play() to report one when paused right after construction" in tests/unit/outsideBrowser.test.ts (b1e85bb). It pauses right after construction and checks that onError doesn't fire, then that it fires once after play(). Dropping the #running check from the deferred report makes it fail.

pnpm test:unit (260 passed), pnpm test:e2e (72 passed in Chromium and WebKit) and pnpm lint pass locally.

[andre-j3sus]

clanky review

[clanky-the-clanker]

✅ Approved — No issues found. The change from window to globalThis keeps browser behaviour the same, and outside a browser a relative API URL is reported through onError without throwing.

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

✅ All checks passed

🔒 Security: ✅ LGTM

No security issues. The authorization token is still withheld from relative URLs outside a browser.

---

📋 Code Quality: ✅ LGTM

I found no defects. The relative-URL check, the deferred onError report with its pause/restart guard, the handling of a missing nextHopProtocol and the authorization helper all work correctly with no location.

---

⚡ Performance: ✅ LGTM

No performance concerns.

---

📝 Documentation: ✅ LGTM

The README note on Node.js support matches the behaviour in the code.

---

_{Reviewed commit: b1e85bb4}

[mangeshraut712]

Replacing window.location.origin with globalThis.location and failing early on a relative API URL in Node is the right approach. A few holes remain around that guard.

1. Only downloadApiUrl / uploadApiUrl are checked. The README says latency is also supported in Node, but #apiUrlError does not look at a latency (or reachability) URL. A relative latency endpoint still goes through new URL(apiUrl, globalThis?.location?.origin) in LoadNetworkEngine / BandwidthEngine. With no location, a relative first argument and an undefined base throw a TypeError that nothing reports via onError.

2. isAbsoluteUrl uses new URL(url) with no base, so a protocol-relative URL such as //speed.example.com/api is treated as relative (that constructor throws). In a browser it would resolve against the page origin; in Node it will be rejected or, if it reaches the engine, throw. If you want to accept those, resolve them only when location exists, and treat them as invalid outside a browser with the same onError path.

3. new URL(..., globalThis?.location?.origin) should not be relied on as a fallback once the constructor guard has run. Even for a download URL that passed the check, a later #nextApiUrl(dir) that returns a relative path will throw in Node. Passing an explicit base only when location?.origin is a non-empty string (and otherwise requiring an already-absolute URL) would make the engines fail the same way as the constructor.

[andre-j3sus]

@mangeshraut712 Thanks for taking a look! I checked these against the current head:

1. There's no separate latency URL: latency runs through BandwidthEngine with the same downloadApiUrl / uploadApiUrl lists the constructor check covers. Reachability passes targetUrl straight to fetch, so a relative one is reported as reachable: false rather than throwing.
2. A protocol-relative URL has no scheme to resolve against without a page origin, so rejecting it through onError outside a browser is the intended behavior. In a browser the check doesn't run and it still resolves against the page.
3. #nextApiUrl only rotates through the lists validated at construction, so it can't return a URL the check hasn't seen.

So I don't think any changes are needed here.

[andre-j3sus]

Withdrawing my approval for now: my colleagues and I are still reviewing this change.

Withdrawing approval for now.