Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG][typescript-fetch] Generating incorrect types #3869

Open
keean opened this issue Sep 10, 2019 · 23 comments
Open

[BUG][typescript-fetch] Generating incorrect types #3869

keean opened this issue Sep 10, 2019 · 23 comments

Comments

@keean
Copy link

keean commented Sep 10, 2019

When using openapi-generator to generate a typescript-fetch API from this specification:

openapi-generator generate -i https://api.demo.kaizenep.com/swagger.json -g typescript-fetch -o kaizen-api

The generated code has incorrect types where "List" is referenced. For example line 182 and 219 of output apis/EventsApi.ts

async postEventSearchRaw(requestParameters: PostEventSearchRequest): Promise<runtime.ApiResponse<Array>> {
async postEventSearch(requestParameters: PostEventSearchRequest): Promise<Array> {

This should be:

async postEventSearchRaw(requestParameters: PostEventSearchRequest): Promise<runtime.ApiResponse<List>> {
async postEventSearch(requestParameters: PostEventSearchRequest): Promise<List> {

this is using openapi-generator 4.0.3

@auto-labeler
Copy link

auto-labeler bot commented Sep 10, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@macjohnny
Copy link
Member

can you explain why it should be List instead of Array?

@keean
Copy link
Author

keean commented Sep 10, 2019

First, Array is not a good typescript type (at least not in TypeScript 3.1.3). This will generate the compile time warning:

[ WARN  ]  TypeScript: src/kaizen-api/apis/EventsApi.ts:182:102
           Generic type 'Array<T>' requires 1 type argument(s).

    L181:   */
    L182:  async postEventSearchRaw(requestParameters: PostEventSearchRequest): Promise<runtime.ApiResponse<Array>> {
    L183:      if (requestParameters.payload === null || requestParameters.payload === undefined) {

It should be Array<T> and what is T? I guess the closest allowable typescript type would be Array<any>.

The easiest way to see what type it should be is to look at the code. If we look at the return statement from the function starting at line 182:

return new runtime.JSONApiResponse(response, (jsonValue) => ListFromJSON(jsonValue));

We can see that the parameter to runtime.ApiResponse must be the same as the type returned from ListFromJSON which returns List.

@macjohnny
Copy link
Member

@keean can you try with the latest version 4.1.1?

@keean
Copy link
Author

keean commented Sep 10, 2019

4.1.1 seems to have the same problem, but less serious because it at least compiles (with warnings).

However the client code has to treat the value as a "List" because it has this interface:

export interface List {
    /**
     * 
     * @type {number}
     * @memberof List
     */
    start?: number;
    /**
     * 
     * @type {Array<object>}
     * @memberof List
     */
    hits?: Array<object>;
    /**
     * 
     * @type {number}
     * @memberof List
     */
    total?: number;
    /**
     * 
     * @type {number}
     * @memberof List
     */
    size?: number;
}

It is clearly not an Array, it does not even have a length property. ListFromJSON returns a List because that is what the JSON structure passed from the server its, not an array.

I don't really understand why TypeScript allows this, it seems to be a limitation in the type checking, effectively it allows:

Promise<List> to be silently cast to Promise<Array> and then Promise<Array> to be cast back to Promise<List> in the client code.

The type really is not an Array at all. My code works now without hand editing, so I am not worried about this, but you should be worried about this because a type is completely wrong and all it would take would be one dereference to crash at runtime. Is there a deeper problem here?

@keean
Copy link
Author

keean commented Sep 10, 2019

Have just discovered this is an Error in production mode, it's a warning only for development compilation, so it is important to me to get this resolved.

[ ERROR ]  TypeScript: src/kaizen-api/src/apis/EventsApi.ts:182:102
           Generic type 'Array<T>' requires 1 type argument(s).

    L181:   */
    L182:  async postEventSearchRaw(requestParameters: PostEventSearchRequest): Promise<runtime.ApiResponse<Array>> {
    L183:      if (requestParameters.payload === null || requestParameters.payload === undefined) {

[ ERROR ]  TypeScript: src/kaizen-api/src/apis/UsersApi.ts:390:100
           Generic type 'Array<T>' requires 1 type argument(s).

    L389:   */
    L390:  async postUserSearchRaw(requestParameters: PostUserSearchRequest): Promise<runtime.ApiResponse<Array>> {
    L391:      if (requestParameters.payload === null || requestParameters.payload === undefined) {

[ ERROR ]  TypeScript: src/kaizen-api/src/runtime.ts:127:24
           Cannot find name 'GlobalFetch'.

    L127:  export type FetchAPI = GlobalFetch['fetch'];

[22:56.4]  build failed in 14.45 s

So I am back to hand editing the Array to List to get this to work.

@macjohnny
Copy link
Member

Would you like to implement a fix?
I think the type should be Array, and hence fixed in the ...FromJSON
@TiFu what is your opinion?

@keean
Copy link
Author

keean commented Sep 10, 2019

If we add a console.log, we can see the actual type there is a List not an Array:

await postEventSearch = {hits: Array(1), size: 10, start: 0, total: 1}

So we can clearly see the Promise resolves to a List and not an Array, hence the return type of postEventSearch must be Promise<List>

The ListFromJSON is returning the correct type. You can clearly see what it is meant to be looking at the swagger.json file:

"200": {
     "description": "Success",
     "schema": {
         "$ref": "#/definitions/list"
     }
},

and the reference to List is clearly an object not an array:

"list": {
    "properties": {
        "hits": {
            "items": {
                "type": "object"                
            },
            "type": "array"
        },
        "size": {
            "type": "integer"
        },
        "start": {
            "type": "integer"
        },
        "total": {
            "type": "integer"
        }
    },
   "type": "object"
},

So postEventSearch returns a single List object not an Array.

I am happy to look at implementing a fix, but I have not looked at the OpenAPI generator code before. Where is the code that derives what the return type should be? In the template it gets referred to as returnType so it is perhaps not a template problem? How is the returnType derived?

@TiFu
Copy link
Contributor

TiFu commented Sep 11, 2019

@macjohnny I agree with @keean that it should be of type List.

I think the bug is in AbstractTypeScriptClientCodegen:

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java#L110

It defines that the type list maps to a (TypeScript) array which in my opinion doesn't make a whole lot of sense to include - The OAI Spec doesn't define that list is equivalent to array and this also doesn't make a whole lot of sense in the context of TypeScript. Removing that line will likely fix this bug.
Note that removing this will be a breaking change. However, I'd hope no one relies on this behavior.

@keean would you be willing to test this and submit a PR if this fixes the bug?

@macjohnny
Copy link
Member

@tifi @keean thanks for the analysis. Now I understand, List is a custom type, and I agree, the mapping in


does not make sense. @wing328 can you confirm there is no generic type List in the parsed spec?

@keean
Copy link
Author

keean commented Sep 11, 2019

I am happy to test. As far as I am aware there is no generic List type in TypeScript, and it is not a reserved word in OpenAPI. List of TypeScript Basic types:

https://www.typescriptlang.org/docs/handbook/basic-types.html

And TypeScript advanced types:

https://www.typescriptlang.org/docs/handbook/advanced-types.html

But whatever language is used, unless OpenAPI defines it as a reserved word, I would expect the schema custom definition to be used, which means in a language where "List" is reserved, then the OpenAPI type would need to be mapped to "ApiList" or some other identifier.

I would assume that an OpenAPI sphema referencing an undefined "List" object would fail to validate?

@wing328
Copy link
Member

wing328 commented Sep 11, 2019

does not make sense. @wing328 can you confirm there is no generic type List in the parsed spec?

"List" is not an OpenAPI type from what I can tell.

@keean
Copy link
Author

keean commented Sep 11, 2019

So if I comment out that line, it generates the correct types.

However I still have to make a manual change to get it to compile without error, that I might as well mention here, I have to replace GlobalFetch with WindowOrWorkerGlobalScope in src/runtime.ts

@macjohnny
Copy link
Member

An option to generate code for typescript 3+ was introduced, which should fix that

@evstinik
Copy link

@macjohnny oh finally, you saved my life. An option for ts3+.. how long I've been searching for that just to switch to newest ts version 3.7. Thanks!

@dngconsulting
Copy link
Contributor

It it possible to fix this compilation error also ?

 /app/src/sdk/runtime.ts
webapp       | TypeScript error in /app/src/sdk/runtime.ts(297,11):
webapp       | 'value', which lacks return-type annotation, implicitly has an 'any' return type.  TS7010
webapp       |
webapp       |     295 |     constructor(public raw: Response) {}
webapp       |     296 |
webapp       |   > 297 |     async value() {
webapp       |         |           ^
webapp       |     298 |         return undefined;
webapp       |     299 |     }
webapp       |     300 | }

Thanx

Sami

@macjohnny
Copy link
Member

@dngconsulting yes, I would be happy to review your pull-request.

@dngconsulting
Copy link
Contributor

dngconsulting commented Dec 5, 2019

@macjohnny here we go => #4711 hoping that the merge will be as quick as your answer 😉

@Bakkiyaraj
Copy link

Cannot find name 'GlobalFetch'. TS2304
export type FetchAPI = GlobalFetch['fetch'];

We are using typescript-fetch to generate models. Can you please confirm which release has the fix?
or suggest the workaround for this

@janniclas
Copy link

janniclas commented Feb 10, 2020

@Bakkiyaraj A general workaround is to replace GlobalFetch['fetch'] with WindowOrWorkerGlobalScope['fetch'].
The correct code is generated if you use the --additional-properties=typescriptThreePlus=true flag during generation

@trumbitta
Copy link
Contributor

@janniclas I'm confused: shouldn't TS3+ be the default?

@DanielRuf
Copy link
Contributor

@janniclas I'm confused: shouldn't TS3+ be the default?

Related: #9973

@rvansa
Copy link

rvansa commented Jun 1, 2022

I am experiencing the typescript error due to Array without generics with typescript-fetch using version 6.0.0, too - I don't have the items in OpenApi as it is a free-form input; I'd expect Array<any>. Can I do something about this in this version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests