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

lib: refactor ServerResponse.prototype.writeHead #32399

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,48 +237,50 @@ ServerResponse.prototype._implicitHeader = function _implicitHeader() {
this.writeHead(this.statusCode);
};

ServerResponse.prototype.writeHead = writeHead;
function writeHead(statusCode, reason, obj) {
ServerResponse.prototype.writeHead = function(
statusCode,
statusMessage,
headers
) {
const originalStatusCode = statusCode;

statusCode |= 0;
if (statusCode < 100 || statusCode > 999) {
throw new ERR_HTTP_INVALID_STATUS_CODE(originalStatusCode);
}


if (typeof reason === 'string') {
// writeHead(statusCode, reasonPhrase[, headers])
this.statusMessage = reason;
} else {
if (headers === undefined && typeof statusMessage === 'object') {
// writeHead(statusCode[, headers])
if (!this.statusMessage)
this.statusMessage = STATUS_CODES[statusCode] || 'unknown';
obj = reason;
headers = statusMessage;
} else if (typeof statusMessage === 'string') {
// writeHead(statusCode, reasonPhrase[, headers])
this.statusMessage = statusMessage;
}
if (this.statusMessage == null)
this.statusMessage = STATUS_CODES[statusCode] || 'unknown';
himself65 marked this conversation as resolved.
Show resolved Hide resolved
this.statusCode = statusCode;

let headers;
let _headers;
if (this[kOutHeaders]) {
// Slow-case: when progressive API and header fields are passed.
let k;
if (obj) {
const keys = ObjectKeys(obj);
if (headers) {
const keys = ObjectKeys(headers);
// Retain for(;;) loop for performance reasons
// Refs: https://github.com/nodejs/node/pull/30958
for (let i = 0; i < keys.length; i++) {
k = keys[i];
if (k) this.setHeader(k, obj[k]);
if (k) this.setHeader(k, headers[k]);
}
}
if (k === undefined && this._header) {
throw new ERR_HTTP_HEADERS_SENT('render');
}
// Only progressive api is used
headers = this[kOutHeaders];
_headers = this[kOutHeaders];
} else {
// Only writeHead() called
headers = obj;
_headers = headers;
}

if (checkInvalidHeaderChar(this.statusMessage))
Expand Down Expand Up @@ -307,10 +309,10 @@ function writeHead(statusCode, reason, obj) {
this.shouldKeepAlive = false;
}

this._storeHeader(statusLine, headers);
this._storeHeader(statusLine, _headers);

return this;
}
};

// Docs-only deprecated: DEP0063
ServerResponse.prototype.writeHeader = ServerResponse.prototype.writeHead;
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http-mutable-headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ const s = http.createServer(common.mustCall((req, res) => {
case 'writeHead':
res.statusCode = 404;
res.setHeader('x-foo', 'keyboard cat');
res.writeHead(200, { 'x-foo': 'bar', 'x-bar': 'baz' });
res.writeHead(200, undefined, { 'x-foo': 'bar', 'x-bar': 'baz' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also keep the test case for writeHead(status[, headers])?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm looking for a suitable test file to add this new case

Copy link
Member

@ronag ronag Jun 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@himself65 ping, did you get any further with this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no, I have forgot about this...

break;

default:
Expand Down