Should ws.subscribe() be marked async and await sendMessage()?

[hjoelr]

@bennycode While I have been working on writing tests for my auto-resubscribing functionality, I was noticing some interesting behavior where calling ws.disconnect() from inside the WebSocketEvent.ON_OPEN event after a function call to subscribe would result in a weird issue where line 501 would have an undefined reference to this.socket:

coinbase-pro-node/src/client/WebSocketClient.ts

Lines 483 to 502 in a6ca202

	async sendMessage(message: WebSocketRequest): Promise<void> {
	if (!this.socket) {
	throw new Error(`Failed to send message of type "${message.type}": You need to connect to the WebSocket first.`);
	}
	/**
	* Authentication will result in a couple of benefits:
	* 1. Messages where you're one of the parties are expanded and have more useful fields
	* 2. You will receive private messages, such as lifecycle information about stop orders you placed
	* @see https://docs.pro.coinbase.com/#subscribe
	*/
	const signature = await this.signRequest({
	httpMethod: 'GET',
	payload: '',
	requestPath: `${UserAPI.URL.USERS}/self/verify`,
	});
	Object.assign(message, signature);
	this.socket.send(JSON.stringify(message));
	}

I realized that because sendMessage() was marked async and awaiting a call to sign the request, calling subscribe() without awaiting would result in the sendMessage() function completing after my WebSocketEvent.ON_OPEN event finishes.

All this to say, I wonder if this function should be marked async and awaits the sendMessage() function? This would allow the caller to decide if they want to await subscribe():

coinbase-pro-node/src/client/WebSocketClient.ts

Lines 504 to 509 in a6ca202

	subscribe(channel: WebSocketChannel | WebSocketChannel[]): void {
	this.sendMessage({
	channels: Array.isArray(channel) ? channel : [channel],
	type: WebSocketRequestType.SUBSCRIBE,
	}).finally(() => {});
	}

I'd be happy to submit a PR if you think I'm right.

Here's a test that would fail on line 501 as I referenced above.

it(`fails with "Unhandled promise rejection: TypeError: Cannot read properties of undefined (reading 'send')"`, done => {
  const ws = createWebSocketClient();

  const channel: WebSocketChannel = {
    name: WebSocketChannelName.TICKER,
    product_ids: ['BTC-USD', 'ETH-USD'],
  };

  ws.on(WebSocketEvent.ON_OPEN, () => {
    ws.subscribe(channel);

    // expect(...);

    ws.disconnect();
  });

  ws.on(WebSocketEvent.ON_CLOSE, () => {
    done();
  });

  ws.connect();
});

After the proposed change, you could update the test with async/await on the ws.subscribe(channel) line and the test would pass (assuming all expects past):

it('succeeds because disconnect() runs after the subscribe finishes', done => {
  const ws = createWebSocketClient();

  const channel: WebSocketChannel = {
    name: WebSocketChannelName.TICKER,
    product_ids: ['BTC-USD', 'ETH-USD'],
  };

  ws.on(WebSocketEvent.ON_OPEN, async () => {
    await ws.subscribe(channel);

    // expect(...);

    ws.disconnect();
  });

  ws.on(WebSocketEvent.ON_CLOSE, () => {
    done();
  });

  ws.connect();
});

[bennycode]

Hi @hjoelr and thank you very much for your insights. Your proposal makes sense and I vote for marking the function as async. Please submit a PR and I will merge it. 😀

[hjoelr]

@bennycode Okay, I'll work on it as I can. In theory this is a simple change, but I envision that the tests may need to be reworked some to include the await keyword in order to satisfy the linter. Not sure what that will reveal.