Ignore Got defaults when extending instances

[ghermeto]

Describe the bug

• Node.js version: 10, 12, 14
• OS & version: MacOS

The value passed to responseType is not being respected the plugin is extended by another plugin that doesn't set it.

Actual behavior

If PluginA sets responseType to json (or something else) and then PluginA extends a PluginB which doesn't set a responseType, the resulting responseType will be text.

Expected behavior

If only one plugin sets the responseType, then that value should be used. If more then one sets it, then it should prefer the extension one.

Code to reproduce

test('extending responseType', withServer, async (t, server) => {
    server.get('/', (req, res) => {
        res.json(req.headers);
    });

    const instance1 = got.extend({
        prefixUrl: server.url,
        responseType: 'json'
    });

    const instance2 = got.extend({
        headers: { 'x-test': 'test' }
    });

    const merged = instance1.extend(instance2);

    const {body} = await merged.get('');
    t.is((body as unknown as Record<string, unknown>)['x-test'], 'test');
});

If, instead, merged is defined as instance2.extend(instance1) the tests pass.

Quick fix

replacing L21-23 from /source/as-promise/normalize-arguments.ts by

    if (options.responseType === undefined) {
        options.responseType = defaults?.responseType ?? 'text';
    } else if (options.responseType === 'text' && defaults?.responseType) {
        options.responseType = defaults.responseType;
    }

will make the test pass.

Checklist

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

[ghermeto]

actually that "quick fix" above will break the following case:

test('extending responseType', withServer, async (t, server) => {
    server.get('/', (req, res) => {
        res.json(req.headers);
    });

    const instance1 = got.extend({
        prefixUrl: server.url,
        responseType: 'json'
    });

    const instance2 = got.extend({
        headers: { 'x-test': 'test' },
                responseType: 'text'
    });

    const merged = instance1.extend(instance2);

    const {body} = await merged.get('');
    t.is((body as unknown as Record<string, unknown>)['x-test'], 'test');
});

In this case, I would expect the responseType to be set to text because it is explicitly overridden.

[szmarczak]

Can you try replacing

https://github.com/sindresorhus/got/blob/2b352d3f9e216e116efc0249a3a78b8d4492f97c/source/as-promise/normalize-arguments.ts#L22

with

        options.responseType = defaults?.responseType ?? 'text';

?

[ghermeto]

I tried to just use options.responseType = defaults?.responseType ?? 'text';, but that code path is never reached because the defaults in source/index.ts will always set the value to text before we get there.

I think removing the default might help... let me try that. I will open a PR

[szmarczak]

Actually this is not a bug. You're calling got.extend() which extends the default Got instance, which has set responseType to text.

[szmarczak]

@sindresorhus Should we ignore Got defaults when extending instances?

[szmarczak]

responseType is an option for the end-user of Got. It should be possible to change it by instance(url, {responseType: 'type'}) and it should not break any plugin.

@ghermeto Do you see any other property that its default value should be ignored when extending instances?

[ghermeto]

@szmarczak how important are the instances defaults?

I ask because it seems like when you call the got function (that makes the HTTP request), it also calls normalizeArgument no matter what. Having the instances hold only the values the user explicitly set (or inherit) would avoid this issue and would also be a performance improvement in some cases.

To clarify what I mean by possible performance improvement:

In a scenario where you build a client on the user's behalf, you can set prefixUrl, retries, responseType at bootstrapping time (or on its own package), but you also need to set up some properties during the request, like distributed tracing span, IPC metrics, headers, etc. In those scenarios, I think got.extend() adds tremendous value, but it also means going through normalizeArgument twice.

But I'm focused on this use-case and maybe I'm missing the purpose of having the instances defaults calculated through normalizeArgument.

[Giotino]

This comment is based on the situation before #1448 which patched (more or less) this behaviour for a single option.

I think the way the extend function is currently implemented is quite limiting.

Example time:

// The options of instanceA are { ...defaults, headers: { test: 'header' } }
const instanceA = extend({ headers: { test: 'header' } });

// The options of instanceB are { ...defaults, prefixUrl: 'http://example.com' }
const instanceB = extend({ prefixUrl: 'http://example.com' });

// And now the problem, when merging the two (with `extend`) defaults are considered the same as the instance options, in fact, options were merged before with the defaults and now there's no way to distinguish them.

// Here're the results of `extend`

// This is nearly the same as doing { ...defaults, prefixUrl: 'http://example.com', ...defaults, headers: { test: 'header' } }
const mergeAonB = instanceB.extend(instanceA);
// prefixUrl is going to disappera because is present in the defaults

// While this is { ...defaults, headers: { test: 'header' }, ...defaults, prefixUrl: 'http://example.com' }
const mergeBonA = instanceA.extend(instanceB);
// Which is fine because headers is present in defaults but merged as enumerable

As you can see mergeAonB is wrong, this is going to happen with a lot of options, not only with prefixUrl.

I think extend should only extend user options and not the defaults.

const instanceC = got.extend({
  prefixUrl: 'http://example.org'
});

const options = {
  headers: { test: 'header' }
};
const instanceD = got.extend(options);

const instanceE = instanceC.extend(options);
const instanceF = instanceC.extend(instanceD);

In this example instanceE and instanceF should have the same options.

My ideal result of mergeAonB is { ...defaults, prefixUrl: 'http://example.com', headers: { test: 'header' } } (there's not the second default unpacking).

My idea of reimplementation of extend would be not merging the options with defaults until right before the request. With this only the user specified options are going to be merged on extend and the defaults are applied before the request.

How my idea is different in results from the current state?

// The options of instanceA are { headers: { test: 'header' } }
const instanceA = extend({ headers: { test: 'header' } });
// When the request is made the options would be { ...defaults, headers: { test: 'header' } }

// The options of instanceB are { ...defaults, prefixUrl: 'http://example.com' }
const instanceB = extend({ prefixUrl: 'http://example.com' });
// When the request is made the options would be { ...defaults, prefixUrl: 'http://example.com' }

// Here're the results of `extend`

// This is nearly the same as doing { prefixUrl: 'http://example.com', headers: { test: 'header' } }
// Because it merges (in order) Boptions, Aoptions
const mergeAonB = instanceB.extend(instanceA);
// When the request is made the options would be { ...defaults, prefixUrl: 'http://example.com', headers: { test: 'header' } }
// `prefixUrl` is preserved

// While this is { ...defaults, headers: { test: 'header' }, prefixUrl: 'http://example.com' }
const mergeBonA = instanceA.extend(instanceB);
// When the request is made the options would be { ...defaults,  headers: { test: 'header' },prefixUrl: 'http://example.com' }
// Which is fine as always

On extend options should be merged with existing options (no defaults) and checked, on request options should be merged with defaults. check and merge are basically the normalizeArguments function splitted.

The only thing that could be a problem is the defualts.options getter.

[szmarczak]

/cc @sindresorhus