Merging port into defaults doesn't work with prefixUrl

[jmm]

Describe the bug

With Node 12, [email protected].

Merging port into default options doesn't work when prefixUrl is set.

Code to reproduce

See https://github.com/jmm/repro-got-merge-port-to-defaults.

const gotInstance = got.extend({
  mutableDefaults: true,
  prefixUrl: "http://example.com",
})

const port = 1234

gotInstance.defaults.options = gotInstance.mergeOptions(gotInstance.defaults.options, {
  port,
})

assert.equal(gotInstance.defaults.options.port, port)

Actual behavior

I expect port to be set as specified and the assertion to be true.

Expected behavior

Port is undefined.

---

I suspect this code: https://github.com/sindresorhus/got/blob/v11.8.2/source/core/index.ts#L1639-L1645

[szmarczak]

port is not a Got option. Got 12 will throw when passing a non-existing option.

[szmarczak]

prefixUrl is always a string. You'll need to fetch it from the defaults, parse it, replace the port, and pass to mergeOptions again.

[jmm]

@szmarczak

port is not a Got option.

How do you figure?

options is documented as including:

Any of the https.request options.

Which includes port. And it's used when provided:

got("http://localhost", {
  port: 8000,
  timeout: 100,
})
.catch(reason => {
  console.log(reason.message)
})

//=> "connect ECONNREFUSED 127.0.0.1:8000"

---

prefixUrl is always a string. You'll need to fetch it from the defaults, parse it, replace the port, and pass to mergeOptions again.

Thanks for the suggestion. If you're saying that prefixUrl is unaffected by changing default URL elements, that's a different story from port not being an option, and doesn't actually explain why port is not even merged into options in the case I showed. Whereas the code I linked to probably does.

And aside from the ergonomics, that appears to result in internal inconsistency:

const gotInstance = got.extend({
  mutableDefaults: true,
  prefixUrl: "http://example.com:8888",
})

gotInstance.defaults.options = gotInstance.mergeOptions(gotInstance.defaults.options, {
  prefixUrl: "http://example.com:9999",
})

console.log(gotInstance.defaults.options.prefixUrl)
console.log(gotInstance.defaults.options.url)

http://example.com:9999/
URL {
  href: 'http://example.com:8888/',
  origin: 'http://example.com:8888',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'example.com:8888',
  hostname: 'example.com',
  port: '8888',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

options.url doesn't seem to be documented and I don't know how it's used, but that smells like trouble.

What does the hooks.init documentation mean by?:

The options object may not have a url property. To modify it, use a beforeRequest hook instead.

Why / when would the options object have a url property? I don't see that documented anywhere.

---

On a side matter, why make updating the defaults so ceremonious?

// Before
gotInstance.defaults.options = got.mergeOptions(gotInstance.defaults.options, options)

// After
gotInstance.mergeInOptions(options)

[szmarczak]

options is documented as including:

The docs are a bit outdated. Passing port will throw in Got 12.

options.url doesn't seem to be documented

See my comment above.

If you're saying that prefixUrl is unaffected by changing default URL elements, that's a different story from port not being an option, and doesn't actually explain why port is not even merged into options in the case I showed.

Options will be a lot stricter in Got 12 - will throw when setting a non-existing option. Each option will be documented properly and the merging behavior will also be documented. prefixUrl is only controlled by the user.

And aside from the ergonomics, that appears to result in internal inconsistency:

The url property is the final url of the request, therefore accessing it before making an actual request is moot. In Got 12 it will return undefined as expected.

const got = require('got')

const gotInstance = got.extend({
  mutableDefaults: true,
  prefixUrl: "http://example.com:8888",
})

gotInstance.defaults.options = gotInstance.mergeOptions(gotInstance.defaults.options, {
  prefixUrl: "http://example.com:9999",
})

gotInstance('', {timeout: 0}).catch(error => console.log(error.options.url.toString()));

console.log(gotInstance.defaults.options.prefixUrl)
console.log(gotInstance.defaults.options.url.toString())

http://example.com:9999/
http://example.com:8888/
http://example.com:9999/

See https://github.com/sindresorhus/got#hooksbeforeredirect - but the example is outdated. It should access options.url.hostname instead.

On a side matter, why make updating the defaults so ceremonious?

You might want to reuse merged options for performance reasons. This will be documented in Got 12.

[szmarczak]

What does the hooks.init documentation mean by?:

It means that if you need to replace some part of the URL every time, then you need to use beforeRequest. init is called when the options are being normalized. beforeRequest is called right before making the actual HTTP request.

[jmm]

The docs are a bit outdated. Passing port will throw in Got 12.

That's good to know, but again, this is about the released version, 11.8.2. It comes across as dismissive when you close an issue on the basis that {x} isn't an option when {x} is documented (and utilized!) as an option by the version of the package in question.

Each option will be documented properly and the merging behavior will also be documented.

👍 That's great! I appreciate the attention to the documentation.

prefixUrl is only controlled by the user.

Ok, thank you.

prefixUrl is always a string. You'll need to fetch it from the defaults, parse it, replace the port, and pass to mergeOptions again.

So if I pass prefixUrl as a URL is it stringified? Because if it was preserved as a URL, rather than do that I'd probably prefer to pass it as a URL and then just set .port.

The url property is the final url of the request, therefore accessing it before making an actual request is moot. In Got 12 it will return undefined as expected.

Ok thanks.

You might want to reuse merged options for performance reasons. This will be documented in Got 12.

Ok thanks. It'll be good to see that documentation, because I'm not sure what scenario you're describing. It seems like most of the time a method like .mergeInOptions(newOptions) would be suitable, but if I needed to do something less usual I could copy the merged options out of .defaults.options after, or do the more ceremonious .mergeOptions.

It means that if you need to replace some part of the URL every time, then you need to use beforeRequest. init is called when the options are being normalized. beforeRequest is called right before making the actual HTTP request.

Yeah my question was about the reference to...

The options object may not have a url property.

...because I don't see that property documented. So if not for that reference I would have no reason to expect that the options object would ever have a url property or that I should do anything to manipulate it.

[szmarczak]

So if I pass prefixUrl as a URL is it stringified?

Yes.

Because if it was preserved as a URL, rather than do that I'd probably prefer to pass it as a URL and then just set .port.

This seems like a good idea but note that it would be undefined if there was no prefixUrl. Currently if there's no prefixUrl then it's just an empty string.

It seems like most of the time a method like .mergeInOptions(newOptions) would be suitable, but if I needed to do something less usual I could copy the merged options out of .defaults.options after, or do the more ceremonious .mergeOptions.

Well, Got 12 is going to have an Options class with a method called merge. It should be possible to do options.merge({someOption: value}), but this will be the same as setting it via options.someOption = value (and the latter is faster).

[szmarczak]

Actually I tried

const got = require('got');

const gotInstance = got.extend({
  mutableDefaults: true,
  prefixUrl: "http://example.com:8888",
});

// gotInstance.defaults.options.url = undefined;

const parsedPrefixUrl = new URL(gotInstance.defaults.options.prefixUrl);
parsedPrefixUrl.port = 9999;

// Got 11
gotInstance.defaults.options.prefixUrl = parsedPrefixUrl.href;

and it turns out that it's not possible because options.url is set somehow. This is definitely a bug. Thanks for reporting! I doubt this will be fixed in Got 11 as I'm very focused on Got 12 and getting it released ASAP.

[szmarczak]

because I don't see that property documented. So if not for that reference I would have no reason to expect that the options object would ever have a url property or that I should do anything to manipulate it.

You're right, this should've been properly documented. Got 12 is going to feature a much better documentation.

[jmm]

This seems like a good idea but note that it would be undefined if there was no prefixUrl. Currently if there's no prefixUrl then it's just an empty string.

Ok thanks.

---

Well, Got 12 is going to have an Options class with a method called merge. It should be possible to do options.merge({someOption: value}), but this will be the same as setting it via options.someOption = value (and the latter is faster).

I actually wanted to ask if it's supposed to be supported (I'm talking about v11 here) to set properties of defaults.options directly or only via the merge method? The option setting and URL generation is all over the place right now. For example:

/* 1A) */
const gotInstance = got.extend({
  mutableDefaults: true,
  prefixUrl: "http://example.com",
})

gotInstance.defaults.options = gotInstance.mergeOptions(gotInstance.defaults.options, {
  prefixUrl: "http://example.com:9999",
})

gotInstance("", {timeout: 0}).catch(reason => {
  console.log("", reason.options.url.toString())
})

//=> http://example.com:9999/

/* ... */

/* 1B) */
const gotInstance = got.extend({
  mutableDefaults: true,
  prefixUrl: "http://example.com",
})

gotInstance.defaults.options.prefixUrl = "http://example.com:9999"

//=> Error: Cannot change `prefixUrl` from http://example.com/ to http://example.com:9999: http://example.com/

/* 2A) */
const gotInstance = got.extend({
  mutableDefaults: true,
  prefixUrl: "http://example.com",
})

gotInstance.defaults.options = gotInstance.mergeOptions(gotInstance.defaults.options, {
  port: 9999,
})

gotInstance("", {timeout: 0}).catch(reason => {
  console.log("", reason.options.url.toString())
})

//=> http://example.com/

/* ... */

/* 2B) */
const gotInstance = got.extend({
  mutableDefaults: true,
  prefixUrl: "http://example.com",
})

gotInstance.defaults.options.port = 9999

gotInstance("", {timeout: 0}).catch(reason => {
  console.log("", reason.options.url.toString())
})

//=> http://example.com:9999/

---

Actually I tried [...] and it turns out that it's not possible because options.url is set somehow. This is definitely a bug. Thanks for reporting! I doubt this will be fixed in Got 11 as I'm very focused on Got 12 and getting it released ASAP.

Ok thanks.

For my present use case I think I'm just going to defer setting prefixUrl at all until the point where I was originally going to set the port.

But in the bigger picture, as we're both seeing and these examples show, the behavior is inconsistent / surprising / not intuitive right now. Maybe I'm not thinking of all the angles, but I wonder if it wouldn't be simpler to just base the whole thing on a URL instance and not even have prefixUrl. When using prefixUrl, you can then only make a request for a relative path anyway. So what if setting a prefix URL was just...

got.extend({
  url: "http://example.com",
})

...and url was stored as a URL instance and then if you wanted to modify part of it you could set a specific property, like .port, or overwrite the whole thing. Then when you get to the actual request do new URL(url, instance.defaults.options.url).

You're right, this should've been properly documented. Got 12 is going to feature a much better documentation.

Ok thanks, 🙌 for the docs.

[szmarczak]

Then when you get to the actual request do new URL(url, instance.defaults.options.url).

This has been already discussed and we decided to go with prefixUrl. See #783

[szmarczak]

if it's supposed to be supported (I'm talking about v11 here) to set properties of defaults.options directly or only via the merge method?

It should be possible to do both, but mergeOptions fails (because it generaters url while it should not). So for now you should stick to changing the defaults directly.