Remove body argument to header functions

[paulmelnikow]

Currently the third argument to header functions receives a buffer containing the response body. I can't think of a reason why this would be helpful.

I could imagine the request body being helpful, though that should be accessible from the request.

I'm inclined to remove this parameter from the function in a breaking change.

See #1541 where I added a test covering the current behavior.

[gr2m]

If this is undocumented and untested, I’d be okay to remove it without a breaking change.

If it breaks someone’s code it’s fair play to say it was never documented / tested ¯\_(ツ)_/¯ Documenting it as breaking change is going the extra mile. Go if you like, but I don’t feel like we have to

[paulmelnikow]

It is tested and documented – just not useful!

Like many of the tests, the intention of the test (which I changed in #1541) was not quite clear, and I can't say it left me confident someone intended to pass the response body to the callback, vs the test containing a bug.

The behavior is documented, ambiguously:

… you can use a function to generate the headers values. The function will be passed the request, response, and body (if available). The body will be either a buffer, a stream, or undefined.

[paulmelnikow]

Oops! I accidentally pushed this branch and it let me. I stopped the Travis builld which should prevent the release, though I can't force push because of branch protection.

Should I turn it off temporarily and force push the commit before?

Can we turn on branch protection so I can try to prevent a repeat performance? 😝

[gr2m]

Sure, go ahead 👍

[paulmelnikow]

Okay, done.

I can't find an option to prevent accidentally pushing to the branch. I can be careful… though I'd rather find a way to do that. Am I missing something? Is it only possible to do that if "Include administrators" is checked?

I can try to be more careful about it…

[paulmelnikow]

Huh, there's a test that's failing in #1544. In this one it's super clear that the response body is intended to be passed to the header function.

nock/tests/test_default_reply_headers.js

Lines 46 to 66 in 560a5d8

	test('default reply headers as functions work', async t => {
	const date = new Date().toUTCString()
	const message = 'A message.'
	nock('http://example.test')
	.defaultReplyHeaders({
	'Content-Length': (req, res, body) => body.length,
	Date: () => date,
	Foo: () => 'foo',
	})
	.get('/')
	.reply(200, message, { foo: 'bar' })
	const { headers } = await got('http://example.test')
	t.deepEqual(headers, {
	'content-length': message.length,
	date,
	foo: 'bar',
	})
	})

Before removing this, I want to see if I can find any more information about why this use case was added. I'm still inclined to remove it as it seems very specific and something that can be accomplished around in other ways.

[paulmelnikow]

Okay, tracked this down. This was added in #315 in response to #263. I can see for the Content-Length use case that it would be useful to have the processed response available. Instead of removing this behavior, I'm updating #1544 to improve the documentation, tests, and examples.

[nockbot]

🎉 This issue has been resolved in version 11.0.0-beta.16 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[nockbot]

🎉 This issue has been resolved in version 11.0.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀