kogosoftwarellc / open-api

A Monorepo of various packages to power OpenAPI in node
MIT License
895 stars 237 forks source link

openapi-types: remove 'any' type from OpenApiV2.Document.paths[idx] typing (fixes #552) #720

Closed 12kb closed 3 years ago

chadxz commented 3 years ago

@12kb can you share some context behind this change for us to better understand?

12kb commented 3 years ago

Hi! Yepp, sure)

  1. I found in my code, that im not able to properly type

    const value = document.paths[path]; // returns any.

    value type is any. That happens because of OpenAPIV2.Document['paths'][number] has PathItemObject<T> | any type. any is stronger type than PathItemObject, so typescript merges them both to just any.

  2. I've spend some time to hack around this in my code, but in the end I was unable to fix it properly on my side. (casting is improper fix i think :))

  3. I found this issue, where @jsdevel said that any is not intentional here and welcomed a PR.

  4. I didn't find a PR about that, and have done this tiny change myself).

And now I'll ask a question) Actually this change is a breaking one. If someone has code which relies on document.paths is any, the code will break down. Shall i increase major version of openapi-types lib to 8.0.0, right?

chadxz commented 3 years ago

I imagine @jsdevel will want to be the one to bump the version, but it certainly important to consider.

Also, I'm not exactly sure if we need a test for this or not, but it's clear that the types of public API objects are just as much a part of the public API as method signature/behavior. Therefore theoretically I do believe this should have a test, but since the tests are passing with this change, there doesn't appear to have been a test for this already in place.

Any idea if there are any other | any types in the codebase? Might be good to squash any others at the same time if so in order to limit the major version churn.

Thoughts @jsdevel ?

12kb commented 3 years ago
  1. I've investigated the topic a bit, and found where it comes from. scrsht

BUT I still believe we should remove any here. 'Any' word in spec !== using any in typescript. any here makes type completely unusable. If keeping any, we can safely remove PathItemObject part there, anyway it doesn't work.

Spec says about extensions. So, I believe It's job of extenders to provide proper typings for their extensions. It's doable via declaration merging. Something like this:

import 'openapi-types';
declare module 'openapi-types' {
    export namespace OpenAPIV2 {
         export interface PathsObject {
              'x-myLovelyField': MyType
         }
    }
}

will do the job. (Same way as plugin vendors provide their plugins typings for e.g. express or jquery).

2.

I do believe this should have a test

Well, in general we are able to provide kind of test for this. I.e. it's possible to write a piece of code that won't compile (or will compile otherwise) if this type is changed. But i see 2 issues here:

3.

Any idea if there are any other | any

Good point. Thanks! I see one more | any on ResponsesObject. I'll investigate it.

12kb commented 3 years ago

Japp, same issue with ResponsesObject. '^x-' fields for extensions. Ineteresting point is that there are more Any-s in spec, e.g. on ResponseObject (not Responses), but luckily it's missed in openapi-types lib :). 😌 Otherwise this would destroy type system completely).

So my plan is to

  1. Remove | any from ResponsesObject.
  2. Pupm the version to 8.0.0. Agreed 🤝 ?)
12kb commented 3 years ago

@jsdevel Could you please help me with test. Where can I add it? There is currently no testing infrastructure in openapi-types.

12kb commented 3 years ago

@chadxz @jsdevel Hi guys. Sorry, it's not clear to me what else you expect me to do here. Could you please explain me what i'm missing? Thanks.)

12kb commented 3 years ago

Thanks a lot!)