-
-
Notifications
You must be signed in to change notification settings - Fork 936
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.
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
Normalize the URL in the baseUrl
option
#579
Conversation
test/arguments.js
Outdated
const instanceA = got.extend({baseUrl: `${s.url}/test/`}); | ||
const {body} = await instanceA('/foobar'); | ||
t.is(body, `/test/foobar`); | ||
}); |
There was a problem hiding this comment.
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');
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup
source/normalize-arguments.js
Outdated
return options; | ||
}; | ||
|
||
module.exports.prenormalize = prenormalize; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
The docs needs to be updated too. |
Would be more descriptive to title the PR:
|
Can you comment on why it was necessary to split it out into a preNormalize step? |
Code comment or GitHub comment? |
Just a comment on this thread is fine. |
source/normalize-arguments.js
Outdated
@@ -13,6 +13,47 @@ const knownHookEvents = require('./known-hook-events'); | |||
|
|||
const retryAfterStatusCodes = new Set([413, 429, 503]); | |||
|
|||
const preNormalize = options => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've split normalize-argument.js
into two parts:
preNormalize
handles all the stuff which is related with static options likebaseUrl
,followRedirect
,hooks
etc.
It is used ingot.create()
to normalizedefaults.options
.normalize
doespreNormalize
+ handles all the stuff related with dynamic options likeurl
,headers
,body
etc.
baseUrl
optionbaseUrl
option
Fixes #562