Rework connection state handling

[arthurschreiber]

This is a first step of cleaning up the connection state handling.

The end goal here is to rework the state handling from just a set of random objects with different callback handlers to proper classes, each implementing all the logic required for all message processing that happens while each individual state is active.

[chdh]

I understand that it's only a first step and welcome the idea. But it's tricky that the this variables within the State classes refer to the connection object and not to the class objects, as one would assume. There is no isolation between the protocol state machine state and the connection object state. The protocol state machine should be encapsulated in a separate module.

[arthurschreiber]

I understand that it's only a first step and welcome the idea. But it's tricky that the this variables within the State classes refer to the connection object and not to the class objects, as one would assume. There is no isolation between the protocol state machine state and the connection object state. The protocol state machine should be encapsulated in a separate module.

I totally agree, I'm just afraid too make too many changes in one go, causing the changes to become unreviewable and sit around forever for the fear of breaking things. 😞

[arthurschreiber]

Also I think letting TokenStreamparser listener to events with flat structure at connection is a lot simpler design, for example https://github.com/tediousjs/tedious/pull/850/files#diff-875da84119066ec244742e95d1209f0cR1752 has been repeated throughout the code

My reasoning behind this change was that having only one listener for all token events at the connection lever was not great for a few reasons:

• Not all tokens are valid in all states, and in different states different tokens can have slightly different meaning. There was a bit of code that tried to handle some tokens (e.g. ORDER, ROW and some others) when no request was currently in progress, but that means that the connection-level handlers now suddenly needs to know which state is currently active.
• We should be verifying the order of tokens in a single message and probably fail if tokens come back in an unexpected order. Handling this at the connection level might pose difficult because the token stream parser does currently not know about when a message starts and when it ends (nor does it really have to know about this).

I'll think a bit about how we could refactor some of this to contain less duplication. 🙇

[MichaelSun90]

Hi @arthurschreiber, I think this is a valid enhancement for the driver, do we still consider to do this?