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.

[Atrue]

code can not be 0 neither null because of the row 169

[steveluscher]

Actually, because of line 168, code can be undefined. This line serves to default it to CLOSE_CODES.CLOSE_ABNORMAL if it is.

[Atrue]

Right, but undefined is a possible argument, it should be the same as 1000 code (https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/close#code)

But your change makes the opposite. It checks the null that is not possible by design and throws an error on the row 169

[steveluscher]

I think this might be a misunderstanding. In JavaScript, foo == null (notice the double-equal-sign) is an idiomatic way to test for foo being null or undefined. It is what foo ?? bar transpiles to (example).

[Atrue]

Oh, I see. I think the == operator should be forbidden by ESLint. Anyway, according to row 169, the code can either be undefined or a valid number, so this check is redundant.

At least, it can be replaced with an undefined check.

[Atrue]

the expecting behaviour is calling close with a code that is not a number, < 1000, or > 4999 throws an error, so:

• socket.close() - ok, code === 1000
• socket.close(0) - fail
• socket.close(null) - fail

[steveluscher]

This library is not an implementation of a websocket server, but rather a mock of one. My expectation of a mock is that I can configure it to behave in a certain way, pass it to another system, and have it embody that behaviour using an interface that conforms to the interface the system expects.

Said another way, if I configure the mock to close the socket with error code 0, I expect it to fire the close event with error code 0. That is the aim of this PR.

[Atrue]

It’s what I’m really trying to understand. Is it a possible case closing the connection on the server with the 0 code? As currently I see it’s not supported by design (I sent you an rfc link).

Another words, this library can be used to mock a real server with the real behaviour. If the case is not possible at all, so this library should say about this.

So, if you have such case, please send a link to part of the code of the real server. I would really like to sort this out.

[steveluscher]

I think we'll have to agree to disagree about the role of this mock. I contend that the mock should behave how I tell it to, and you believe that the mock should be opinionated and reject certain configurations.

We don't need to agree, we only need to resolve the following:

If not to accept this PR, what would you write here in order to make the mock throw an error having wasClean equal to false?

ws.close({ /* what would you write here? */ });

[Atrue]

Here're the difference.

test("closes normally", (done) => {
  const server = new Server("ws://test");
  const ws = new WebSocket("ws://test");

  server.on("connection", (client) => {
    client.close(); // the same as client.close({ code: 1000 });
  });

  ws.onclose = (e) => {
    expect(e.code).toBe(1000);
    expect(e.wasClean).toBe(true);

    done();
  };
});

test("closes wasClean=false", (done) => {
  const server = new Server("ws://test");
  const ws = new WebSocket("ws://test");

  server.on("connection", (client) => {
    client.close({ code: 1001 });
  });

  ws.onclose = (e) => {
    expect(e.code).toBe(1001);
    expect(e.wasClean).toBe(false);

    done();
  };
});

According to rfc, e.code can not be less than 1000. But how is your issue related to wasClean?

[steveluscher]

Thanks for your time.