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: alter ERR_HTTP2_INVALID_CONNECTION_HEADERS #19807

Closed
wants to merge 3 commits into from
Closed

errors: alter ERR_HTTP2_INVALID_CONNECTION_HEADERS #19807

wants to merge 3 commits into from

Conversation

davidmarkclements
Copy link
Member

changes the base instance for ERR_HTTP2_INVALID_CONNECTION_HEADERS
from Error to TypeError as a more accurate representation
of the error.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

errors: alter ERR_HTTP2_INVALID_CONNECTION_HEADERS
b53ac5c 
changes the base instance for ERR_HTTP2_INVALID_CONNECTION_HEADERS
from Error to TypeError as a more accurate representation
of the error.
@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Apr 4, 2018
BridgeAR
BridgeAR reviewed Apr 4, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I guess no test is failing and it would be great if you could add a test in that case as well.

lib/internal/errors.js Outdated
@@ -712,9 +712,8 @@ E('ERR_HTTP2_HEADER_SINGLE_VALUE',
E('ERR_HTTP2_INFO_STATUS_NOT_ALLOWED',
'Informational status codes cannot be used', RangeError);

// This should probably be a `TypeError`.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please also remove the line break above as all other errors are directly beneath each other.

@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 4, 2018
Trott
Trott previously requested changes Apr 4, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

I hate to be the Bringer Of The Red X but I'm really reluctant to overload TypeError like this. This is an invalid value, an invalid object, a malformed parameter, it's all sorts of things but TypeError doesn't seem like the correct description for it.

@BridgeAR
Copy link
Member

BridgeAR commented Apr 4, 2018

@Trott so far we use the TypeError to indicate faulty user input. The same is also done in chrome at times.

@Trott
Copy link
Member

Trott commented Apr 4, 2018

The same is also done in chrome at times.

That could be compelling. Can you point to an example? (Or, of course, point me to where you've already explained this if I'm asking you to repeat something you've already laid out somewhere...)

@BridgeAR
Copy link
Member

BridgeAR commented Apr 4, 2018

I do not have a example handy but I am absolutely certain that I had cases like that before. I can check the V8 code base but I would rather not as it probably takes some time.

@davidmarkclements
Copy link
Member Author

attempting to set the length property of an array if the length property is read only results in a type error:

https://github.com/nodejs/node/blob/master/deps/v8/src/accessors.cc#L189

@Trott Trott dismissed their stale review April 4, 2018 19:47

quietly going away

@BridgeAR BridgeAR requested a review from a team April 9, 2018 10:42
@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2018
mcollina
mcollina approved these changes Apr 9, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM but I would prefer a test.

@targos targos removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 9, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

@davidmarkclements would you be so kind and add a test?

@davidmarkclements
Copy link
Member Author

@BridgeAR checks added

@trivikr
Copy link
Member

trivikr commented Apr 12, 2018

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

mcollina
mcollina approved these changes Apr 12, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Still LGTM

@BridgeAR
Copy link
Member

Landed in a37e267 🎉

@BridgeAR BridgeAR closed this Apr 13, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 13, 2018
@BridgeAR
errors: alter ERR_HTTP2_INVALID_CONNECTION_HEADERS
a37e267 
changes the base instance for ERR_HTTP2_INVALID_CONNECTION_HEADERS
from Error to TypeError as a more accurate representation
of the error.

PR-URL: nodejs#19807
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@davidmarkclements davidmarkclements mentioned this pull request Apr 16, 2018
Closed
3 tasks
jasnell pushed a commit that referenced this pull request Apr 16, 2018
@jasnell
errors: alter ERR_HTTP2_INVALID_CONNECTION_HEADERS
b6fbe16 
changes the base instance for ERR_HTTP2_INVALID_CONNECTION_HEADERS
from Error to TypeError as a more accurate representation
of the error.

PR-URL: #19807
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
@BridgeAR
errors: alter ERR_HTTP2_INVALID_CONNECTION_HEADERS
a0fdfee 
changes the base instance for ERR_HTTP2_INVALID_CONNECTION_HEADERS
from Error to TypeError as a more accurate representation
of the error.

PR-URL: nodejs#19807
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@kjin kjin mentioned this pull request Sep 19, 2018
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

@Trott Trott Trott left review comments

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@davidmarkclements @BridgeAR @Trott @trivikr @mcollina @jasnell @targos @nodejs-github-bot