Explain error messages better (be developer friendly)

[binarykitchen]

Okay folks, this is for #394

Changes are:

• Ready states now can be obtained as strings as well with the new getReadyState(true) function
• All errors like Error: not opened are explained better now. This thanks to the common helper function generateBadStatusError(this)
• All relevant unit tests are updated.
• Mocha + Should packages are updated to latest version.

All tests are passing.

[gblazex]

+1

[lpinca]

Hmm I think the error message (not opened) is pretty clear.
If we want to add the invalid ready state to the error message we can use the numeric value as the ready state constants are documented.
Anyway the developer in most cases will just check if the ready state is OPEN

if (ws.readyState === WebSocket.OPEN) {
  ws.send('foo');
}

so I'm inclined to close this.

[lpinca]

I'm going to close this. If you disagree with my decision please comment so we can re-evaluate.

[shellscape]

@lpinca I would like to throw my hat in the ring with a "disagree." This message absolutely needs to be more verbose. It's a usability and developer-friendliness issue more than anything. While I completely understand that it might make total sense to you, as the author/owner, seasoned developers are seeing this message and wondering why they're receiving it. Remember, these folks have no intimate knowledge of the codebase. It's great that we know something isn't open, but the message fails to convey key points:

1. What isn't open? (the socket, the server connection?)
2. Why isn't "it" being considered open?
3. What data indicates a non-open state?

All three points would help developers find the reasons behind the error. At present we have to scratch our heads and wonder where we should start looking. Be friendly in your output, your users will thank you in droves.

[lpinca]

@shellscape this a WebSocket library so I think a basic/minimal knowledge of WebSocket is required. To answer your questions:

1. The error is emitted on the WebSocket object so it seems natural to me that the not open thing is the WebSocket object itself.
2. Because the WebSocket could be in CONNECTING, CLOSING or CLOSED state.
3. See 2.

[lpinca]

Actually the error is passed to the callback or thrown if there is no callback, not emitted, sorry. Question 1 makes sense.

[shellscape]

@lpinca I don't believe you're viewing this from a user's perspective. you're pretty clearly viewing it from a maintainers perspective with an intimate knowledge of the library, which is natural. but you're still glossing over the fact that the error message as it stands is not user or developer friendly.

I honestly don't understand the pushback on making an error message more verbose. It only serves to assist the library's users/devs. It wouldn't add any technical debt, it wouldn't add bloat, and it would only be more helpful. So why the pushback?

[lpinca]

I'm fine with changing the message and making it more verbose. I just don't see the point.
For reference this is the error thrown when you try to write to a closed net.Socket:

$ node
> var s = new net.Socket()
undefined
> s.write('foo')
Error: This socket is closed
    at Socket._writeGeneric (net.js:719:18)
    at Socket._write (net.js:779:8)
    at doWrite (_stream_writable.js:387:12)
    at writeOrBuffer (_stream_writable.js:373:5)
    at Socket.Writable.write (_stream_writable.js:290:11)
    at Socket.write (net.js:697:40)
    at repl:1:3
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at REPLServer.defaultEval (repl.js:239:29)
    at bound (domain.js:301:14)

Feel free to open a PR to improve the error message. It is a semver-major change so it will not be included in any 3.x release.

[shellscape]

@lpinca indeed, notice that the message does specify what is not open. the authors of that core module saw it necessary to create a friendly error message.

I must disagree though; in no way is updating a string a major semver change.

Verbatim from semver.org:

MAJOR version when you make incompatible API changes

I understand that you're a contributor to the node core, a member of the org, and have been for some time. So it boggles the mind as to why you'd claim that an update to an error message would be a major version change. On the surface it really looks like you're being intentionally difficult in a passive attempt to dissuade anyone from making a positive change in this case.

[lpinca]

It breaks tests that check the error message or functions that handle the error based on the error message. Node.js core treats these changes as semver-major.

[shellscape]

Perhaps the node core team should lobby semver to update their descriptions of version indicators then, as nothing close to that specific case appears on semver.org, and isn't in any drafts for future versions as far as I've been able to find. It seems an arbitrary choice at best.

[lpinca]

Here is an example: nodejs/node#3100 (comment)
I actually think it makes sense. You are breaking other people code by doing that.

On the surface it really looks like you're being intentionally difficult in a passive attempt to dissuade anyone from making a positive change in this case.

Open a PR, I'm more than happy to merge it and release it in 3.2.1 but I'll redirect any complaint to you 😄. Chances are there are none.

[lpinca]

FWIW a similar change (https://github.com/websockets/ws/pull/1036) has been done in a 2.x release with no one ending up complaining.

[shellscape]

@lpinca I've spoken to quite a few people about the move to error codes and the issues they've run into with error messages. I have some more reading to do on their implementation but I believe now that this should be a more comprehensive change following their lead.

[lpinca]

@shellscape are you suggesting to use custom errors with codes like Node.js?

[shellscape]

potentially. there is some more discussion and reading needed before making a call on that. see https://twitter.com/JoyeeCheung/status/920118626903928832 for some good bits, replies from maintainers about the error codes.

[lpinca]

Yes, I know that, I've reviewed some of those "migration" PRs. See https://github.com/nodejs/node/blob/master/lib/internal/errors.js for the actual implementation.
I'm not sure if it makes sense outside of Node.js though. A major version bump is usually cheap for userland modules.

[shellscape]

@lpinca indeed. it warrants more discussion