-
-
Notifications
You must be signed in to change notification settings - Fork 935
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
Unexpected behavior with beforeRetry
and baseUrl
#867
Comments
This is an invalid usage. If you receive relative path, the base is the current page, not the option. It's according to the HTTP specification. So, everything works as expected. Trying to modify |
Oh, sorry I copy pasted and then didnt change the method. The report here is about |
This still works as expected. The URL is normalized before making a request. Also you still did copy paste it. I think what you're looking for is: const {URL} = require('url');
const got = require('got');
const baseUrl = new URL('http://somethingthatdoesntexist.com/path');
const r = got.extend({
baseUrl,
hooks: {
beforeRetry: [
(options, response) => {
const oldBaseUrl = new URL(options.baseUrl);
options.baseUrl = new URL('https://bar.com/other-path');
const index = options.pathname.indexOf(oldBaseUrl.pathname);
if (index !== -1) {
options.pathname = options.baseUrl.pathname + '/' + options.pathname.slice(index + oldBaseUrl.pathname.length);
}
options.hostname = options.baseUrl.hostname;
console.log(options);
}
],
beforeRequest: [
options => {
console.log(options);
}
]
}
});
(async () => {
await r('/more/path');
})().catch(console.error); You receive 404 error as expected, because |
So what you outlined is basically what I did. IMO the DX would be better if when setting |
I just proposed a solution. I didn't say it was right. The opposite, it's just a workaround, NOT a proper solution. When making a request we need to know the absolute URL. You may get redirected to another page and your logic is broken. The best solution is to make another request. |
Sorry I miss understood I think since you closed the issue with that comment. I thought you meant that as an answer.
This is what I meant when I said I meant I think you just miss-understood my issue because I used the wrong code example. Sorry this caused back and forth like this. |
I'm sorry for not saying that at first. It's late here so I'm sorry for giving unclear examples. By:
I meant that:
That is the correct behavior as I understand what you mean. You just have some constant part e.g. Let me propose a solution even different from making another request. You need to pass the input as some option, e.g. |
Just wanted to follow up on this, are you saying that no changes will be made based on this? In case others stumble upon this, here is the example code which does work (but is IMO a very bad developer experience): const got = require('got');
const url = require('url');
got.extend({
hooks: {
beforeRetry: [(opts, err) => {
if (err.code !== 'ENOTFOUND' && err.statusCode !== 502 && err.statusCode !== 504) {
return;
}
opts.baseUrl = 'http://fallback.com/other/path'
const bu = new URL(opts.baseUrl);
if (bu.pathname.charAt(bu.pathname.length - 1) !== '/') {
bu.pathname = bu.pathname + '/';
}
const u = new URL(url.resolve(bu.pathname, opts.url.replace(/^\//, '')), opts.baseUrl);
opts.protocol = u.protocol;
opts.hostname = u.hostname;
opts.port = u.port ? parseInt(u.port, 10) : _options.port;
opts.pathname = u.pathname;
opts.path = `${u.pathname}${u.search}`;
opts.href = `${u.protocol}//${u.hostname}${opts.port ? `:${opts.port}` : ''}${u.pathname}${u.search}${u.hash}`;
}]
}
}); |
That's right. You can't exactly fallback to another |
@sindresorhus has convinced me that it would be a nice feature. Will think of something. |
Describe the bug
Actual behavior
The examples show updating
hostname
in the options, but if you originally usedbaseUrl
and then try to modifybaseUrl
in your hook, it does not re-derive the url. It seems like inconsistent/unexpected behavior.Expected behavior
Run the same logic which sets hostname, path, etc if the hook modifies
baseUrl
Code to reproduce
In the above example I would expect two requests (if the first failed):
https://foo.com/path/path/more/path
https://bar.com/other-path/more/path
What happens is a second request to
https://foo.com/path/path/more/path
.Checklist
The text was updated successfully, but these errors were encountered: