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.

Sign up for GitHub

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

Jump to bottom

errors,http_server: migrate to use internal/errors.js #13301

Closed
wants to merge 1 commit into from
Closed

errors,http_server: migrate to use internal/errors.js #13301

wants to merge 1 commit into from

Conversation

bidipyne
Copy link
Contributor

Checklist

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

_http_server.js
internal/errors.js

ref: #11273
I read and understood the contribution guidelines, please review and suggest.
@jasnell
Thanks.

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. labels May 30, 2017
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 30, 2017
jasnell
jasnell reviewed May 30, 2017
View reviewed changes
lib/_http_server.js Outdated
@@ -211,8 +212,8 @@ function writeHead(statusCode, reason, obj) {
}
if (k === undefined) {
if (this._header) {
throw new Error('Can\'t render headers after they are sent to the ' +
'client');
throw new errors.Error('ERR_ASSERTION', 'Can\'t render headers ' +
Copy link
Member

Choose a reason for hiding this comment

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

Since we're updating anyway, it would be nice to get rid of the contraction.

Cannot render headers ...

Copy link
Member

Choose a reason for hiding this comment

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

ERR_ASSERTION is the wrong choice here. That should be reserved for uses of the assert module. A new error code would be more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell Rectifications done. Thank You

jasnell
jasnell reviewed May 30, 2017
View reviewed changes
lib/_http_server.js Outdated
@@ -223,7 +224,8 @@ function writeHead(statusCode, reason, obj) {
}

if (common._checkInvalidHeaderChar(this.statusMessage))
throw new Error('Invalid character in statusMessage.');
throw new errors.Error('ERR_ASSERTION', 'Invalid character in ' +
'statusMessage.');
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, ERR_ASSERTION is the wrong choice here. A new error code would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

jasnell
jasnell reviewed May 31, 2017
View reviewed changes
lib/internal/errors.js Outdated
@@ -117,13 +117,16 @@ E('ERR_CONSOLE_WRITABLE_STREAM',
E('ERR_INDEX_OUT_OF_RANGE', 'Index out of range');
E('ERR_INVALID_ARG_TYPE', invalidArgType);
E('ERR_INVALID_CALLBACK', 'callback must be a function');
E('ERR_INVALID_CHAR', 'Invalid character in statusMessage.');
Copy link
Member

Choose a reason for hiding this comment

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

Since this is specific to http, perhaps ERR_HTTP_INVALID_CHAR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-phrased.

jasnell
jasnell reviewed May 31, 2017
View reviewed changes
lib/internal/errors.js Outdated
E('ERR_INVALID_FILE_URL_HOST', 'File URL host %s');
E('ERR_INVALID_FILE_URL_PATH', 'File URL path %s');
E('ERR_INVALID_HANDLE_TYPE', 'This handle type cannot be sent');
E('ERR_INVALID_OPT_VALUE',
(name, value) => {
return `The value "${String(value)}" is invalid for option "${name}"`;
});
E('ERR_INVALID_STATUS_CODE',
(originalStatusCode) => `Invalid status code: ${originalStatusCode}`);
Copy link
Member

Choose a reason for hiding this comment

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

same here... ERR_HTTP_INVALID_STATUS_CODE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

jasnell
jasnell reviewed May 31, 2017
View reviewed changes
lib/internal/errors.js Outdated
@@ -139,6 +142,8 @@ E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected');
E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe');
E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks');
E('ERR_MISSING_ARGS', missingArgs);
E('ERR_RENDER_HEADER',
'Cannot render headers after they are sent to the client');
Copy link
Member

Choose a reason for hiding this comment

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

ERR_HTTP_HEADERS_SENT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rectified. Thanks!

jasnell
jasnell reviewed May 31, 2017
View reviewed changes
test/parallel/test-http-response-statuscode.js Outdated
@@ -1,3 +1,5 @@
// Flags: --expose-internals
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't need the --expose-internals flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

jasnell
jasnell reviewed May 31, 2017
View reviewed changes
test/parallel/test-http-write-head.js Outdated
@@ -19,6 +19,8 @@
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

// Flag: --expose-internals
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, this shouldn't need the --expose-internals flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

mhdawson
mhdawson approved these changes May 31, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

lib/_http_server.js Outdated
@@ -223,7 +225,8 @@ function writeHead(statusCode, reason, obj) {
}

if (common._checkInvalidHeaderChar(this.statusMessage))
throw new Error('Invalid character in statusMessage.');
throw new errors.Error('ERR_HTTP_INVALID_CHAR', 'Invalid character in ' +
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing its broken into 2 strings to avoid the long line. I'm not sure I've seen us use the + for this, instead could you just put the 'Invalid ...' on a second line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as per suggestion. Thanks!

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/8420/

jasnell pushed a commit that referenced this pull request Jun 2, 2017
@bidipyne @jasnell
errors,http_server: migrate to use internal/errors.js
a9f798e 
PR-URL: #13301
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 2, 2017

Landed in a9f798e

@jasnell jasnell closed this Jun 2, 2017
@lance lance mentioned this pull request Oct 25, 2017
Closed
2 tasks
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bidipyne @jasnell @mhdawson @nodejs-github-bot