Repository: sindresorhus/got
Pull request #252 (2016-12-20 11:44:36)

Don't throw HTTPError on 304 responses
Resolves https://github.com/sindresorhus/got/issues/251

lukechilds: @sindresorhus Continuing discussion from here: https://github.com/sindresorhus/got/issues/251#issuecomment-285131422

I've added a test 👍

When writing it I noticed that the [mock 404 route](https://github.com/sindresorhus/got/blob/a6907b439919166a3f5d9d18cdef3afc5440245f/test/http.js#L18-L23) is wrapped with `setTimeout`? Why is this? Tests still pass if I comment out `setTimout`. Should I be doing that for my 304 route?

Regarding the readme update, is it really necessary to mention a 304 response will have an empty body in the readme? That's normal HTTP behaviour, if people are sending `if-modified-since` headers then they are gonna be expecting to handle 304s.
sindresorhus: > When writing it I noticed that the mock 404 route is wrapped with setTimeout? Why is this? Tests still pass if I comment out setTimout. Should I be doing that for my 304 route?

Nah, you don't need. According to `git blame`, it was added to workaround some flakyness. Probably related to older Node.js versions. https://github.com/sindresorhus/got/commit/46d0d4a4f404d633beb4a5235cf2fec1e9b4fc1a

@floatdrop Let's just remove that setTimeout now? I don't think it applies anymore.

> Regarding the readme update, is it really necessary to mention a 304 response will have an empty body in the readme? That's normal HTTP behaviour, if people are sending if-modified-since headers then they are gonna be expecting to handle 304s.

Expecting users to know something is like asking for support questions. Doesn't hurt to have a quick reminder about it.
floatdrop: @sindresorhus yeah, it can be removed now. Tests were failing without it on 0.10/0.12 I think.
lukechilds: Ok, I've made those changes.

@sindresorhus is the wording/position in the readme ok?

Patch:
diff --git a/index.js b/index.js
index 757380e..b7cab05 100644
--- a/index.js
+++ b/index.js
@@ -119,7 +119,7 @@ function asPromise(opts) {
 						}
 					}
 
-					if (statusCode < 200 || statusCode > limitStatusCode) {
+					if (statusCode !== 304 && (statusCode < 200 || statusCode > limitStatusCode)) {
 						throw new got.HTTPError(statusCode, opts);
 					}
 
@@ -178,7 +178,7 @@ function asStream(opts) {
 
 		res.pipe(output);
 
-		if (statusCode < 200 || statusCode > 299) {
+		if (statusCode !== 304 && (statusCode < 200 || statusCode > 299)) {
 			proxy.emit('error', new got.HTTPError(statusCode, opts), null, res);
 			return;
 		}
@@ -336,7 +336,8 @@ function stdError(error, opts) {
 		hostname: opts.hostname,
 		method: opts.method,
 		path: opts.path,
-		protocol: opts.protocol
+		protocol: opts.protocol,
+		url: opts.href
 	});
 }
 
diff --git a/readme.md b/readme.md
index 03846a1..2a0877e 100644
--- a/readme.md
+++ b/readme.md
@@ -176,7 +176,7 @@ Sets `options.method` to the method name and makes a request.
 
 ## Errors
 
-Each error contains (if available) `statusCode`, `statusMessage`, `host`, `hostname`, `method` and `path` properties to make debugging easier.
+Each error contains (if available) `statusCode`, `statusMessage`, `host`, `hostname`, `method`, `path`, `protocol` and `url` properties to make debugging easier.
 
 In Promise mode, the `response` is attached to the error.
 
@@ -340,7 +340,9 @@ request(`https://${config.host}/production/`, {
 ```
 
 
-## Tip
+## Tips
+
+### User Agent
 
 It's a good idea to set the `'user-agent'` header so the provider can more easily see how their resource is used. By default, it's the URL to this repo.
 
@@ -355,6 +357,10 @@ got('todomvc.com', {
 });
 ```
 
+### 304 Responses
+
+Bear in mind, if you send an `if-modified-since` header and receive a `304 Not Modified` response, the body will be empty. It's your responsibility to cache and retrieve the body contents.
+
 
 ## Related
 


Test patch:
diff --git a/test/error.js b/test/error.js
index 40306d2..ff0f385 100644
--- a/test/error.js
+++ b/test/error.js
@@ -28,6 +28,7 @@ test('properties', async t => {
 		t.is(err.host, `${s.host}:${s.port}`);
 		t.is(err.method, 'GET');
 		t.is(err.protocol, 'http:');
+		t.is(err.url, err.response.requestUrl);
 	}
 });
 
diff --git a/test/http.js b/test/http.js
index 3e51c2e..6b0c997 100644
--- a/test/http.js
+++ b/test/http.js
@@ -15,6 +15,11 @@ test.before('setup', async () => {
 		res.end();
 	});
 
+	s.on('/304', (req, res) => {
+		res.statusCode = 304;
+		res.end();
+	});
+
 	s.on('/404', (req, res) => {
 		res.statusCode = 404;
 		res.end('not');
@@ -54,6 +59,14 @@ test('error with code', async t => {
 	}
 });
 
+test('status code 304 doesn\'t throw', async t => {
+	const p = got(`${s.url}/304`);
+	await t.notThrows(p);
+	const response = await p;
+	t.is(response.statusCode, 304);
+	t.is(response.body, '');
+});
+
 test('buffer on encoding === null', async t => {
 	const data = (await got(s.url, {encoding: null})).body;
 	t.truthy(Buffer.isBuffer(data));