openapi-library / OpenAPIValidators

Use Jest or Chai to assert that HTTP responses satisfy an OpenAPI spec
MIT License
189 stars 35 forks source link

`openapi-validator` - missing types for `request` and `superagent` #258

Closed DetachHead closed 2 years ago

DetachHead commented 2 years ago
node_modules/openapi-validator/dist/classes/RequestPromiseResponse.d.ts:1:40 - error TS2307: Cannot find module 'request' or its corresponding type declarations.

1 import type { Request, Response } from 'request';
                                         ~~~~~~~~~

node_modules/openapi-validator/dist/classes/SuperAgentResponse.d.ts:1:50 - error TS2307: Cannot find module 'superagent' or its corresponding type declarations.

1 import type { Response, SuperAgentRequest } from 'superagent';
                                                   ~~~~~~~~~~~~

@types/request and @types/superagent should probably be listed as dependencies instead of devDependencies as they are required downstream if using you're using typescript and don't want to enable skipLibCheck

rwalle61 commented 2 years ago

Thanks @DetachHead for this and the PR 🎁 Sorry for the delay, I've been very busy but will try to look at this soon.

(I've not run in to this issue before, so would like to recreate and understand it a bit better before I approve. If you can link other examples / articles mentioning this that would speed up my check 🙂)

DetachHead commented 2 years ago

this stackoverflow answer pretty much sums it up, but basically any typescript project that imports openapi-validator will fail to compile due to these errors unless you either:

rwalle61 commented 2 years ago

Thanks for the explanation and SO link! I've managed to recreate this. I agree request and superagent should be prod deps. I found that axios is missing, so we should add that too. I'll:

  1. merge your PR
  2. raise and merge another one for axios https://github.com/openapi-library/OpenAPIValidators/pull/263
  3. publish this to npm in a new patch version, check it compiles without errors

If you have any other thoughts, please let me know 🙂

Investigation

(Are you importing openapi-validator directly or either jest-openapi or chai-openapi-response-validator?)

If I import only openapi-validator into a project, I get your 2 errors (request and superagent), and an axios error:

node_modules/openapi-validator/dist/classes/AxiosResponse.d.ts:1:57 - error TS2307: Cannot find module 'axios' or its corresponding type declarations.

1 import type { AxiosResponse as AxiosResponseType } from 'axios';
                                                          ~~~~~~~

I'm not sure why you don't get the axios error. openapi-validator uses it but doesn't list it in its package.json. The tests pass because this is a monorepo, and the jest and chai plugins list axios in their package.jsons. This is pretty fragile, so however we fix request and superagent, we should do the same for axios.

If I import chai-openapi-response-validator instead of openapi-validator, then I get the 3 errors above, and 2 chai errors:

node_modules/chai-openapi-response-validator/dist/index.d.ts:1:23 - error TS2688: Cannot find type definition file for 'chai'.

1 /// <reference types="chai" />
                        ~~~~

node_modules/chai-openapi-response-validator/dist/index.d.ts:19:78 - error TS2694: Namespace 'global.Chai' has no exported member 'ChaiPlugin'.

19 export default function (filepathOrObject: string | OpenAPISpecObject): Chai.ChaiPlugin;

Similar to above, chai-openapi-response-validator lists chai as a devDep, not a prod dep. However, users will normally have chai installed, so won't hit this error. We could list chai as a peer deps, but it doesn't matter because no one can use chai-openapi-response-validator without chai. I'll leave this as is.

So back to request, superagent, and axios. Should they be prod deps or peer deps? Making them prod deps should work. In theory there could be a type conflict if someone uses two versions of a package together. But we don't - if a user does expect(axiosResponse).toSatisfyApiSpec, that axiosResponse is treated as any, so it can be cast to our internal axios type without conflict.

Setting them as peer deps wouldn't solve the problem. Users would still get the above errors until they install the 3 packages manually (as you're doing) or skipLibCheck. npm / yarn will display a peer dep warning, but it's easy to miss.

So yes, let's make request, superagent, and axios prod deps of openapi-validator.

Notes

If I remove @types/jest and jest, jest-openapi doesn't have compile errors . Not sure why.

openapi-validator uses openapi-types but lists it as a dev dep not a prod dep. We don't get a compilation error because the chai and jest plugins have a prod dep openapi-response-validator which includes openapi-types. That's fragile. However, if we list openapi-types as a prod dep, we could get type conflicts, because the argument to chaiResponseValidator(filepathOrObject) expects a string or an object satisfying the OpenAPI type from our version of openapi-types. So if a user provides an object satisfying the type of a different version of openapi-types, there could be a compilation error. That might be good actually. But I don't have much experience with this so I'll leave it until someone asks for it

rwalle61 commented 2 years ago

Not released yet!

rwalle61 commented 2 years ago

Published in https://github.com/openapi-library/OpenAPIValidators/releases/tag/v0.14.2, thanks for your help and patience!

Please close this when you've confirmed that 0.14.2 solves your original problem 🙂

DetachHead commented 2 years ago

(Are you importing openapi-validator directly or either jest-openapi or chai-openapi-response-validator?)

yeah i'm just importing jest-openapi. looks like the reason i didn't get the axios error was because my project already had axios as a dependency.

Please close this when you've confirmed that 0.14.2 solves your original problem 🙂

just tested it on my project and the errors are gone. thanks! :)