fix: do not double prepend baseUrl on retry

[JustinBeckwith]

Fixes #141

[codecov]

Codecov Report

Merging #142 into master will decrease coverage by 0.3%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
- Coverage   96.85%   96.55%   -0.31%     
==========================================
  Files           4        4              
  Lines         572      580       +8     
  Branches       96       98       +2     
==========================================
+ Hits          554      560       +6     
- Misses         17       19       +2     
  Partials        1        1

Impacted Files	Coverage Δ
src/gaxios.ts	93.33% <77.77%> (-0.6%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7137a2d...a81aa0b. Read the comment docs.

[bcoe]

we discussed in air a potential improvement, of switching to a shallow clone for local opts which removes baseUrl/baseURL as a parameter, such that it's not passed on retry.

Everything else looks good to me.

[JustinBeckwith]

Coming back around ... I better understand the problem now. This issue is this:

• The request method calls validateOpts, which merges the default options (baseUrl in this case) with the per-request options
• The catch handler in that method calls this.request again, which does the merge again. This means we can't just shallow clone our way out.
• The second time through the loop, the baseUrl is applied again

I can think of two ways to fix this:

• create an internal private doRequest method, and separate that from the request method. The request method would do the options validation and merging, and the _doRequest method is the one that gets retried in the case of a failure.
• keep it the way it is now, using the startsWith hacky.

@stephenplusplus the problem here wasn't that the option wasn't retained ... it's that it got prepended once for every time we retried the request.

[bcoe]

If we go with the approach of simply checking for baseUrl as a prefix, perhaps we could perform an additional check to ensure that we're not re-appending a scheme:

we never want:

https://www.baseurl.com/foo/http://www.oldurl.com

Perhaps we could do:

const {parse, resolve} = require("url");
if (baseUrl && !opts.url.startsWith(baseUrl)) {
  opts.url  = resolve(baseUrl, parse(opts.url).path)
}

Or perhaps if a scheme already exists in opts.url, it should take precedence ... this might be the safest change:

const {resolve} = require('url')
if (baseUrl && !opts.url.startsWith(baseUrl) && !/https?:\/\//.test(opts.url)) {
  opts.url  = resolve(baseUrl, opts.url)
}

[bcoe]

I'm fine with the simple approach, my only comment would be that I think we can tighten the logic up a bit, with something like:

const {resolve} = require('url')
if (baseUrl && !opts.url.startsWith(baseUrl) && !/https?:\/\//.test(opts.url)) {
  opts.url  = resolve(baseUrl, opts.url)
}

If opts.url is already a fully qualified URL, don't try to append, and use resolve to append the baseURL and opts.url, such that http://www.foo.com/bar appends properly to bar/foo.

[fivechjr]

Just a suggestion; Maybe we can just add another parameter to validateOpts?

For example, isRetrying:

private validateOpts(options: GaxiosOptions, isRetrying: boolean): GaxiosOptions

Then we can use it like this:

if (baseUrl && !isRetrying) {
      opts.url = baseUrl + opts.url;
}