deepgram / deepgram-go-sdk

Go SDK for Deepgram's automated speech recognition APIs.
https://developers.deepgram.com
MIT License
31 stars 27 forks source link

Stop introducing breaking changes into the API #249

Closed adayNU closed 2 months ago

adayNU commented 2 months ago

See here

Modules formalized an important principle in Go, the import compatibility rule:

If an old package and a new package have the same import path, the new package must be backwards compatible with the old package.

There have been multiple minor version releases that have introduced breaking API changes. Can you please stop doing that? Or at the absolute bare minimum please mention them in bold and all caps in the release notes?

dvonthenen commented 2 months ago

Hi @adayNU

Yea, unfortunately, we had to make a breaking change from our v1.3.1 to v1.3.4. The interface on v1.3.4 should be stable now, but the functionality wasn't corrected until the latest v1.3.6.

The breaking change should have been around the ConnectWithCancel functions. We did not make this change lightly, but it was necessary to order to address a significant issue around Connect. We discovered that the code to retry the connection was not working properly and would, in more cases than not, cause the client to reconnect (and fail every time) to the websocket on a tight loop in the order of 1000+ times. Since the impact was just to connect, the decision was made to break the client to alleviate this unwanted behavior. Those changes were made in the past couple of weeks quite rapidly to address the issue as quickly as possible.

I will go back and put a heavy note on these changes on the release notes:

adayNU commented 2 months ago

Much appreciated! Thank you!

dvonthenen commented 2 months ago

Much appreciated! Thank you!

For transparency, the root cause of the issues was that we inadvertently tried to reuse the context ctx object for reconnection. This turned out to be the big "no no". So the breaking interface change to alleviate this problem, was to allow the end user to specify and provide their own cancel context/function.

adayNU commented 2 months ago

Makes sense. I also observed a few other minor breaking API changes like:

These were the ones that seemed very avoidable, for example:

// Client ...
type Client struct {
    // Definition.
}

// PrerecordedClient ...
// Deprecated: Use Client instead.
type PrerecordedClient Client
dvonthenen commented 2 months ago

Makes sense. I also observed a few other minor breaking API changes like:

  • Renaming prerecorded.PrerecordedClient => prerecorded.Client
  • Methods that accepted a interfaces.PreRecordedTranscriptionOptions were switched to accept a pointer to that type

These were the ones that seemed very avoidable, for example:

// Client ...
type Client struct {
    // Definition.
}

// PrerecordedClient ...
// Deprecated: Use Client instead.
type PrerecordedClient Client

Yea, that is also correct. I forgot about those.

We introduced a static checker (golangci-lint), which complained about passing these larger structs, without using a pointer. Thus duplicating the object for the call. Since the call is just on the init, we bit the bullet on this one as well to be in compliance with the static check. I believe it was all the Options type structs. Those structs will only grow as we add more features, capabilities, etc, and it was only a matter of time before hitting that again with the other Options.