Unexpected behavior with beforeRetry and baseUrl

[wesleytodd]

Describe the bug

Actual behavior

The examples show updating hostname in the options, but if you originally used baseUrl and then try to modify baseUrl 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

const r = got.extend('https://foo.com/path', {
	hooks: {
		beforeRetry: [
			(options, response) => {
				options.baseUrl = 'https://bar.com/other-path';
			}
		]
	}
});

await r('/more/path')

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

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[szmarczak]

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 options.baseUrl on redirect should throw. I'll make a PR for that. it's already done I think...

[wesleytodd]

Oh, sorry I copy pasted and then didnt change the method. The report here is about beforeRetry. Sorry about the confusion, I updated the example.

[szmarczak]

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 somethingthatdoesntexist.com does not exist and bar.com exists. You get https://bar.com/other-path/more/path.

[wesleytodd]

So what you outlined is basically what I did. IMO the DX would be better if when setting options.baseUrl it would do all this for you in the library. If it does this when passing it originally I expected it to do the same here.

[szmarczak]

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. baseUrl is used only once to build it. Then we don't need it because on redirects we know the base is the current URL. It's completely being forgotten. So if you try to modify it in a beforeRedirect hook, it will throw.

You may get redirected to another page and your logic is broken. The best solution is to make another request.

[wesleytodd]

I just proposed a solution. I didn't say it was right

Sorry I miss understood I think since you closed the issue with that comment. I thought you meant that as an answer.

because on redirects

This is what I meant when I said I meant beforeRetry. The original code snippet was not correct. I have updated beforeRedirect to beforeRetry. What I want is the first request which fails to then retry to a different url. Because the url was specified using baseUrl in the first place, the simplest method to me seemed to use the same way to specify the retry behavior.

I think you just miss-understood my issue because I used the wrong code example. Sorry this caused back and forth like this.

[szmarczak]

Sorry I miss understood I think since you closed the issue with that comment. I thought you meant that as an answer.

I'm sorry for not saying that at first. It's late here so I'm sorry for giving unclear examples.

By:

This still works as expected.

I meant that:

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.

That is the correct behavior as baseUrl has no effect there. The URL has been normalized already. To process the request we don't baseUrl. We just need the full URL.

I understand what you mean. You just have some constant part e.g. /foo/bar and you want to fallback to a different prefix e.g. from https://example.com/123/ to https://example.com/456.

Let me propose a solution even different from making another request. You need to pass the input as some option, e.g. context and use it in the beforeRetry hook. Since you know the prefix you just need to do new URL(prefix + options.context) and overwrite these options: hostname, port and pathname (I recommend converting the URL instance into options and merging them, see the utils directory). I'll give you a code example tomorrow, I'm going to sleep now :)

[wesleytodd]

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}`;
    }]
  }
});

[szmarczak]

Just wanted to follow up on this, are you saying that no changes will be made based on this?

That's right. You can't exactly fallback to another prefixUrl because the URL is normalized afterwards and you may get redirected to a different site. It's best to write your own function to handle these scenarios.

[szmarczak]

@sindresorhus has convinced me that it would be a nice feature. Will think of something.