res: use http.ServerResponse._header when accessors exist (#930)

* Don't use http.ServerResponse._header when accessors exist Structure of http.ServerResponse._header will change in future Node versions. Avoid reading and setting it directly when helpers exist. * Add new header test case * make things a little more strict
koajs · Mar 8, 2017 · e9d7aba · e9d7aba · yufan-zhao · May 10, 2017
1 parent 188c096
commit e9d7aba
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 3 deletions.
diff --git a/lib/context.js b/lib/context.js
@@ -121,8 +121,16 @@ const proto = module.exports = {
       return;
     }
 
-    // unset all headers, and set those specified
-    this.res._headers = {};
+    const { res } = this;
+
+    // first unset all headers
+    if (typeof res.getHeaderNames === 'function') {
+      res.getHeaderNames().forEach(name => res.removeHeader(name));
+    } else {
+      res._headers = {}; // Node < 7.7
+    }
+
+    // then set those specified
     this.set(err.headers);
 
     // force text/plain

diff --git a/lib/response.js b/lib/response.js
@@ -44,7 +44,10 @@ module.exports = {
    */
 
   get header() {
-    return this.res._headers || {};
+    const { res } = this;
+    return typeof res.getHeaders === 'function'
+      ? res.getHeaders()
+      : res._headers || {};  // Node < 7.7
   },
 
   /**

diff --git a/test/context/onerror.js b/test/context/onerror.js
@@ -1,8 +1,10 @@
 
 'use strict';
 
+const assert = require('assert');
 const request = require('supertest');
 const Koa = require('../..');
+const context = require('../helpers/context');
 
 describe('ctx.onerror(err)', () => {
   it('should respond', done => {
@@ -169,5 +171,23 @@ describe('ctx.onerror(err)', () => {
         .expect('Content-Type', 'text/plain; charset=utf-8')
         .expect('Internal Server Error', done);
     });
+
+    it('should use res.getHeaderNames() accessor when available', () => {
+      let removed = 0;
+      const ctx = context();
+
+      ctx.app.emit = () => {};
+      ctx.res = {
+        getHeaderNames: () => ['content-type', 'content-length'],
+        removeHeader: () => removed++,
+        end: () => {},
+        emit: () => {}
+      };
+
+      ctx.onerror(new Error('error'));
+
+      assert.equal(removed, 2);
+    });
   });
 });
+
diff --git a/test/response/header.js b/test/response/header.js
@@ -1,7 +1,9 @@
 
 'use strict';
 
+const request = require('supertest');
 const response = require('../helpers/context').response;
+const Koa = require('../..');
 
 describe('res.header', () => {
   it('should return the response header object', () => {
@@ -10,6 +12,28 @@ describe('res.header', () => {
     res.header.should.eql({ 'x-foo': 'bar' });
   });
 
+  it('should use res.getHeaders() accessor when available', () => {
+    const res = response();
+    res.res._headers = null;
+    res.res.getHeaders = () => ({ 'x-foo': 'baz' });
+    res.header.should.eql({ 'x-foo': 'baz' });
+  });
+
+  it('should return the response header object when no mocks are in use', async () => {
+    const app = new Koa();
+    let header;
+
+    app.use(ctx => {
+      ctx.set('x-foo', '42');
+      header = Object.assign({}, ctx.response.header);
+    });
+
+    await request(app.listen())
+      .get('/');
+
+    header.should.eql({ 'x-foo': '42' });
+  });
+
   describe('when res._headers not present', () => {
     it('should return empty object', () => {
       const res = response();