Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multipart boundary mismatch #540

Merged
merged 3 commits into from
Oct 29, 2023
Merged

Fix multipart boundary mismatch #540

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a20029
Add functionality to run same test in multiple browsers
kdelmonte Oct 27, 2023
9485d30
Fix an issue in firefox where the body option was being detected as a…
kdelmonte Oct 27, 2023
9560921
Replace hardcoded request options list in favor of a registry of opti…
kdelmonte Oct 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ jobs:
with:
node-version: ${{ matrix.node-version }}
- run: npm install
- name: Install Playwright Browsers
run: npx playwright install --with-deps
- run: npm test
# TODO: Enable again when https://github.com/istanbuljs/nyc/issues/1287 is fixed.
# - uses: codecov/codecov-action@v1
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
"expect-type": "^0.16.0",
"express": "^4.18.2",
"pify": "^6.1.0",
"playwright-chromium": "^1.37.1",
"playwright": "^1.39.0",
"raw-body": "^2.5.2",
"ts-node": "^10.9.1",
"typescript": "^5.2.2",
Expand Down
17 changes: 17 additions & 0 deletions source/core/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,20 @@ export const kyOptionKeys: KyOptionsRegistry = {
onDownloadProgress: true,
fetch: true,
};

export const requestOptions = new Set([
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any other way than having a hard-coded list here? We'll have to keep this up to date and if we miss something, it could cause problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really want to avoid this too. Let me give it some thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a similar solution to the KyOptionsRegistry can be employed here to defer the list keeping to Typescript and undici. I will try this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there are two RequestInit interfaces that are relevant here: the one exported from node's undici-types and the one exported from Typescript's lib.dom.d.ts.

Common fields present in both interfaces:

  • method
  • keepalive
  • headers
  • body
  • redirect
  • integrity
  • signal
  • credentials
  • mode
  • referrer
  • referrerPolicy
  • window

Unique to undici-types:

  • dispatcher
  • duplex

Unique to Typescript's lib.dom.d.ts:

  • cache

Standard Request Options as specified in MDN

Present in both undici-types and lib.dom.d.ts:

  • method
  • headers
  • body
  • mode
  • credentials
  • redirect
  • referrer
  • referrerPolicy
  • integrity
  • keepalive
  • signal

Present only in lib.dom.d.ts:

  • cache

Missing in both undici-types and lib.dom.d.ts:

  • priority (⚠️ this field is still considered experimental but it is already supported in Chromium based browsers)

With all of this being said, I think we can leave priority out and let it be detected as an unknown property since it will still be passed down to fetch.

Please let me know what you think of this approach @sindresorhus and @sholladay.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sounds like a good approach.

'options',
'method',
'headers',
'body',
'mode',
'credentials',
'cache',
'redirect',
'referrer',
'referrerPolicy',
'integrity',
'keepalive',
'signal',
'priority',
]);
4 changes: 2 additions & 2 deletions source/utils/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {kyOptionKeys} from '../core/constants.js';
import {kyOptionKeys, requestOptions} from '../core/constants.js';

export const findUnknownOptions = (
request: Request,
Expand All @@ -7,7 +7,7 @@ export const findUnknownOptions = (
const unknownOptions: Record<string, unknown> = {};

for (const key in options) {
if (!(key in kyOptionKeys) && !(key in request)) {
if (!requestOptions.has(key) && !(key in kyOptionKeys) && !(key in request)) {
unknownOptions[key] = options[key];
}
}
Expand Down
32 changes: 15 additions & 17 deletions test/browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import test, {type ExecutionContext} from 'ava';
import busboy from 'busboy';
import express from 'express';
import {type Page} from 'playwright-chromium';
import {chromium, webkit, type Page} from 'playwright';
import type ky from '../source/index.js';
import {createHttpTestServer, type ExtendedHttpTestServer, type HttpServerOptions} from './helpers/create-http-test-server.js';
import {parseRawBody} from './helpers/parse-body.js';
import {withPage} from './helpers/with-page.js';
import {browserTest, defaultBrowsersTest} from './helpers/with-page.js';

declare global {
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
Expand Down Expand Up @@ -46,7 +46,7 @@ test.afterEach(async () => {
await server.close();
});

test.serial('prefixUrl option', withPage, async (t: ExecutionContext, page: Page) => {
defaultBrowsersTest('prefixUrl option', async (t: ExecutionContext, page: Page) => {
server.get('/', (_request, response) => {
response.end('zebra');
});
Expand Down Expand Up @@ -74,7 +74,7 @@ test.serial('prefixUrl option', withPage, async (t: ExecutionContext, page: Page
t.deepEqual(results, ['rainbow', 'rainbow', 'rainbow', 'rainbow']);
});

test.serial('aborting a request', withPage, async (t: ExecutionContext, page: Page) => {
defaultBrowsersTest('aborting a request', async (t: ExecutionContext, page: Page) => {
server.get('/', (_request, response) => {
response.end('meow');
});
Expand All @@ -98,7 +98,7 @@ test.serial('aborting a request', withPage, async (t: ExecutionContext, page: Pa
t.is(errorName, 'AbortError');
});

test.serial('should copy origin response info when using `onDownloadProgress`', withPage, async (t: ExecutionContext, page: Page) => {
defaultBrowsersTest('should copy origin response info when using `onDownloadProgress`', async (t: ExecutionContext, page: Page) => {
const json = {hello: 'world'};
const status = 202;
const statusText = 'Accepted';
Expand Down Expand Up @@ -133,7 +133,7 @@ test.serial('should copy origin response info when using `onDownloadProgress`',
});
});

test.serial('should not copy response body with 204 status code when using `onDownloadProgress` ', withPage, async (t: ExecutionContext, page: Page) => {
defaultBrowsersTest('should not copy response body with 204 status code when using `onDownloadProgress` ', async (t: ExecutionContext, page: Page) => {
const status = 204;
const statusText = 'No content';
server.get('/', (_request, response) => {
Expand Down Expand Up @@ -182,7 +182,7 @@ test.serial('should not copy response body with 204 status code when using `onDo
}]);
});

test.serial('aborting a request with onDonwloadProgress', withPage, async (t: ExecutionContext, page: Page) => {
browserTest('aborting a request with onDownloadProgress', [chromium], async (t: ExecutionContext, page: Page) => {
server.get('/', (_request, response) => {
response.end('meow');
});
Expand Down Expand Up @@ -214,9 +214,8 @@ test.serial('aborting a request with onDonwloadProgress', withPage, async (t: Ex
t.is(error, 'TypeError: Failed to fetch');
});

test.serial(
defaultBrowsersTest(
'throws TimeoutError even though it does not support AbortController',
withPage,
async (t: ExecutionContext, page: Page) => {
server.get('/', (_request, response) => {
response.end();
Expand Down Expand Up @@ -249,7 +248,7 @@ test.serial(
},
);

test.serial('onDownloadProgress works', withPage, async (t: ExecutionContext, page: Page) => {
browserTest('onDownloadProgress works', [chromium, webkit], async (t: ExecutionContext, page: Page) => {
server.get('/', (_request, response) => {
response.writeHead(200, {
'content-length': '4',
Expand Down Expand Up @@ -289,7 +288,7 @@ test.serial('onDownloadProgress works', withPage, async (t: ExecutionContext, pa
t.is(result.text, 'meow');
});

test.serial('throws if onDownloadProgress is not a function', withPage, async (t: ExecutionContext, page: Page) => {
defaultBrowsersTest('throws if onDownloadProgress is not a function', async (t: ExecutionContext, page: Page) => {
server.get('/', (_request, response) => {
response.end();
});
Expand All @@ -305,7 +304,7 @@ test.serial('throws if onDownloadProgress is not a function', withPage, async (t
t.is(error, 'TypeError: The `onDownloadProgress` option must be a function');
});

test.serial('throws if does not support ReadableStream', withPage, async (t: ExecutionContext, page: Page) => {
defaultBrowsersTest('throws if does not support ReadableStream', async (t: ExecutionContext, page: Page) => {
server.get('/', (_request, response) => {
response.end();
});
Expand All @@ -322,7 +321,7 @@ test.serial('throws if does not support ReadableStream', withPage, async (t: Exe
t.is(error, 'Error: Streams are not supported in your environment. `ReadableStream` is missing.');
});

test.serial('FormData with searchParams', withPage, async (t: ExecutionContext, page: Page) => {
defaultBrowsersTest('FormData with searchParams', async (t: ExecutionContext, page: Page) => {
t.plan(3);

server.get('/', (_request, response) => {
Expand Down Expand Up @@ -354,7 +353,7 @@ test.serial('FormData with searchParams', withPage, async (t: ExecutionContext,
}, server.url);
});

test.serial('FormData with searchParams ("multipart/form-data" parser)', withPage, async (t: ExecutionContext, page: Page) => {
defaultBrowsersTest('FormData with searchParams ("multipart/form-data" parser)', async (t: ExecutionContext, page: Page) => {
t.plan(3);

server.get('/', (_request, response) => {
Expand Down Expand Up @@ -426,9 +425,8 @@ test.serial('FormData with searchParams ("multipart/form-data" parser)', withPag
}, server.url);
});

test.serial(
defaultBrowsersTest(
'headers are preserved when input is a Request and there are searchParams in the options',
withPage,
async (t: ExecutionContext, page: Page) => {
t.plan(2);

Expand Down Expand Up @@ -459,7 +457,7 @@ test.serial(
},
);

test.serial('retry with body', withPage, async (t: ExecutionContext, page: Page) => {
browserTest('retry with body', [chromium, webkit], async (t: ExecutionContext, page: Page) => {
t.plan(4);

let requestCount = 0;
Expand Down
31 changes: 20 additions & 11 deletions test/helpers/with-page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@
/* eslint-disable ava/no-ignored-test-files */
import process from 'node:process';
import type {ExecutionContext, Implementation} from 'ava';
import {chromium, type Page} from 'playwright-chromium';
import test from 'ava';
import {chromium, firefox, webkit, type BrowserType, type Page} from 'playwright';
import type {ExecutionContext} from 'ava';

type Run = (t: ExecutionContext, page: Page) => Promise<void>;

const PWDEBUG = Boolean(process.env['PWDEBUG']);
const DEFAULT_BROWSERS = [chromium, firefox, webkit];

export const withPage: Implementation<any[]> = async (t: ExecutionContext, run: Run): Promise<void> => {
const browser = await chromium.launch({
devtools: PWDEBUG,
});
const page = await browser.newPage();
try {
await run(t, page);
} finally {
await browser.close();
export const browserTest = (title: string, browserTypes: BrowserType[], run: Run) => {
for (const browserType of browserTypes) {
test.serial(`${browserType.name()} - ${title}`, async t => {
const browser = await browserType.launch({devtools: PWDEBUG});
const page = await browser.newPage();
try {
await run(t, page);
} finally {
await browser.close();
}
});
}
};

export const defaultBrowsersTest = (title: string, run: Run) => {
browserTest(title, DEFAULT_BROWSERS, run);
};