feat(routeFromHar): add interceptAPIRequests option#41294
Conversation
Make `BrowserContext.routeFromHAR` also intercept requests issued via APIRequestContext (`page.request.*` / `context.request.*`) when the new `interceptAPIRequests: true` option is set. Defaults to `false` so existing behavior is unchanged. Under the hood, `HarBackend.lookup` gains an `apiRequestOnly` flag that filters matches to entries with `_apiRequest: true` (already written by the HAR recorder), so API-side replay never picks up a browser-side recording for the same URL. Fixes microsoft#22869
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@dcrousso Could you please take a look? |
dcrousso
left a comment
dcrousso
left a comment
There was a problem hiding this comment.
thanks so much for taking the time to submit a PR!
this is a really great start, but i think it's missing a few things in order to fully work
please let me know if any of my comments are unclear
| if (postData) | ||
| setHeader(headers, 'content-length', String(postData.byteLength)); | ||
| const { body, log, response } = await this._sendRequestWithRetries(progress, requestUrl, options, postData, params.maxRetries); | ||
| const harResponse = await this._lookupInHar(progress, requestUrl, method, headers, postData); |
There was a problem hiding this comment.
i think this will prevent the dispatch of APIRequestContext.Events.Request and APIRequestContext.Events.RequestFinished meaning that if a new recording is captured while replaying from the HAR then it wont include any previously captured API requests
There was a problem hiding this comment.
Good catch. The short-circuit bypassed _sendRequest, which is the only emitter of Request/RequestFinished. The HAR path now emits both events (mirroring _sendRequest), so capturing a new recording while replaying still includes the API requests. Added a test (should re-record intercepted APIRequestContext requests into a new HAR).
| log.push(`HAR: ${lookupResult.message ?? 'lookup failed'}`); | ||
| continue; | ||
| } | ||
| if (lookupResult.action === 'noentry') { |
There was a problem hiding this comment.
NIT: 'missing' would be a better name here
There was a problem hiding this comment.
I kept 'noentry' here — the action is part of the LocalUtilsHarLookupResult wire protocol (localUtils.yml), so renaming it would be a breaking change for cross-version client/server compatibility. Happy to rename if you'd still prefer it and are OK treating it as a protocol change.
Make the HAR-replay path for APIRequestContext behave like the live network path and tighten the server-side plumbing: - Emit Request/RequestFinished events so a recording captured while replaying still includes the API requests. - Apply set-cookie side-effects via addCookies, like the live path. - Honor maxRedirects (throw when exceeded) and report the final entry URL on redirects. - Populate statusText, securityDetails and serverAddr on the response; log via progress.log alongside the in-memory log. - Reuse the already-open HarBackend (looked up by harId) instead of opening a second backend for the same HAR file. - Rename to routeAPIRequestsFromHar/unrouteAPIRequestsFromHar, give the newest registration priority (unshift), and fold the registration bookkeeping into the dispatcher's _disposables. Fixes: microsoft#22869
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
# Conflicts: # packages/playwright-core/src/client/harRouter.ts
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dcrousso
left a comment
dcrousso
left a comment
There was a problem hiding this comment.
very nice work! :)
sorry it took me a minute to get to this
| // Reuse the HarBackend that was already opened via localUtils.harOpen for the page-side | ||
| // route, rather than opening a second backend for the same HAR file. The backend is owned | ||
| // by LocalUtils and closed via harClose, so this registration must not dispose it. | ||
| const localUtils = [...this.connection._dispatcherByGuid.values()].find(d => d._type === 'LocalUtils') as LocalUtilsDispatcher | undefined; |
There was a problem hiding this comment.
NIT: is there no better way of finding this than by looking at the _type? that seems somewhat fragile
There was a problem hiding this comment.
Agreed. Replaced the inline _dispatcherByGuid scan with a typed helper on DispatcherConnection — getDispatcher<T>(type) (sits next to the existing existingDispatcher<T>(object)) — so the call site is now this.connection.getDispatcher<LocalUtilsDispatcher>('LocalUtils'). Done in f5c0b22.
Co-authored-by: Devin Rousso <hi@devinrousso.com> Signed-off-by: Kevin Tan <stkevintan@zju.edu.cn>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…equests - abort HAR lookup errors when notFound:'abort' (was silently falling through) - iterate a copy of the registrations to survive concurrent unroute - close the HAR last in HarRouter.dispose, after unrouting API registrations - look up LocalUtils via a typed DispatcherConnection.getDispatcher<T>() helper instead of an inline _dispatcherByGuid scan - assert response.serverAddr() in the intercepted-request test (record in 'full' mode so serverIPAddress/serverPort are persisted)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "MCP"3 failed 7458 passed, 1132 skipped Merge workflow run. |
Test results for "tests 1"4 flaky49329 passed, 1163 skipped Merge workflow run. |
3 participants
Fixes #22869
Summary
interceptAPIRequestsoption toBrowserContext.routeFromHARsopage.request.*/context.request.*calls are also served from the HAR file.false— fully backward compatible._apiRequest: true(already written by the HAR recorder), so browser-side recordings are never served to API requests for the same URL.