Node: Tracking issue: Migrate errors to internal/errors.js

[joyeecheung]

There are a few old-style errors added since https://github.com/nodejs/node/issues/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

• [x] [lib/_http_outgoing.js](https://github.com/nodejs/node/tree/master/lib/_http_outgoing.js) - https://github.com/nodejs/node/pull/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
  
  
• [x] [lib/_tls_legacy.js](https://github.com/nodejs/node/tree/master/lib/_tls_legacy.js) - https://github.com/nodejs/node/pull/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.
  
  
• [x] [lib/_tls_wrap.js](https://github.com/nodejs/node/tree/master/lib/_tls_wrap.js) - https://github.com/nodejs/node/pull/17792
  
  
  
  • new Error('TLS session renegotiation disabled for this socket'): needs a new error, maybe ERR_TLS_RENEGOTIATION_DISABLED
  
  
• [x] [lib/fs.js](https://github.com/nodejs/node/tree/master/lib/fs.js) - https://github.com/nodejs/node/pull/17719
  
  
  
  • new Error('Invalid data')): can be migrated to ERR_INVALID_ARG_TYPE
  
  
• [x] [lib/net.js](https://github.com/nodejs/node/tree/master/lib/net.js) - https://github.com/nodejs/node/pull/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
  
  
• [x] [lib/repl.js](https://github.com/nodejs/node/tree/master/lib/repl.js) - https://github.com/nodejs/node/pull/17716
  
  
  
  • new Error('Script execution interrupted.')): needs a new error, maybe ERR_SCRIPT_EXECUTION_INTERRUPTED
  
  

[joyeecheung]

Just like https://github.com/nodejs/node/issues/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]

@joyeecheung @jasnell i can take these up.

[ramsgoli]

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

[b0yfriend]

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

[joyeecheung]

@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]

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

[kysnm]

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

[mannanali413]

@jasnell I will take up lib/_tls_wrap.js

[styfle]

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

[jasnell]

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

[styfle]

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

[Xavier-J-Ortiz]

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. :)

[Xavier-J-Ortiz]

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

https://github.com/Xavier-J-Ortiz/node/commit/b251e5c6ba028211368f1a8d58e017c3dc85938c

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]

@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]

@joyeecheung Thank you for pointing that out.

1) 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.

2) 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]

@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]

@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]

@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.

[iansu]

@acparas Are you still working on lib/_http_outgoing.js? If not I'd like to give it a try.

[b0yfriend]

Yes! Been busy the last couple days, but I’m working on it right now.

[joyeecheung]

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