Skip to content

Commit

Permalink
fix: merge querystring with params (#53)
Browse files Browse the repository at this point in the history
  • Loading branch information
stephenplusplus authored and JustinBeckwith committed Mar 5, 2019
1 parent 9c34810 commit a4dfc01
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 9 deletions.
20 changes: 13 additions & 7 deletions src/gaxios.ts
Expand Up @@ -126,6 +126,19 @@ export class Gaxios {
opts.url = baseUrl + opts.url;
}

const parsedUrl = new URL(opts.url);

This comment has been minimized.

Copy link
@AndrewSolonovich

AndrewSolonovich Mar 5, 2019

This brakes google/storage, in some cases returns "Error: URL is not a constructor".
Using gitlab with Docker executor with image node:6.10.0.

This comment has been minimized.

Copy link
@stephenplusplus

stephenplusplus Mar 5, 2019

Author Contributor

Sorry, it looks like URL was not supported until Node@6.13. I will send a fix for this now. In the meantime, if upgrading to 6.13 is an option, that should address the issue immediately.

Thank you for reporting this!

This comment has been minimized.

Copy link
@JustinBeckwith

JustinBeckwith Mar 5, 2019

Contributor

Realistically - we can't support older versions of node.js 6 - which is about to go EOL in less than a month! Is there a way you can upgrade?

This comment has been minimized.

Copy link
@AndrewSolonovich

AndrewSolonovich Mar 6, 2019

Yes, there is a way, and I'll do this.
But I'm not sure if this is your bug or google/storage issue.. Imagine - this commit broke official npm package for google storage :) for node <6.13 because they have gaxios in its dependencies.

Potentially a lot of nodejs code working with google cloud is affected by this (which runs on node <6.13 of course)

opts.url = `${parsedUrl.origin}${parsedUrl.pathname}`;
opts.params = extend(
qs.parse(parsedUrl.search.substr(1)), // removes leading ?
opts.params);

opts.paramsSerializer = opts.paramsSerializer || this.paramsSerializer;
if (opts.params) {
parsedUrl.search = opts.paramsSerializer(opts.params);
}

opts.url = parsedUrl.href;

if (typeof options.maxContentLength === 'number') {
opts.size = options.maxContentLength;
}
Expand All @@ -147,19 +160,12 @@ export class Gaxios {
}

opts.validateStatus = opts.validateStatus || this.validateStatus;
opts.paramsSerializer = opts.paramsSerializer || this.paramsSerializer;
opts.responseType = opts.responseType || 'json';
if (!opts.headers['Accept'] && opts.responseType === 'json') {
opts.headers['Accept'] = 'application/json';
}
opts.method = opts.method || 'GET';

if (opts.params) {
const parts = new URL(opts.url);
parts.search = opts.paramsSerializer(opts.params);
opts.url = parts.href;
}

const proxy = loadProxy();
if (proxy) {
if (this.agentCache.has(proxy)) {
Expand Down
27 changes: 25 additions & 2 deletions test/test.getch.ts
Expand Up @@ -123,7 +123,17 @@ describe('🥁 configuration options', () => {
assert.strictEqual(response, res);
});

it('should encode query string parameters', async () => {
it('should encode URL parameters', async () => {
const path = '/?james=kirk&montgomery=scott';
const opts = {url: `${url}${path}`};
const scope = nock(url).get(path).reply(200, {});
const res = await request(opts);
assert.strictEqual(res.status, 200);
assert.strictEqual(res.config.url, url + path);
scope.done();
});

it('should encode parameters from the params option', async () => {
const opts = {url, params: {james: 'kirk', montgomery: 'scott'}};
const path = '/?james=kirk&montgomery=scott';
const scope = nock(url).get(path).reply(200, {});
Expand All @@ -133,14 +143,27 @@ describe('🥁 configuration options', () => {
scope.done();
});

it('should merge URL parameters with the params option', async () => {
const opts = {
url: `${url}/?james=beckwith&montgomery=scott`,
params: {james: 'kirk'}
};
const path = '/?james=kirk&montgomery=scott';
const scope = nock(url).get(path).reply(200, {});
const res = await request(opts);
assert.strictEqual(res.status, 200);
assert.strictEqual(res.config.url, url + path);
scope.done();
});

it('should allow overriding the param serializer', async () => {
const qs = '?oh=HAI';
const params = {james: 'kirk'};
const opts: GaxiosOptions = {
url,
params,
paramsSerializer: (ps) => {
assert.deepStrictEqual(params, ps);
assert.deepEqual(params, ps);
return '?oh=HAI';
}
};
Expand Down

0 comments on commit a4dfc01

Please sign in to comment.