When an explicit `0` is passed as the close `code` it is honoured instead of defaulted back to `1000`

[steveluscher]

After this PR, this works as expected:

ws.close({ code: 0, reason: 'o no', wasClean: false });

Before this PR that would force the code back to 1000.

[Atrue]

@steveluscher Interesting, I’m trying to figure out if the 0 code can be even used as status code on the server. According to this https://datatracker.ietf.org/doc/html/rfc6455#section-7.4.2 Status codes in the range 0-999 are not used. The lib ws for node have the same logic for server-side websockets: https://github.com/websockets/ws/blob/master/lib/validation.js#L37

Do you have any issues with the current behaviour?

[steveluscher]

Without even getting into that, would you agree that the objective of this existing code was to set code to a default value if it was not set by the caller?

const code = options.code || CLOSE_CODES.CLOSE_NORMAL;

On that basis alone, the change is correct. The existing code would not ‘set it to CLOSE_NORMAL if it's unset.’ It would only set it to CLOSE_NORMAL if it was unset and not set to zero.

[Atrue]

@steveluscher Yes, obviously there is a difference between the || and ?? operators, but from the user's perspective, there is no point in making this change, as code with the value 0 should not be allowed at all.

So the main question is: what is the purpose of creating this PR? Do you have some logic broken without this?

If you want to maintain the project, you can cover a test case instead: write a test that expects closing the server/client with code 0 to throw an error. According to the current code, there is only client validation and no server validation.

[steveluscher]

Here's a positive outcome that could result from landing this PR: Someone who writes this test, whose intent is to assert that the server closed uncleanly, will not have to debug for minutes or hours why the code ends up being 1000, wasClean ends up being true, and their test fails.

it('throws an error when the connection closes uncleanly', async () => {
  expect.assertions(1);
  const requestPromise = makeCoolWebSocketRequest(...);
  ws.close({ code: 0, reason: 'o no', wasClean: false });
  await expect(requestPromise).rejects.toThrow(new CustomWebSocketError('close', {
    code: 0,
    reason: 'o no',
    wasClean: false,
  });
});

This is how I came to make this PR.

@Atrue, can you think of a reason not to make this change?

[Atrue]

@steveluscher Alright, so you've catched the issue with '0' code. To ensure that this effort is not in vain, please include the test from the previous message and modify the server.js file to achieve the expected behaviour.

Currently I don't have much time to maintain this, but I'll merge it if you bring it to the end. Thanks.

[steveluscher]

That should do it.

[steveluscher]

Also, I opted for a more compatible syntax, rather than the nullish coalescing operator, lest someone be running tests in an environment that doesn't support it.

[steveluscher]

Thanks for your time.