typesense / typesense-go

Go client for Typesense: https://github.com/typesense/typesense
Apache License 2.0
208 stars 55 forks source link

Upgrade oapi-codegen dependency #160

Closed maticmeznar closed 3 months ago

maticmeznar commented 4 months ago

Description

Because of the way old versions of oapi-codegen were structured, they pull in a lot of transitive dependencies like Iris and Gin web frameworks, needlessly increasing the dependency tree for any project importing oapi-codegen generated code, including this Typesense client. Read more about it at https://www.jvt.me/posts/2023/10/23/oapi-codegen-v2-decrease/

Would it be possible to upgrade the dependency?

kishorenc commented 3 months ago

~I've published a new version of the client with latest openapi changes.~

kishorenc commented 3 months ago

Sorry, the above comment was for another issue :)

As for this issue, yes, I will look into what it will take to upgrade.

maticmeznar commented 3 months ago

Thank you, much appreciated!

cysp commented 3 months ago

I took a look at upgrading oapi-codegen and it went fairly smoothly, after figuring out how to get code generation tools running. The diff of updating oapi-codegen itself looks like this: https://github.com/typesense/typesense-go/commit/e4cd9e3c19a1ac7ff03ee4fcbb7b8a953fafd67d My current use case is fairly simple so the fact that the change didn't cause any breakage there is not particularly surprising, but the update does look great from a dependency reduction perspective: https://github.com/cysp/terraform-provider-typesense/compare/main...cysp/oapi-codegen-v2

I've spun out a couple of preparatory changes to allow running code generation tools:

If you like the overall approach and diff, I'll raise a pr with the actual upgrade once those are applied.

kishorenc commented 3 months ago

Thank you @cysp -- I will be reviewing the PRs over the next few days. I can merge them to a temporary branch and then merge back into master once all PRs are done.

kishorenc commented 3 months ago

I've reviewed and merged both the PRs into oapi-codegen-upgrade branch. I have verified that the tests are also passing (Github test integration is broken at the moment).

cysp commented 3 months ago

Thanks 🙂 I've rebased and raised https://github.com/typesense/typesense-go/pull/165 targeting that branch with the oapi-codegen v2 upgrade itself.

kishorenc commented 3 months ago

@cysp

Thanks, I have merged that PR, but it seems like we also need to either move the code into v2 directory or update gomod to tag it: https://github.com/typesense/typesense-go/issues/162#issuecomment-2164987726

If you can help with with this one as well as part of this change, I will test that branch fully once before merging it back to main branch and doing a release. Thanks.

cysp commented 3 months ago

~Looking at the changes in v2, are they actually a semver-major change? The phrasing in the upgrade notes sounds like the added generic types are an optional feature rather than a source-breaking one. It seems like the simplest solution would be to release that as an update in the v1 release line rather than bumping the major version, given all of this rigmarole with changing import paths. Would that be an option?~

Moved to a more appropriate thread: https://github.com/typesense/typesense-go/issues/162#issuecomment-2165339107

kishorenc commented 3 months ago

@cysp

Can you also please update the README with how one can run the generator after the move to go run?

cysp commented 3 months ago

👍 there was already a note about using go generate, so I've updated that to execute generation across the entire package and removed the note to configure $GOBIN as that is no longer required: https://github.com/typesense/typesense-go/pull/166

kishorenc commented 3 months ago

Thank you so much @cysp -- published v1.1.0 with all your changes.

cysp commented 3 months ago

Nice one, cheers. I just applied the v1.1.0 update in my project that uses this library and it all went swimmingly 🙂