-
-
Notifications
You must be signed in to change notification settings - Fork 971
Normalize the URL in the baseUrl option
#579
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
Conversation
| 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. |
|
|
||
| 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:
preNormalizehandles all the stuff which is related with static options likebaseUrl,followRedirect,hooksetc.
It is used ingot.create()to normalizedefaults.options.normalizedoespreNormalize+ handles all the stuff related with dynamic options likeurl,headers,bodyetc.
baseUrl optionbaseUrl option

Fixes #562