From e914f6bbc7c1eb5a5f1ff3f80f8fdff85b9887c9 Mon Sep 17 00:00:00 2001 From: Joel Einbinder Date: Tue, 7 Sep 2021 13:27:53 -0400 Subject: [PATCH] feat(network): remove Headers class and add headersArray method (#8749) --- docs/src/api/class-headers.md | 31 ------ docs/src/api/class-request.md | 10 +- docs/src/api/class-response.md | 8 +- src/client/api.ts | 1 - src/client/network.ts | 109 +++++++++------------- tests/browsercontext-fetch.spec.ts | 3 +- tests/page/page-network-request.spec.ts | 26 +++--- tests/page/page-network-response.spec.ts | 19 ++-- tests/page/page-request-intercept.spec.ts | 6 +- types/types.d.ts | 47 ++++------ utils/doclint/missingDocs.js | 4 +- 11 files changed, 109 insertions(+), 155 deletions(-) delete mode 100644 docs/src/api/class-headers.md diff --git a/docs/src/api/class-headers.md b/docs/src/api/class-headers.md deleted file mode 100644 index 0263f52d7d..0000000000 --- a/docs/src/api/class-headers.md +++ /dev/null @@ -1,31 +0,0 @@ -# class: Headers - -HTTP request and response all headers collection. - -## method: Headers.get -- returns: <[string|null]> -Returns header value for the given name. - -### param: Headers.get.name -- `name` <[string]> -Header name, case-insensitive. - -## method: Headers.getAll -- returns: <[Array]<[string]>> - -Returns all header values for the given header name. - -### param: Headers.getAll.name -- `name` <[string]> -Header name, case-insensitive. - -## method: Headers.headerNames -- returns: <[Array]<[string]>> - -Returns all header names in this headers collection. - -## method: Headers.headers -- returns: <[Object]<[string], [string]>> - -Returns all headers as a dictionary. Header names are normalized to lower case, multi-value headers are concatenated -using comma. diff --git a/docs/src/api/class-request.md b/docs/src/api/class-request.md index 31cf271e2b..d0eb635717 100644 --- a/docs/src/api/class-request.md +++ b/docs/src/api/class-request.md @@ -17,9 +17,9 @@ If request gets a 'redirect' response, the request is successfully finished with request is issued to a redirected url. ## async method: Request.allHeaders -- returns: <[Headers]> +- returns: <[Object]<[string], [string]>> -An object with all the request HTTP headers associated with this request. +An object with all the request HTTP headers associated with this request. The header names are lower-cased. ## method: Request.failure - returns: <[null]|[string]> @@ -61,6 +61,12 @@ Returns the [Frame] that initiated this request. **DEPRECATED** Incomplete list of headers as seen by the rendering engine. Use [`method: Request.allHeaders`] instead. +## async method: Request.headersArray +- returns: <[Array]<[Array]<[string]>>> + +An array with all the request HTTP headers associated with this request. Unlike [`method: Request.allHeaders`], header names are not lower-cased. +Headers with multiple entries, such as `Set-Cookie`, appear in the array multiple times. + ## method: Request.isNavigationRequest - returns: <[boolean]> diff --git a/docs/src/api/class-response.md b/docs/src/api/class-response.md index 08f4477922..de0e27ad01 100644 --- a/docs/src/api/class-response.md +++ b/docs/src/api/class-response.md @@ -3,7 +3,7 @@ [Response] class represents responses which are received by page. ## async method: Response.allHeaders -- returns: <[Headers]> +- returns: <[Object]<[string], [string]>> An object with all the response HTTP headers associated with this response. @@ -27,6 +27,12 @@ Returns the [Frame] that initiated this response. **DEPRECATED** Incomplete list of headers as seen by the rendering engine. Use [`method: Response.allHeaders`] instead. +## async method: Response.headersArray +- returns: <[Array]<[Array]<[string]>>> + +An array with all the request HTTP headers associated with this response. Unlike [`method: Response.allHeaders`], header names are not lower-cased. +Headers with multiple entries, such as `Set-Cookie`, appear in the array multiple times. + ## async method: Response.json * langs: js, python - returns: <[Serializable]> diff --git a/src/client/api.ts b/src/client/api.ts index 663659fe90..736ddb6cb5 100644 --- a/src/client/api.ts +++ b/src/client/api.ts @@ -41,4 +41,3 @@ export { Video } from './video'; export { Worker } from './worker'; export { CDPSession } from './cdpSession'; export { Playwright } from './playwright'; -export { RawHeaders as Headers } from './network'; diff --git a/src/client/network.ts b/src/client/network.ts index 2c2ef35319..63396e4806 100644 --- a/src/client/network.ts +++ b/src/client/network.ts @@ -18,7 +18,7 @@ import { URLSearchParams } from 'url'; import * as channels from '../protocol/channels'; import { ChannelOwner } from './channelOwner'; import { Frame } from './frame'; -import { Headers, HeadersArray, RemoteAddr, SecurityDetails, WaitForEventOptions } from './types'; +import { Headers, RemoteAddr, SecurityDetails, WaitForEventOptions } from './types'; import fs from 'fs'; import * as mime from 'mime'; import { isString, headersObjectToArray, headersArrayToObject } from '../utils/utils'; @@ -29,7 +29,6 @@ import { Waiter } from './waiter'; import * as api from '../../types/types'; import { URLMatch } from '../common/types'; import { urlMatches } from './clientHelper'; -import { MultiMap } from '../utils/multimap'; export type NetworkCookie = { name: string, @@ -58,8 +57,8 @@ export class Request extends ChannelOwner | undefined; + _headers: channels.NameValue[]; + private _allHeadersPromise: Promise | undefined; private _postData: Buffer | null; _timing: ResourceTiming; @@ -76,7 +75,7 @@ export class Request extends ChannelOwner { - if (this._rawHeadersPromise) - return this._rawHeadersPromise; - this._rawHeadersPromise = this.response().then(response => { - if (!response) - return new RawHeaders([]); - return response._wrapApiCall(async (channel: channels.ResponseChannel) => { - return new RawHeaders((await channel.rawRequestHeaders()).headers); + _getHeadersIfNeeded() { + if (!this._allHeadersPromise) { + this._allHeadersPromise = this.response().then(response => { + // there is no response, so should we return the headers we have now? + if (!response) + return this._headers; + return response._wrapApiCall(async (channel: channels.ResponseChannel) => { + return await (await channel.rawRequestHeaders()).headers; + }); }); - }); - return this._rawHeadersPromise; + } + return this._allHeadersPromise; + } + + async allHeaders(): Promise { + return headersArrayToObject(await this._getHeadersIfNeeded(), true); + } + + async headersArray(): Promise { + return (await this._getHeadersIfNeeded()).map(header => [header.name, header.value]); } async response(): Promise { @@ -204,14 +212,10 @@ export class InterceptedResponse implements api.Response { private readonly _route: Route; private readonly _initializer: channels.InterceptedResponse; private readonly _request: Request; - private readonly _headers: Headers; - private readonly _rawHeaders: RawHeaders; constructor(route: Route, initializer: channels.InterceptedResponse) { this._route = route; this._initializer = initializer; - this._headers = headersArrayToObject(initializer.headers, true /* lowerCase */); - this._rawHeaders = new RawHeaders(initializer.headers); this._request = Request.from(initializer.request); } @@ -251,11 +255,15 @@ export class InterceptedResponse implements api.Response { } headers(): Headers { - return { ...this._headers }; + return headersArrayToObject(this._initializer.headers, true /* lowerCase */); } - async allHeaders(): Promise { - return this._rawHeaders; + async allHeaders(): Promise { + return headersArrayToObject(this._initializer.headers, true /* lowerCase */); + } + + async headersArray(): Promise { + return this._initializer.headers.map(header => [header.name, header.value]); } async body(): Promise { @@ -413,7 +421,7 @@ export class Response extends ChannelOwner(); - private _rawHeadersPromise: Promise | undefined; + private _rawHeadersPromise: Promise | undefined; static from(response: channels.ResponseChannel): Response { return (response as any)._object; @@ -453,15 +461,23 @@ export class Response extends ChannelOwner { - if (this._rawHeadersPromise) - return this._rawHeadersPromise; - this._rawHeadersPromise = this._wrapApiCall(async (channel: channels.ResponseChannel) => { - return new RawHeaders((await channel.rawResponseHeaders()).headers); - }); + async _getHeadersIfNeeded() { + if (!this._rawHeadersPromise) { + this._rawHeadersPromise = this._wrapApiCall(async (channel: channels.ResponseChannel) => { + return await channel.rawResponseHeaders(); + }); + } return this._rawHeadersPromise; } + async allHeaders(): Promise { + return headersArrayToObject((await this._getHeadersIfNeeded()).headers, true /* lowerCase */); + } + + async headersArray(): Promise { + return (await this._getHeadersIfNeeded()).headers.map(header => [header.name, header.value]); + } + async finished(): Promise { return this._finishedPromise.then(() => null); } @@ -639,36 +655,3 @@ export class RouteHandler { this.handledCount++; } } - -export class RawHeaders implements api.Headers { - private _headersArray: HeadersArray; - private _headersMap = new MultiMap(); - - constructor(headers: HeadersArray) { - this._headersArray = headers; - for (const header of headers) - this._headersMap.set(header.name.toLowerCase(), header.value); - } - - get(name: string): string | null { - const values = this.getAll(name); - if (!values) - return null; - return values.join(', '); - } - - getAll(name: string): string[] { - return [...this._headersMap.get(name.toLowerCase())]; - } - - headerNames(): string[] { - return [...this._headersMap.keys()]; - } - - headers(): Headers { - const result: Headers = {}; - for (const name of this._headersMap.keys()) - result[name] = this.get(name)!; - return result; - } -} diff --git a/tests/browsercontext-fetch.spec.ts b/tests/browsercontext-fetch.spec.ts index 9e68c2c166..e3ba1e6de7 100644 --- a/tests/browsercontext-fetch.spec.ts +++ b/tests/browsercontext-fetch.spec.ts @@ -18,6 +18,7 @@ import http from 'http'; import zlib from 'zlib'; import { pipeline } from 'stream'; import { contextTest as it, expect } from './config/browserTest'; +import type { Response } from '..'; it.skip(({ mode }) => mode !== 'default'); @@ -41,7 +42,7 @@ it.afterAll(() => { it('should work', async ({context, server}) => { // @ts-expect-error - const response = await context._fetch(server.PREFIX + '/simple.json'); + const response: Response = await context._fetch(server.PREFIX + '/simple.json'); expect(response.url()).toBe(server.PREFIX + '/simple.json'); expect(response.status()).toBe(200); expect(response.statusText()).toBe('OK'); diff --git a/tests/page/page-network-request.spec.ts b/tests/page/page-network-request.spec.ts index 902cf559f0..a44c1afe9f 100644 --- a/tests/page/page-network-request.spec.ts +++ b/tests/page/page-network-request.spec.ts @@ -90,7 +90,7 @@ it('should get the same headers as the server', async ({ page, server, browserNa }); const response = await page.goto(server.PREFIX + '/empty.html'); const headers = await response.request().allHeaders(); - expect(headers.headers()).toEqual(serverRequest.headers); + expect(headers).toEqual(serverRequest.headers); }); it('should get the same headers as the server CORS', async ({page, server, browserName, platform}) => { @@ -111,7 +111,7 @@ it('should get the same headers as the server CORS', async ({page, server, brows expect(text).toBe('done'); const response = await responsePromise; const headers = await response.request().allHeaders(); - expect(headers.headers()).toEqual(serverRequest.headers); + expect(headers).toEqual(serverRequest.headers); }); it('should return postData', async ({page, server, isAndroid}) => { @@ -266,15 +266,14 @@ it('should return navigation bit when navigating to image', async ({page, server expect(requests[0].isNavigationRequest()).toBe(true); }); -it('should report all headers', async ({ page, server, browserName, platform }) => { - const expectedHeaders = {}; +it('should report raw headers', async ({ page, server, browserName, platform }) => { + let expectedHeaders: string[][]; server.setRoute('/headers', (req, res) => { + expectedHeaders = []; for (let i = 0; i < req.rawHeaders.length; i += 2) - expectedHeaders[req.rawHeaders[i].toLowerCase()] = req.rawHeaders[i + 1]; - if (browserName === 'webkit' && platform === 'win32') { - delete expectedHeaders['accept-encoding']; - delete expectedHeaders['accept-language']; - } + expectedHeaders.push([req.rawHeaders[i], req.rawHeaders[i + 1]]); + if (browserName === 'webkit' && platform === 'win32') + expectedHeaders = expectedHeaders.filter(([name, value]) => name.toLowerCase() !== 'accept-encoding' && name.toLowerCase() !== 'accept-language'); res.end(); }); await page.goto(server.EMPTY_PAGE); @@ -289,8 +288,8 @@ it('should report all headers', async ({ page, server, browserName, platform }) ] })) ]); - const headers = await request.allHeaders(); - expect(headers.headers()).toEqual(expectedHeaders); + const headers = await request.headersArray(); + expect(headers.sort()).toEqual(expectedHeaders.sort()); }); it('should report raw response headers in redirects', async ({ page, server, browserName }) => { @@ -311,7 +310,7 @@ it('should report raw response headers in redirects', async ({ page, server, bro redirectChain.unshift(req.url()); const res = await req.response(); const headers = await res.allHeaders(); - headersChain.unshift(headers.get('sec-test-header')); + headersChain.unshift(headers['sec-test-header']); } expect(redirectChain).toEqual(expectedUrls); @@ -332,7 +331,6 @@ it('should report all cookies in one header', async ({ page, server }) => { document.cookie = 'myOtherCookie=myOtherValue'; }); const response = await page.goto(server.EMPTY_PAGE); - const headers = await response.request().allHeaders(); - const cookie = headers.get('cookie'); + const cookie = (await response.request().allHeaders())['cookie']; expect(cookie).toBe('myCookie=myValue; myOtherCookie=myOtherValue'); }); diff --git a/tests/page/page-network-response.spec.ts b/tests/page/page-network-response.spec.ts index 5e7b5f0cad..19501931cb 100644 --- a/tests/page/page-network-response.spec.ts +++ b/tests/page/page-network-response.spec.ts @@ -25,9 +25,9 @@ it('should work', async ({page, server}) => { res.end(); }); const response = await page.goto(server.EMPTY_PAGE); - expect(response.headers()['foo']).toBe('bar'); - expect(response.headers()['baz']).toBe('bAz'); - expect(response.headers()['BaZ']).toBe(undefined); + expect((await response.allHeaders())['foo']).toBe('bar'); + expect((await response.allHeaders())['baz']).toBe('bAz'); + expect((await response.allHeaders())['BaZ']).toBe(undefined); }); @@ -134,10 +134,13 @@ it('should report all headers', async ({ page, server, browserName, platform }) page.waitForResponse('**/*'), page.evaluate(() => fetch('/headers')) ]); - const headers = await response.allHeaders(); + const headers = await response.headersArray(); const actualHeaders = {}; - for (const name of headers.headerNames()) - actualHeaders[name] = headers.getAll(name); + for (const [name, value] of headers) { + if (!actualHeaders[name]) + actualHeaders[name] = []; + actualHeaders[name].push(value); + } delete actualHeaders['Keep-Alive']; delete actualHeaders['keep-alive']; delete actualHeaders['Connection']; @@ -163,7 +166,7 @@ it('should report multiple set-cookie headers', async ({ page, server }) => { page.waitForResponse('**/*'), page.evaluate(() => fetch('/headers')) ]); - const headers = await response.allHeaders(); - const cookies = headers.getAll('set-cookie'); + const headers = await response.headersArray(); + const cookies = headers.filter(([name, value]) => name.toLowerCase() === 'set-cookie').map(([, value]) => value); expect(cookies).toEqual(['a=b', 'c=d']); }); diff --git a/tests/page/page-request-intercept.spec.ts b/tests/page/page-request-intercept.spec.ts index 25462f18c4..8bf15162ca 100644 --- a/tests/page/page-request-intercept.spec.ts +++ b/tests/page/page-request-intercept.spec.ts @@ -17,7 +17,7 @@ import { fail } from 'assert'; import os from 'os'; -import type { Route } from '../../index'; +import type { Route, Response } from '../../index'; import { expect, test as it } from './pageTest'; it('should fulfill intercepted response', async ({page, server, browserName}) => { @@ -195,12 +195,14 @@ it('should give access to the intercepted response', async ({page, server}) => { const route = await routePromise; // @ts-expect-error - const response = await route._continueToResponse(); + const response: Response = await route._continueToResponse(); expect(response.status()).toBe(200); expect(response.ok()).toBeTruthy(); expect(response.url()).toBe(server.PREFIX + '/title.html'); expect(response.headers()['content-type']).toBe('text/html; charset=utf-8'); + expect((await response.allHeaders())['content-type']).toBe('text/html; charset=utf-8'); + expect(await (await response.headersArray()).filter(([name, value]) => name.toLowerCase() === 'content-type')).toEqual([['Content-Type', 'text/html; charset=utf-8']]); // @ts-expect-error await Promise.all([route.fulfill({ response }), evalPromise]); diff --git a/types/types.d.ts b/types/types.d.ts index 626cec95fd..a4ceb9792a 100644 --- a/types/types.d.ts +++ b/types/types.d.ts @@ -12690,33 +12690,6 @@ export interface FileChooser { }): Promise; } -/** - * HTTP request and response all headers collection. - */ -export interface Headers { - /** - * @param name - */ - get(name: string): string|null; - - /** - * Returns all header values for the given header name. - * @param name - */ - getAll(name: string): Array; - - /** - * Returns all header names in this headers collection. - */ - headerNames(): Array; - - /** - * Returns all headers as a dictionary. Header names are normalized to lower case, multi-value headers are concatenated - * using comma. - */ - headers(): { [key: string]: string; }; -} - /** * Keyboard provides an api for managing a virtual keyboard. The high level api is * [keyboard.type(text[, options])](https://playwright.dev/docs/api/class-keyboard#keyboard-type), which takes raw @@ -13048,9 +13021,9 @@ export interface Mouse { */ export interface Request { /** - * An object with all the request HTTP headers associated with this request. + * An object with all the request HTTP headers associated with this request. The header names are lower-cased. */ - allHeaders(): Promise; + allHeaders(): Promise<{ [key: string]: string; }>; /** * The method returns `null` unless this request has failed, as reported by `requestfailed` event. @@ -13083,6 +13056,13 @@ export interface Request { */ headers(): { [key: string]: string; }; + /** + * An array with all the request HTTP headers associated with this request. Unlike + * [request.allHeaders()](https://playwright.dev/docs/api/class-request#request-all-headers), header names are not + * lower-cased. Headers with multiple entries, such as `Set-Cookie`, appear in the array multiple times. + */ + headersArray(): Promise>>; + /** * Whether this request is driving frame's navigation. */ @@ -13268,7 +13248,7 @@ export interface Response { /** * An object with all the response HTTP headers associated with this response. */ - allHeaders(): Promise; + allHeaders(): Promise<{ [key: string]: string; }>; /** * Returns the buffer with response body. @@ -13292,6 +13272,13 @@ export interface Response { */ headers(): { [key: string]: string; }; + /** + * An array with all the request HTTP headers associated with this response. Unlike + * [response.allHeaders()](https://playwright.dev/docs/api/class-response#response-all-headers), header names are not + * lower-cased. Headers with multiple entries, such as `Set-Cookie`, appear in the array multiple times. + */ + headersArray(): Promise>>; + /** * Returns the JSON representation of response body. * diff --git a/utils/doclint/missingDocs.js b/utils/doclint/missingDocs.js index 9f209d0388..fc8c76bd85 100644 --- a/utils/doclint/missingDocs.js +++ b/utils/doclint/missingDocs.js @@ -56,13 +56,13 @@ module.exports = function lint(documentation, jsSources, apiFileName) { continue; const params = methods.get(member.alias); if (!params) { - errors.push(`Documented "${cls.name}.${member.alias}" not found is sources`); + errors.push(`Documented "${cls.name}.${member.alias}" not found in sources`); continue; } const memberParams = paramsForMember(member); for (const paramName of memberParams) { if (!params.has(paramName) && paramName !== 'options') - errors.push(`Documented "${cls.name}.${member.alias}.${paramName}" not found is sources`); + errors.push(`Documented "${cls.name}.${member.alias}.${paramName}" not found in sources`); } } }