feat: only start sending messages after connection_ack is received

[apollo-cla]

@onhate: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[onhate]

@calvernaz I've added an extra check on sendMessage on top of your changes to enqueue any attempt of sending messages while it's not initialized. Let me know your thoughts.

[calvernaz]

Hi @onhate, sorry for coming back only now. I looked into your changes and I don't see any particular wrong with it. However, I'm wondering why we need the extra guard.

The state of the connection should be enough to either send the message or queue it,

  private sendMessageRaw(message: any) {
    switch (this.status) {
      case this.wsImpl.OPEN:
        // send the message
     case this.wsImpl.CONNECTING:
      // queue the message

Once the connection is OPEN the first message sent is MessageTypes.GQL_CONNECTION_INIT and you're allowing for that (that's correct) but the existing flow already determines that https://github.com/apollographql/subscriptions-transport-ws/blob/8976df8c8b35476e4e4071adf5b565f6a6e7df18/src/client.ts#L562

Now, the only risk of sending messages with the connection OPEN and out of order was fixed by the PR where we only flush the queue after receiving the ACKNOWLEDGE https://github.com/apollographql/subscriptions-transport-ws/blob/8976df8c8b35476e4e4071adf5b565f6a6e7df18/src/client.ts#L597

FWIW, I don't see any issue with your changes, I'm just trying to understand it if is required for out of order issue, or something that you trying to cover in case someone misuses the API.

[onhate]

@calvernaz the issue I found is that START messages were still sent before CONNECTION_ACK was received, that forced me to implement on the server side a wait for connection initialized system to reply the START messages only after connection data was persisted on database and I could read it.

The check you mentioned only verifies if the websocket connection is OPEN, not if the connection was initialized.

case this.wsImpl.OPEN:
        // send the message

If executeOperation is invoked before the connection is initialized it will just send the message ignoring if it's initialized or not.

https://github.com/apollographql/subscriptions-transport-ws/blob/8976df8c8b35476e4e4071adf5b565f6a6e7df18/src/client.ts#L325

[calvernaz]

Hmm..wondering why I don't have the same issue.

Couple of things is worth to mention, when I said the websocket connection is OPEN, it means from the protocol perspective I have a bind with the peer, so I have a TCP connection open and initialized.

I think what you mean by "initialized" is the readiness to start sending messages from the graphql over websockets protocol perspective. From that perspective, I can see from the snippet that "GQL_START" can be called before receiving the "GQL_CONNECTION_ACK".

I can see how your code would fix this and potentially fixes all the other potential places where an attempt could be done to send messages without waiting for the acknowledge, which IMO should be fixed if that's the case.

Thanks, for adding this 👍

https://github.com/apollographql/subscriptions-transport-ws/blob/master/PROTOCOL.md