Hawkbat / VTubeStudioJS

An implementation of the VTube Studio API for Node and browser JS
MIT License
45 stars 2 forks source link

I made the handling for the websocket close and error #3

Closed eslym closed 3 years ago

eslym commented 3 years ago

Let the on-going API call knows when the websocket is closed or error. better for error handling

Hawkbat commented 3 years ago

Interesting approach! Definitely better error handling wise. A couple thoughts:

This technically breaks the auto-retrying websocket wrapper I use due to the fact it completely shuts down the bus when an error occurs, but that's really more a bug in the wrapper than with this library's handling, so I'll fix that on my end after this gets merged. It does prevent reusing the bus object if the connection errors out, but I suppose that was already the case.

I'm a little bit wary of adding all this closing logic to the base class since it's not relevant to e.g. the EchoBus implementation of the message bus, and alternative transport mechanisms like an HTTP proxy over websockets might not have a useful way of implementing error/close logic. Do you think we could move it to WebSocketBus instead, something like:

 export class WebSocketBus extends MessageBusBase {
     protected _isClosed: boolean = false;

    ...

    override receive<T extends IApiMessage<any, any>>(msg: T): void {
        if(this._isClosed) return;
        return super.receive(msg);
    }

    ...
}

A bit pedantic maybe, but I've been bitten by adding too much behavior to base classes in the past and making it hard to change in a backwards compatible way later.

For the new thrownBy property on the error message, rather than needing multiple lines to set that like on line 181 of your PR, it would be cleaner to make it a new optional parameter directly on the error constructor, and it would also make sense to include it in the error message for clients that are just logging error messages instead of having access to that child property. So line 181 would look more like: new VTubeStudioError({ errorID: ErrorCode.MessageBusError, message:Message bus error: ${e}}, requestID, e)

Everything else is mostly code style and linting; I see a lot of missing whitespace between operators, semi-colons which don't match the existing code style, etc. I can clean this up post-merge if you're fine with that.

eslym commented 3 years ago

I just need a way to gracefully shutdown the plugin, maybe the auto retrying connection part can change to configurable? there is no point to retry connection if users are not using beta version of vtubestudio or vtubestudio is not even running

eslym commented 3 years ago

I just need a way to gracefully shutdown the plugin, maybe the auto retrying connection part can change to configurable? there is no point to retry connection if users are not using beta version of vtubestudio or vtubestudio is not even running

@Hawkbat any idea regarding this? i need to handle the unexpected close of vts or the plugin start before vts, and really need a way to gracefully shutdown the plugin

Hawkbat commented 3 years ago

PR is merged and published as version vtubestudio@1.4.0. I did clean up some code style things in a commit afterwards but the functionality matches your PR.