Add missing new for errors lib/*.js
#1246
Conversation
|
LGTM |
|
Now that I look at it, there's a bunch of similar |
|
There should probably be an established policy of whether or not to include |
|
Seems a no-brainer to me, when not including |
Not including `new` adds a useless frame and removes a potentially useful frame.
|
Changed my commit to fix the other cases I could find in |
nstepien
changed the title
new for a TypeError in crypto.jsnew for errors lib/*.js
|
changing all error constructor calls LGTM |
I think instead of writing down every little rule, we should just enforce these through a linter. |
|
Also, LGTM |
| @@ -108,12 +108,12 @@ exports.lookup = function lookup(hostname, options, callback) { | |||
|
|
|||
| // Parse arguments | |||
| if (hostname && typeof hostname !== 'string') { | |||
| throw TypeError('invalid arguments: hostname must be a string or falsey'); | |||
| throw new TypeError('invalid arguments: hostname must be a string or falsey'); | |||
There was a problem hiding this comment.
This line is now 82 characters long, could you wrap it?
|
One comment, otherwise LGTM. |
|
@brendanashworth |
Not including `new` adds a useless frame and removes a potentially useful frame. PR-URL: #1246 Reviewed-By: Petka Antonov <petka_antonov@hotmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
|
Thanks @MayhemYDG, this has been merged in |
|
Noticed this commit got merged with a nickname instead of a real name as git It's too late now to fix that, and everyone who LGTM'd that is probably guilty, including me 😇. Just something to keep an look out in the future @iojs/collaborators. |
|
@silverwind $ git config --global user.name "J. Random User""Random User" sounds like a nickname. Either way you got my name in my email address. |
|
Yeah, that's partially at fault too. I'll mention it on the next AUTHORS update. |
|
This also changes |
Although it is optional, when not including `new` you potentially lose a single useful frame in the stack trace and add a useless frame. nodejs/node#1246 Closes #32.
|
Maybe it's better to report this to v8 team, so they could make |
Update punycode to the latest released version. This is mainly in order to further reduce the maintenance burden. In nodejs#1246 a fix introducing `new` to errors was introduced and it has since been ported back to the punycode library. This puts Node back in sync with the library itself so it can receive future fixes and updates directly. PR-URL: Reviewed-By: Reviewed-By:
It doesn't break anything, but it does add an unwanted line in the stack trace.