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

Jump to bottom

Normalize the URL in the baseUrl option #579

Merged
merged 8 commits into from
Aug 23, 2018
Merged

Normalize the URL in the baseUrl option #579

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7c4478a
Rewrite the `baseUrl` option
szmarczak Aug 23, 2018
294ee69
Merge branch 'master' into baseUrl-fix
szmarczak Aug 23, 2018
f2278b6
Nitpicks
szmarczak Aug 23, 2018
1af5cde
Update readme.md
szmarczak Aug 23, 2018
f074094
Update readme.md
sindresorhus Aug 23, 2018
fbf4174
Update normalize-arguments.js
szmarczak Aug 23, 2018
2e17859
Update normalize-arguments.js
sindresorhus Aug 23, 2018
ca78716
Keep coverage at 100%
szmarczak Aug 23, 2018
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
1 change: 1 addition & 0 deletions source/create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const aliases = [

const create = defaults => {
defaults = merge({}, defaults);
defaults.options = normalizeArguments.prenormalize(defaults.options);
if (!defaults.handler) {
defaults.handler = next;
}
Expand Down
113 changes: 66 additions & 47 deletions source/normalize-arguments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,47 @@ const knownHookEvents = require('./known-hook-events');

const retryAfterStatusCodes = new Set([413, 429, 503]);

const prenormalize = options => {
options = {
headers: {},
...options
};

if (options.baseUrl && !options.baseUrl.toString().endsWith('/')) {
options.baseUrl += '/';
}

if (is.undefined(options.followRedirect)) {
options.followRedirect = true;
}

if (is.nullOrUndefined(options.hooks)) {
options.hooks = {};
}
if (is.object(options.hooks)) {
for (const hookEvent of knownHookEvents) {
const hooks = options.hooks[hookEvent];
if (is.nullOrUndefined(hooks)) {
options.hooks[hookEvent] = [];
} else if (is.array(hooks)) {
for (const [index, hook] of hooks.entries()) {
if (!is.function(hook)) {
throw new TypeError(
`Parameter \`hooks.${hookEvent}[${index}]\` must be a function, not ${is(hook)}`
);
}
}
} else {
throw new TypeError(`Parameter \`hooks.${hookEvent}\` must be an array, not ${is(hooks)}`);
}
}
} else {
throw new TypeError(`Parameter \`hooks\` must be an object, not ${is(options.hooks)}`);
}

return options;
};

module.exports = (url, options, defaults) => {
if (Reflect.has(options, 'url') || (is.object(url) && Reflect.has(url, 'url'))) {
throw new TypeError('Parameter `url` is not an option. Use got(url, options)');
Expand All @@ -22,8 +63,14 @@ module.exports = (url, options, defaults) => {
throw new TypeError(`Parameter \`url\` must be a string or object, not ${is(url)}`);
}

options = prenormalize(options);

if (is.string(url)) {
if (options.baseUrl) {
if (url.toString().startsWith('/')) {
url = url.toString().slice(1);
}

url = urlToOptions(new URLGlobal(url, options.baseUrl));
} else {
url = url.replace(/^unix:/, 'http://$&');
Expand All @@ -45,7 +92,6 @@ module.exports = (url, options, defaults) => {

options = {
path: '',
headers: {},
...url,
protocol: url.protocol || 'http:', // Override both null/undefined with default protocol
...options
Expand All @@ -59,22 +105,17 @@ module.exports = (url, options, defaults) => {
get: () => baseUrl
});

if (options.stream && options.json) {
options.json = false;
}

if (options.decompress && is.undefined(options.headers['accept-encoding'])) {
options.headers['accept-encoding'] = 'gzip, deflate';
}

const {query} = options;

if (!is.empty(query) || query instanceof URLSearchParamsGlobal) {
const queryParams = new URLSearchParamsGlobal(query);
options.path = `${options.path.split('?')[0]}?${queryParams.toString()}`;
delete options.query;
}

if (options.stream && options.json) {
options.json = false;
}

if (options.json && is.undefined(options.headers.accept)) {
options.headers.accept = 'application/json';
}
Expand Down Expand Up @@ -125,6 +166,10 @@ module.exports = (url, options, defaults) => {

options.method = options.method.toUpperCase();

if (options.decompress && is.undefined(options.headers['accept-encoding'])) {
options.headers['accept-encoding'] = 'gzip, deflate';
}

if (options.hostname === 'unix') {
const matches = /(.+?):(.+)/.exec(options.path);

Expand Down Expand Up @@ -164,6 +209,15 @@ module.exports = (url, options, defaults) => {
}
}

if (is.number(options.timeout) || is.object(options.timeout)) {
if (is.number(options.timeout)) {
options.gotTimeout = {request: options.timeout};
} else {
options.gotTimeout = options.timeout;
}
delete options.timeout;
}

if (!is.function(options.gotRetry.retries)) {
const {retries} = options.gotRetry;

Expand Down Expand Up @@ -197,42 +251,7 @@ module.exports = (url, options, defaults) => {
};
}

if (is.undefined(options.followRedirect)) {
options.followRedirect = true;
}

if (is.number(options.timeout) || is.object(options.timeout)) {
if (is.number(options.timeout)) {
options.gotTimeout = {request: options.timeout};
} else {
options.gotTimeout = options.timeout;
}
delete options.timeout;
}

if (is.nullOrUndefined(options.hooks)) {
options.hooks = {};
}
if (is.object(options.hooks)) {
for (const hookEvent of knownHookEvents) {
const hooks = options.hooks[hookEvent];
if (is.nullOrUndefined(hooks)) {
options.hooks[hookEvent] = [];
} else if (is.array(hooks)) {
for (const [index, hook] of hooks.entries()) {
if (!is.function(hook)) {
throw new TypeError(
`Parameter \`hooks.${hookEvent}[${index}]\` must be a function, not ${is(hook)}`
);
}
}
} else {
throw new TypeError(`Parameter \`hooks.${hookEvent}\` must be an array, not ${is(hooks)}`);
}
}
} else {
throw new TypeError(`Parameter \`hooks\` must be an object, not ${is(options.hooks)}`);
}

return options;
};

module.exports.prenormalize = prenormalize;
Copy link
Owner

Choose a reason for hiding this comment

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

prenormalize is not a word, so should be preNormalize

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

16 changes: 16 additions & 0 deletions test/arguments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ test.before('setup', async () => {
response.end(request.url);
});

s.on('/test/foobar', (request, response) => {
response.end(request.url);
});

s.on('/?test=it’s+ok', (request, response) => {
response.end(request.url);
});
Expand Down Expand Up @@ -194,6 +198,18 @@ test('allows extra keys in `hooks`', async t => {
await t.notThrowsAsync(() => got(`${s.url}/test`, {hooks: {extra: {}}}));
});

test('baseUrl works', async t => {
const instanceA = got.extend({baseUrl: `${s.url}/test`});
const {body} = await instanceA('/foobar');
t.is(body, `/test/foobar`);
});

test('backslash is optional (baseUrl)', async t => {
const instanceA = got.extend({baseUrl: `${s.url}/test/`});
const {body} = await instanceA('/foobar');
t.is(body, `/test/foobar`);
});
Copy link
Owner

Choose a reason for hiding this comment

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

Can you also add a test with await instanceA('foobar');?

Copy link
Owner

Choose a reason for hiding this comment

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

And a couple of tests where the baseUrl is a WHATWG URL object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup


test('throws when trying to modify baseUrl after options got normalized', async t => {
const instanceA = got.create({
methods: [],
Expand Down
8 changes: 4 additions & 4 deletions test/create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ test('extend keeps the old value if the new one is undefined', t => {
test('extend merges URL instances', t => {
const a = got.extend({baseUrl: new URL('https://example.com')});
const b = a.extend({baseUrl: '/foo'});
t.is(b.defaults.options.baseUrl.toString(), 'https://example.com/foo');
t.is(b.defaults.options.baseUrl.toString(), 'https://example.com/foo/');
});

test('create', async t => {
Expand Down Expand Up @@ -128,7 +128,7 @@ test('no tampering with defaults', t => {
const instance = got.create({
handler: got.defaults.handler,
options: got.mergeOptions(got.defaults.options, {
baseUrl: 'example'
baseUrl: 'example/'
})
});

Expand All @@ -142,8 +142,8 @@ test('no tampering with defaults', t => {
instance.defaults.options.baseUrl = 'http://google.com';
});

t.is(instance.defaults.options.baseUrl, 'example');
t.is(instance2.defaults.options.baseUrl, 'example');
t.is(instance.defaults.options.baseUrl, 'example/');
t.is(instance2.defaults.options.baseUrl, 'example/');
});

test('only plain objects are freezed', async t => {
Expand Down