Skip to content

Commit

Permalink
Make options.path backwards-compatible
Browse files Browse the repository at this point in the history
Also added a note on `options.auth`
  • Loading branch information
szmarczak committed Dec 9, 2019
1 parent 7b2ccb0 commit b3f1ac9
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 31 deletions.
1 change: 1 addition & 0 deletions documentation/migration-guides.md
Expand Up @@ -79,6 +79,7 @@ To use streams, just call `got.stream(url, options)` or `got(url, {isStream: tru
- No `agentClass`/`agentOptions`/`pool` option.
- No `forever` option. You need to use [forever-agent](https://github.com/request/forever-agent).
- No `proxy` option. You need to [pass a custom agent](readme.md#proxies).
- No `auth` option. You need to use `username` / `password` instead.
- No `baseUrl` option. Instead, there is `prefixUrl` which appends a trailing slash if not present. It will be always prepended unless `url` is an instance of URL.
- No `removeRefererHeader` option. You can remove the referer header in a [`beforeRequest` hook](https://github.com/sindresorhus/got#hooksbeforeRequest):

Expand Down
4 changes: 3 additions & 1 deletion readme.md
Expand Up @@ -98,7 +98,7 @@ const pipeline = promisify(stream.pipeline);

It's a `GET` request by default, but can be changed by using different methods or via `options.method`.

**By default, Got will retry on failure. To disable this option, set [`retry`](#retry) to `0`.**
**By default, Got will retry on failure. To disable this option, set [`options.retry`](#retry) to `0`.**

#### got(url?, options?)

Expand All @@ -122,6 +122,8 @@ Type: `object`

Any of the [`https.request`](https://nodejs.org/api/https.html#https_https_request_options_callback) options.

**Note:** Legacy Url support is disabled. `options.path` is supported only for backwards compatibility. Instead of the `options.auth`, you should use `options.username` and/or `options.password`.

###### prefixUrl

Type: `string | URL`
Expand Down
37 changes: 32 additions & 5 deletions source/utils/options-to-url.ts
Expand Up @@ -19,9 +19,12 @@ export interface URLOptions {
search?: string;
searchParams?: Record<string, string | number | boolean | null> | URLSearchParams | string;
hash?: string;

// The only accepted legacy Url options
path?: string;
}

const keys: Array<Exclude<keyof URLOptions, 'href' | 'origin' | 'searchParams'>> = [
const keys: Array<Exclude<keyof URLOptions, 'href' | 'origin' | 'searchParams' | 'path'>> = [
'protocol',
'username',
'password',
Expand All @@ -36,12 +39,22 @@ const keys: Array<Exclude<keyof URLOptions, 'href' | 'origin' | 'searchParams'>>
export default (options: URLOptions): URL => {
let origin: string;

if (Reflect.has(options, 'path')) {
throw new TypeError('Parameter `path` is deprecated. Use `pathname` instead.');
if (options.path) {
if (options.pathname) {
throw new TypeError('Parameters `path` and `pathname` are mutually exclusive.');
}

if (options.search) {
throw new TypeError('Parameters `path` and `search` are mutually exclusive.');
}

if (options.searchParams) {
throw new TypeError('Parameters `path` and `searchParams` are mutually exclusive.');
}
}

if (Reflect.has(options, 'auth')) {
throw new TypeError('Parameter `auth` is deprecated. Use `username`/`password` instead.');
throw new TypeError('Parameter `auth` is deprecated. Use `username` / `password` instead.');
}

if (options.search && options.searchParams) {
Expand All @@ -64,13 +77,27 @@ export default (options: URLOptions): URL => {

const url = new URL(origin);

if (options.path) {
const searchIndex = options.path.indexOf('?');
if (searchIndex === -1) {
options.pathname = options.path;
} else {
options.pathname = options.path.slice(0, searchIndex);
options.search = options.path.slice(searchIndex + 1);
}
}

if (Reflect.has(options, 'path')) {

This comment has been minimized.

Copy link
@akaNightmare

akaNightmare Dec 10, 2019

options.path !== undefined ?

This comment has been minimized.

Copy link
@szmarczak

szmarczak Dec 10, 2019

Author Collaborator

@akaNightmare why so?

This comment has been minimized.

Copy link
@akaNightmare

akaNightmare Dec 10, 2019

I assume that Reflect is heavy and if you can avoid using Reflect - why not?

This comment has been minimized.

Copy link
@akaNightmare

akaNightmare Dec 10, 2019

anyway you can move delete options.path line to the b3f1ac9#diff-6a335487e6da082b0665e7f014b53e21R88

This comment has been minimized.

Copy link
@szmarczak

szmarczak Dec 10, 2019

Author Collaborator

I assume that Reflect is heavy and if you can avoid using Reflect - why not?

It's not heavy. Node.js is blazing fast generally. But when it comes to I/O - yep, streams are slow.

This comment has been minimized.

Copy link
@szmarczak

szmarczak Dec 10, 2019

Author Collaborator

image

This comment has been minimized.

Copy link
@akaNightmare

akaNightmare Dec 11, 2019

this is not relevant...

if (x.a) {
  delete x.a
}

and I mentioned that you checked options.path twhice b3f1ac9#diff-6a335487e6da082b0665e7f014b53e21R80
just move delete options.path; after this line b3f1ac9#diff-6a335487e6da082b0665e7f014b53e21R87

This comment has been minimized.

Copy link
@szmarczak

szmarczak Dec 11, 2019

Author Collaborator

if (Reflect.has(options, 'path')) is not the same as if (options.path).

delete options.path;
}

for (const key of keys) {
if (Reflect.has(options, key)) {
url[key] = options[key].toString();
}
}

if (Reflect.has(options, 'searchParams')) {
if (options.searchParams) {
if (typeof options.searchParams !== 'string' && !(options.searchParams instanceof URLSearchParams)) {
validateSearchParams(options.searchParams);
}
Expand Down
48 changes: 28 additions & 20 deletions test/options-to-url.ts
Expand Up @@ -2,18 +2,40 @@ import test from 'ava';
import is from '@sindresorhus/is';
import optionsToUrl from '../source/utils/options-to-url';

test('`path` is deprecated', t => {
const origin = 'https://google.com';

test('`path` and `pathname` are mutually exclusive', t => {
t.throws(() => {
// @ts-ignore Error tests
optionsToUrl({path: 'a', pathname: 'a'});
}, 'Parameters `path` and `pathname` are mutually exclusive.');
});

test('`path` and `search` are mutually exclusive', t => {
t.throws(() => {
// @ts-ignore Error tests
optionsToUrl({path: ''});
}, 'Parameter `path` is deprecated. Use `pathname` instead.');
optionsToUrl({path: 'a', search: 'a'});
}, 'Parameters `path` and `search` are mutually exclusive.');
});

test('`path` and `searchParams` are mutually exclusive', t => {
t.throws(() => {
// @ts-ignore Error tests
optionsToUrl({path: 'a', searchParams: {}});
}, 'Parameters `path` and `searchParams` are mutually exclusive.');
});

test('`path` option', t => {
const url = optionsToUrl({origin, path: '/?a=1'});
t.is(url.href, `${origin}/?a=1`);
t.true(is.urlInstance(url));
});

test('`auth` is deprecated', t => {
t.throws(() => {
// @ts-ignore Error tests
optionsToUrl({auth: ''});
}, 'Parameter `auth` is deprecated. Use `username`/`password` instead.');
}, 'Parameter `auth` is deprecated. Use `username` / `password` instead.');
});

test('`search` and `searchParams` are mutually exclusive', t => {
Expand All @@ -24,16 +46,12 @@ test('`search` and `searchParams` are mutually exclusive', t => {
});

test('`href` option', t => {
const href = 'https://google.com/';

const url = optionsToUrl({href});
t.is(url.href, href);
const url = optionsToUrl({href: origin});
t.is(url.href, `${origin}/`);
t.true(is.urlInstance(url));
});

test('`origin` option', t => {
const origin = 'https://google.com';

const url = optionsToUrl({origin});
t.is(url.href, `${origin}/`);
t.true(is.urlInstance(url));
Expand All @@ -46,40 +64,30 @@ test('throws if no protocol specified', t => {
});

test('`port` option', t => {
const origin = 'https://google.com';

const url = optionsToUrl({origin, port: 8888});
t.is(url.href, `${origin}:8888/`);
t.true(is.urlInstance(url));
});

test('`protocol` option', t => {
const origin = 'https://google.com';

const url = optionsToUrl({origin, protocol: 'http:'});
t.is(url.href, 'http://google.com/');
t.true(is.urlInstance(url));
});

test('`username` option', t => {
const origin = 'https://google.com';

const url = optionsToUrl({origin, username: 'username'});
t.is(url.href, 'https://username@google.com/');
t.true(is.urlInstance(url));
});

test('`password` option', t => {
const origin = 'https://google.com';

const url = optionsToUrl({origin, password: 'password'});
t.is(url.href, 'https://:password@google.com/');
t.true(is.urlInstance(url));
});

test('`username` option combined with `password` option', t => {
const origin = 'https://google.com';

const url = optionsToUrl({origin, username: 'username', password: 'password'});
t.is(url.href, 'https://username:password@google.com/');
t.true(is.urlInstance(url));
Expand Down
10 changes: 5 additions & 5 deletions test/timeout.ts
Expand Up @@ -606,12 +606,12 @@ test.serial('cancelling the request removes timeouts', withServer, async (t, ser
response.write('hello');
});

const promise = got({
timeout: 500,
const promise = got({
timeout: 500,
retry: 0
}).on('downloadProgress', () => {
promise.cancel();
}).on('request', request => {
}).on('downloadProgress', () => {
promise.cancel();
}).on('request', request => {
request.on('error', error => {
if (error.message === 'Timeout awaiting \'request\' for 500ms') {
t.fail(error.message);
Expand Down

0 comments on commit b3f1ac9

Please sign in to comment.