x/exp/cmd/apidiff: reports bogus changes between identical export data

[pohly]

Go version

go version go1.22.0 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/pohly/.cache/go-build'
GOENV='/home/pohly/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/nvme/gopath/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/nvme/gopath'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/nvme/gopath/go-1.22.0'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/nvme/gopath/go-1.22.0/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/nvme/gopath/src/k8s.io/kubernetes/go.mod'
GOWORK='/nvme/gopath/src/k8s.io/kubernetes/go.work'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3365691786=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I am running apidiff -w twice, then compare the exported data. Specific steps:

• Check out https://github.com/kubernetes/kubernetes.
• git checkout 17854f0e0a153b06f9d0db096e2cd8ab2fa89c11 (or later - needs to include https://github.com/kubernetes/kubernetes/commit/0c7370bb851c15825d30a516722139ccccca0cfc).
• go install golang.org/x/exp/cmd/[email protected] (current latest)
• apidiff -m -w /tmp/after.out ./staging/src/k8s.io/client-go
• apidiff -m /tmp/after.out /tmp/after.out (same file!)

What did you see happen?

Ignoring internal package k8s.io/client-go/applyconfigurations/internal
Ignoring internal package k8s.io/client-go/tools/internal/events
Ignoring internal package k8s.io/client-go/applyconfigurations/internal
Ignoring internal package k8s.io/client-go/tools/internal/events
Incompatible changes:
- ./util/workqueue.BucketRateLimiter: changed from TypedBucketRateLimiter[any] to TypedBucketRateLimiter[T comparable]
- ./util/workqueue.DelayingQueueConfig: changed from TypedDelayingQueueConfig[any] to TypedDelayingQueueConfig[T comparable]
- ./util/workqueue.ItemExponentialFailureRateLimiter: changed from TypedItemExponentialFailureRateLimiter[any] to TypedItemExponentialFailureRateLimiter[T comparable]
- ./util/workqueue.ItemFastSlowRateLimiter: changed from TypedItemFastSlowRateLimiter[any] to TypedItemFastSlowRateLimiter[T comparable]
- ./util/workqueue.MaxOfRateLimiter: changed from TypedMaxOfRateLimiter[any] to TypedMaxOfRateLimiter[T comparable]
- ./util/workqueue.QueueConfig: changed from TypedQueueConfig[any] to TypedQueueConfig[T comparable]
- ./util/workqueue.RateLimitingQueueConfig: changed from TypedRateLimitingQueueConfig[any] to TypedRateLimitingQueueConfig[T comparable]
- ./util/workqueue.Type: changed from Typed[any] to Typed[t comparable]
- ./util/workqueue.WithMaxWaitRateLimiter: changed from TypedWithMaxWaitRateLimiter[any] to TypedWithMaxWaitRateLimiter[T comparable]

Note that e.g. type BucketRateLimiter = TypedBucketRateLimiter[any]. There is no TypedBucketRateLimiter[T comparable].

What did you expect to see?

No changes because the exported data is identical.

[HerrBertling]

I like option B. It’s not breaking (which in the current alpha phase of the package doesn’t really matter, but still…). Also, there’s probably more info contained within response than just the status code, so this would be a handy way to have the “meta” stuff separated from the actual data/error.

edit: I’ll use the library in a react-router/Remix project, so having more than just the status code available in an explicit manner would certainly be super helpful.

Just curious: Why do you not want to use generics?

[drwpow]

True—breaking changes don’t matter much in pre-1.0. But I think the API / usability is everything, and I do think sweating the details matter! I should have put more emphasis on “easy to use” more than “breaking changes.” I also would rather err on the side of “happy path is easy/terse; complex path is mildly annoying“ rather than “everything is equally mildly annoying.” 😄

Why do you not want to use generics?

The main README has a note on this, but that’s a path to errors. Imagine the lowest-common-denominator in a codebase: not yourself, but either someone that doesn’t know better, or someone who is trying to shortcut everything. Imagine having the wrong generic, or simply providing <any> everywhere. That’s effectively as good as not using TypeScript! And further, I believe generics are a last-resort when introspection fails. And within the realm of your OpenAPI schema, I think it’s more helpful to assume everything can be introspected. No errors, no shortcuts, and even better usability!

[drwpow]

So v0.1.0 does somewhat solve this in that it creates unions of “good” and “bad” types. Theoretically, now, if you have different schemas for different error codes (like different shapes entirely), then that will now be reflected when you check { error }. Likewise for { data } (though that is much less-common to have multiple different responses for the same method that have wildly-varying shapes).

I’m leaving this issue open for now, because I want to investigate this further. But I think for various reasons, we’ll be going forward with Option B mostly because that’s lighter-weight and easier to get type inference working for.

Currently the only thing missing in 0.1.0 from making Option B possible is the response.status discriminator—currently it doesn’t associate specific statuses with specific response codes yet (but is very possible to do in an upcoming release)

[drwpow]

Update on this: the response.status discriminator support still hasn’t been added, and this issue will be complete once that’s in.

[EmilePerron]

I am very interested in this, and just wanted to add my support for going forward with Option B!

In the meantime, we use something of the sort to get the right types based on the status code:

if (response.error) {
  let error;

  switch (response.response.status) {
    case 400:
      error = response.error as paths["/rtm/notes/create"]["post"]["responses"]["400"]["content"]["application/ld+json"];
      break;
    case 422:
      error = response.error as paths["/rtm/notes/create"]["post"]["responses"]["422"]["content"]["application/ld+json"];
      break;
    // etc...
  }

It's not nearly as neat, but it kind of "works". 🤷‍♂️

[RobinClowers]

I also support option B, I think checking status codes is a good practice anyway. I'm working with this api:

get_region: {
  responses: {
    200: {
      content: {
        "application/json": components["schemas"]["Region"];
      };
    }; 
    202: {
      content: {
        "application/json": components["schemas"]["Region"];
      };
    };
    204: {
      content: never;
    };
  };
};

~Currently, data isn't type safe for this API, typescript will let me access properties of Region even though in the 204 case, there is no response body.~ I misspoke here! It appears that now data is a union of all possible response shapes, which is at least type safe. However, depending on what the shapes in question are, it can be awkward to narrow the type.

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

[github-actions[bot]]

This issue was closed because it has been inactive for 7 days since being marked as stale. Please open a new issue if you believe you are encountering a related problem.