Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge context #1443

Closed
1 task done
ghermeto opened this issue Sep 9, 2020 · 2 comments · Fixed by #1459
Closed
1 task done

Merge context #1443

ghermeto opened this issue Sep 9, 2020 · 2 comments · Fixed by #1459
Assignees
Labels
enhancement This change will extend Got features
Milestone

Comments

@ghermeto
Copy link
Contributor

ghermeto commented Sep 9, 2020

What problem are you trying to solve?

Right now you can't use more than 1 plugin that makes use of context. For example, the following test case fails:

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

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

	const instance2 = instance1.extend({
		headers: { 'x-test': 'test' },
		context: { something: 'else' }
	});

	const instance3 = instance2.extend({
		hooks: {
			beforeRequest: [
				function injectClientInfo(options) {
					t.is(options.context.test, true)
				}
			]
		}
	});

	await instance3.get('');
});

Right now, context is not merged and not enumerable. This causes the plugin represented by instance2 to remove the context expected by instance1.

Describe the feature

Do a shallow merge on context with "defaults" in /core/index.ts and make it enumerable.

Implementation

I have availability to implement this feature if you decide this is a use-case you would like to support.

Checklist

  • I have read the documentation and made sure this feature doesn't already exist.
@sindresorhus sindresorhus added the bug Something does not work as it should label Sep 9, 2020
@szmarczak
Copy link
Collaborator

From the docs:

Note: The object is never merged, it's just passed through. Got will not modify the object in any way.

You can merge the options in the following:

	const instance2 = instance1.extend({
		headers: { 'x-test': 'test' },
		context: {
                    ...instance1.defaults.options.context,
                    something: 'else'
                }
	});

This change is rather a minor change, although it could potentially break some apps (I don't think that's the case). I'm happy to send a PR.

@szmarczak szmarczak added enhancement This change will extend Got features and removed bug Something does not work as it should labels Sep 9, 2020
@szmarczak szmarczak self-assigned this Sep 9, 2020
@ghermeto
Copy link
Contributor Author

Hi @szmarczak, I submitted a PR for this issue and you are right, in the example above you can solve it that way. However, it was a bad example in my part as the users might not have access to the plugin code(and in my case, they won't). I think the example in the PR better describes the issue.

Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants