Don't throw HTTPError on 304 responses #252
Conversation
lukechilds
force-pushed
the
allow-304
branch
from
4a3588c
to
aceed32
Compare
|
@sindresorhus Continuing discussion from here: #251 (comment) I've added a test 👍 When writing it I noticed that the mock 404 route is wrapped with 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 |
Nah, you don't need. According to @floatdrop Let's just remove that setTimeout now? I don't think it applies anymore.
Expecting users to know something is like asking for support questions. Doesn't hurt to have a quick reminder about it. |
test/http.js
Outdated
| t.is(response.body, ''); | ||
| } catch (err) { | ||
| t.fail('Exception was thrown'); | ||
| } |
There was a problem hiding this comment.
Use t.notThrows rather than try/catch:
const p = got(`${s.url}/304`)
await t.notThrows(p);
const response = await p;
t.is(response.statusCode, 304);
t.is(response.body, '');
test/http.js
Outdated
| @@ -56,6 +61,16 @@ test('error with code', async t => { | |||
| } | |||
| }); | |||
|
|
|||
| test('304 doesn\'t throw', async t => { | |||
There was a problem hiding this comment.
status code 304 doesn't throw
|
@sindresorhus yeah, it can be removed now. Tests were failing without it on 0.10/0.12 I think. |
|
Ok, I've made those changes. @sindresorhus is the wording/position in the readme ok? |
|
Yup. Looks good. Thanks again :) |
Resolves #251