Tracking issue: Migrate errors to internal/errors.js #17709

joyeecheung · 2017-12-16T10:28:15Z

There are a few old-style errors added since #11273 opened. Opening a new issue to track the migration. I have excluded errors that have existing code property as changing those could result in bigger breakages.

On how to migrate those errors, see the guide: Using the internal/errors.js Module

lib/_http_outgoing.js - lib, test: migrate _http_outgoing.js's remaining errors to internal/errors.js #17837
- new Error('write after end'): can be migrated to ERR_STREAM_WRITE_AFTER_END
- new Error('Cannot pipe, not readable')): can be migrated to ERR_STREAM_CANNOT_PIPE
lib/_tls_legacy.js - tls: comment about old-style errors #17759
- ~~new Error('TLS session renegotiation attack detected'): can be migrated to ERR_TLS_SESSION_ATTACK~~ should not be migrated because it's being deprecated. Add a comment there instead.
lib/_tls_wrap.js - _tls_wrap: Migrate the errors to internal/errors #17709 #17792
- new Error('TLS session renegotiation disabled for this socket'): needs a new error, maybe ERR_TLS_RENEGOTIATION_DISABLED
lib/fs.js - lib: migrate errors to internal/errors #17719
- new Error('Invalid data')): can be migrated to ERR_INVALID_ARG_TYPE
lib/net.js - net: migrate errors to internal/errors #17766
- new Error('This socket is closed'), cb): can be migrated to ERR_SOCKET_CLOSED
- new Error('Not running')): needs a new error, maybe ERR_SERVER_NOT_RUNNING
lib/repl.js - repl: migrate errors to internal/errors #17716
- new Error('Script execution interrupted.')): needs a new error, maybe ERR_SCRIPT_EXECUTION_INTERRUPTED

The text was updated successfully, but these errors were encountered:

joyeecheung · 2017-12-16T10:32:21Z

Just like #11273 this should be a good first issue. People who want to start working on this can post a reply here, and cc @jasnell and me in the PR. There is a guide on how to migrate the errors, also please read the contributing guide on how to format the commit and the PR process. It's ok if you want to take multiple tasks in the OP but I would prefer one file per person so more people get the opportunity to start contributing to core.

The tests would almost definitely fail after the errors in lib have been changed, so please fix the tests accordingly and make sure make test passes before submitting the PR. If no tests fail after the change, please add one by following the guide

mannanali413 · 2017-12-16T15:02:12Z

@joyeecheung @jasnell i can take these up.

ramsgoli · 2017-12-17T02:38:16Z

@joyeecheung @jasnell I can take one of these files as well

b0yfriend · 2017-12-17T06:57:33Z

@joyeecheung @jasnell I'd like to tackle lib/_http_outgoing.js's error migration!

joyeecheung · 2017-12-17T08:53:05Z

@ramsgoli @mannanali413 Hi, please indicate which file you are going to work on in this thread to avoid stepping over each other's toes, thanks!

kysnm · 2017-12-17T08:56:16Z

@joyeecheung @jasnell I would like to take the lib/repl.js

kysnm · 2017-12-17T16:51:43Z

@joyeecheung @jasnell If anyone doesn't take the lib/net.js, I would like to take it.

mannanali413 · 2017-12-17T17:43:58Z

@jasnell I will take up lib/_tls_wrap.js

styfle · 2017-12-17T18:09:22Z

@joyeecheung @jasnell I will take lib/fs.js

jasnell · 2017-12-17T18:33:58Z

@styfle ... Heads up, I'm currently doing quite a bit of work in fs.js, including hitting the rest of the error codes

styfle · 2017-12-17T19:05:11Z

@jasnell Should I take a different file or are you doing them all??

Xavier-J-Ortiz · 2017-12-17T20:37:11Z

I'd like to have a go at _tls_legacy.js!

This would be my first contribution to an open source project if I can figure it out. :)

…f noticed something wrong.

Xavier-J-Ortiz · 2017-12-17T22:48:14Z

After reading the Using the internal/errors.js Module I think I got the idea.

Xavier-J-Ortiz@b251e5c

I checked to see within that internal/errors document if any tests are necessary. I didn't change the error message to be anything other than a string, so I think that should not need any tests.

I'm not sure if I owe any kind of test tests outside of this. Let me know if some test is needed for this, and I'll take a stab at writing it too, though I might need some guidance on it.

joyeecheung · 2017-12-18T06:31:34Z

@Xavier-J-Ortiz Usually the migrated error should have a test (using common.expectsError) testing that error has the expected code. The point of migrating these errors is that the users can rely on err.code instead of err.message and if the PR does not test err.code at all, we can't be sure the error is migrated properly.

Can you read the contributing guide and open a PR with your changes? Thanks!

Xavier-J-Ortiz · 2017-12-18T14:51:06Z

@joyeecheung Thank you for pointing that out.

I'll read documentation, the contributing guide you pointed out, as well as try to find where the error tests are located in order to test err.code and write a test for it.
you mentioned opening a PR for the changes. Should I open it right now with what I've done at the moment, or should I create the PR after I create a test.

Thanks again for the guidance so far joyeecheung !

apapirovski · 2017-12-18T19:47:29Z

@joyeecheung I'm not certain if we should migrate _tls_legacy.js over. It's already deprecated (or, more accurately, the createSecurePair that is exported from there), it seems strange to potentially force users to update their code that uses it to account for a change in error. Given the length of time it has been deprecated for, it's possible we might even consider removing it in 10 or 11? Just thinking out loud here...

joyeecheung · 2017-12-19T15:23:06Z

@apapirovski That sounds reasonable to me. In that case, maybe we should add a comment there about why this is throwing old-style errors (I am planning to write a lint rule for that). @Xavier-J-Ortiz sorry for the fuss, can you change your PR to do that instead? Thanks.

Xavier-J-Ortiz · 2017-12-19T16:34:28Z

@joyeecheung Not a problem or a fuss. :)

This is the PR I created just now for this. #17759

Let me know if this is what you were thinking in terms of the comment.

Old style errors are being migrated to internal/errors.js, however, due to depreciation of _tls_legacy.js, it isn't worth the effort to migrate and potentially force users to update their code for this error change. This comment clarifies the reason why this error is not migrated. Refs:nodejs#17709 (comment)

Old style errors are being migrated to internal/errors.js, however, due to depreciation of _tls_legacy.js, it isn't worth the effort to migrate and potentially force users to update their code for this error change. This comment clarifies the reason why this error is not migrated. PR-URL: #17759 Refs: #17709 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

Remove the extra space in the message corresponding to the key ERR_TLS_RENEGOTIATION_DISABLED . Fixes: nodejs#17709

This migrates the old style error in _tls_wrap.js to the new style error ERR_TLS_RENEGOTIATION_DISABLED. Refs: #17709 PR-URL: #17792 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Throw ERR_INVALID_ARG_TYPE when validating arguments passed to WriteStream.prototype._write. Refs: #17709 PR-URL: #17719 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Throw ERR_SOCKET_CLOSED and ERR_SERVER_NOT_RUNNING instead of the old-style errors in net.js. PR-URL: #17766 Refs: #17709 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

A couple of lib/_http_outgoing.js's errors were still in the "old style": `throw new Error(<some message here>)`. This commit migrates those 2 old style errors to the "new style": internal/errors.js's error-system. In the future, changes to these errors' messages won't break semver-major status. With the old style, changes to these errors' messages broke semver-major status. It was inconvenient. Refs: #17709 PR-URL: #17837 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

joyeecheung · 2017-12-27T19:22:06Z

All the errors in the OP have been migrated, thanks everyone!

Old style errors are being migrated to internal/errors.js, however, due to depreciation of _tls_legacy.js, it isn't worth the effort to migrate and potentially force users to update their code for this error change. This comment clarifies the reason why this error is not migrated. PR-URL: #17759 Refs: #17709 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

joyeecheung added errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory. good first issue Issues that are suitable for first-time contributors. labels Dec 16, 2017

joyeecheung added the mentor-available label Dec 16, 2017

kysnm mentioned this issue Dec 17, 2017

repl: migrate errors to internal/errors #17716

Closed

4 tasks

styfle mentioned this issue Dec 17, 2017

lib: migrate errors to internal/errors #17719

Closed

3 tasks

Xavier-J-Ortiz referenced this issue in Xavier-J-Ortiz/node Dec 17, 2017

I believe I Migrated error the error. Will check and do any changes i…

b251e5c

…f noticed something wrong.

kysnm mentioned this issue Dec 19, 2017

net: migrate errors to internal/errors #17766

Closed

4 tasks

mannanali413 added a commit to mannanali413/node that referenced this issue Dec 23, 2017

api/errors.md: Remove extra space in the message

b18af91

Remove the extra space in the message corresponding to the key ERR_TLS_RENEGOTIATION_DISABLED . Fixes: nodejs#17709

b0yfriend mentioned this issue Dec 23, 2017

lib, test: migrate _http_outgoing.js's remaining errors to internal/errors.js #17837

Closed

3 tasks

joyeecheung closed this as completed Dec 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracking issue: Migrate errors to internal/errors.js #17709

Tracking issue: Migrate errors to internal/errors.js #17709

joyeecheung commented Dec 16, 2017 •

edited

Loading

joyeecheung commented Dec 16, 2017 •

edited

Loading

mannanali413 commented Dec 16, 2017

ramsgoli commented Dec 17, 2017

b0yfriend commented Dec 17, 2017

joyeecheung commented Dec 17, 2017

kysnm commented Dec 17, 2017 •

edited

Loading

kysnm commented Dec 17, 2017 •

edited

Loading

mannanali413 commented Dec 17, 2017

styfle commented Dec 17, 2017

jasnell commented Dec 17, 2017

styfle commented Dec 17, 2017

Xavier-J-Ortiz commented Dec 17, 2017 •

edited

Loading

Xavier-J-Ortiz commented Dec 17, 2017 •

edited

Loading

joyeecheung commented Dec 18, 2017

Xavier-J-Ortiz commented Dec 18, 2017 •

edited

Loading

apapirovski commented Dec 18, 2017

joyeecheung commented Dec 19, 2017 •

edited

Loading

Xavier-J-Ortiz commented Dec 19, 2017 •

edited

Loading

joyeecheung commented Dec 27, 2017 •

edited

Loading

Tracking issue: Migrate errors to internal/errors.js #17709

Tracking issue: Migrate errors to internal/errors.js #17709

Comments

joyeecheung commented Dec 16, 2017 • edited Loading

joyeecheung commented Dec 16, 2017 • edited Loading

mannanali413 commented Dec 16, 2017

ramsgoli commented Dec 17, 2017

b0yfriend commented Dec 17, 2017

joyeecheung commented Dec 17, 2017

kysnm commented Dec 17, 2017 • edited Loading

kysnm commented Dec 17, 2017 • edited Loading

mannanali413 commented Dec 17, 2017

styfle commented Dec 17, 2017

jasnell commented Dec 17, 2017

styfle commented Dec 17, 2017

Xavier-J-Ortiz commented Dec 17, 2017 • edited Loading

Xavier-J-Ortiz commented Dec 17, 2017 • edited Loading

joyeecheung commented Dec 18, 2017

Xavier-J-Ortiz commented Dec 18, 2017 • edited Loading

apapirovski commented Dec 18, 2017

joyeecheung commented Dec 19, 2017 • edited Loading

Xavier-J-Ortiz commented Dec 19, 2017 • edited Loading

joyeecheung commented Dec 27, 2017 • edited Loading

joyeecheung commented Dec 16, 2017 •

edited

Loading

joyeecheung commented Dec 16, 2017 •

edited

Loading

kysnm commented Dec 17, 2017 •

edited

Loading

kysnm commented Dec 17, 2017 •

edited

Loading

Xavier-J-Ortiz commented Dec 17, 2017 •

edited

Loading

Xavier-J-Ortiz commented Dec 17, 2017 •

edited

Loading

Xavier-J-Ortiz commented Dec 18, 2017 •

edited

Loading

joyeecheung commented Dec 19, 2017 •

edited

Loading

Xavier-J-Ortiz commented Dec 19, 2017 •

edited

Loading

joyeecheung commented Dec 27, 2017 •

edited

Loading