WIP: Implement class Frame

[MarhiievHE]

#473

• tests and linter show no problems (npm t)
• tests are added/updated for bug fixes and new features
• code is properly formatted (npm run fmt)
• description of changes is added in CHANGELOG.md
• update .d.ts typings

[AliusDieMorietur]

Suggested change

	this.#frame = Buffer.alloc(2);
	this.#frame[0] = 0x81; // FIN = 1, RSV = 0b000, opcode = 0b0001 (text frame)
	let meta = Buffer.alloc(2);
	meta = 0x81; // FIN = 1, RSV = 0b000, opcode = 0b0001 (text frame)

[AliusDieMorietur]

Better to name variables in a way that reflects their purpose, preserving code readability.

[AliusDieMorietur]

Suggested change

	this.#frame[1] = length;
	meta[1] = length;

[AliusDieMorietur]

Suggested change

	this.#frame[1] = LEN_16_BIT;
	this.#frame = Buffer.concat([this.#frame, len]);
	meta[1] = LEN_16_BIT;
	meta = Buffer.concat([this.#frame, len]);

[AliusDieMorietur]

Suggested change

	this.#frame[1] = LEN_64_BIT;
	this.#frame = Buffer.concat([this.#frame, len]);
	meta[1] = LEN_64_BIT;
	meta = Buffer.concat([this.#frame, len]);

[AliusDieMorietur]

Suggested change

	this.#frame = Buffer.concat([this.#frame, data]);
	this.#frame = Buffer.concat([meta, data]);

[AliusDieMorietur]

Always false. Because type of Buffer is object.
https://nodejs.org/dist/latest-v20.x/docs/api/buffer.html#static-method-bufferisbufferobj

> typeof Buffer.from('foo')
'object'

[AliusDieMorietur]

Already checked isBuffer before

[AliusDieMorietur]

Suggested change

	#frame = null;
	#data = null;

[AliusDieMorietur]

Why server variable is required? The task was about refactoring, not about adding new features.

[AliusDieMorietur]

Typically in a constructor you just assign some props for instance. Make some configuration. But you don't do some complex logic of how inputs should be mapped correctly to Frame class props. This is not the responsibility of the constructor. So we can create from method that will take this responsibility. See in the example below. Also we should not handle encode or decode logic in constructor. Because when you create frame with string or buffer there is no difference. new Frame('1234') and new Frame(buffer). Code becomes implicit and less readable. Instead you can do Frame.from(data).decode() and Frame.from(data).decode(). This makes clear what happens in this code. See in the example below. So, please use interface described in example below.

class Frame {
  #buffer = null

  constructor(buffer) {
    this.#buffer = buffer
  }


  encode() {
    ...
  }

  decode() {
    ...
  }


  static from(data) {
    if (Buffer.isBuffer(data)) {
      return new Frame(data)
    } 

    if (typeof data === 'string') {
      return new Frame(Buffer.from(data))
    }

    throw new Error('Unsupported')
  }
}

// USAGE

send(text) {
  const frame = Frame.from(text).encode()
  this.socket.write(frame.data);
}

receive(data) {
  console.log('data: ', data[0], data.length);
  if (data[0] !== OPCODE_SHORT) return;
  const frame = Frame.from(data).decode()
  const text = frame.toString();
  this.send(`Echo "${text}"`);
  console.log('Message:', text);
}

[MarhiievHE]

Okay, with the factory, yeah, that's probably better. But creating a text frame of a protocol from a string and just saving the finished frame in a class field is not the same action. Then it is probably better to just accept a ready frame by constructor and create a protocol frame from input data by static method. Thanks.

[AliusDieMorietur]

Some tests failed

[MarhiievHE]

Some tests failed

These tests don't check the code I wrote.

[dkarpov-w]

Looks like new Frame() will return error.
Uncaught TypeError: Cannot read properties of undefined (reading '1')
Is it ok here?

[MarhiievHE]

Because it doesn't have to work that way, pass a reference to the buffer with WebSocket frame to the constructor.

[MarhiievHE]

Or use static method 'from()' to create new instance