RicoSuter / NSwag

The Swagger/OpenAPI toolchain for .NET, ASP.NET Core and TypeScript.
http://NSwag.org
MIT License
6.7k stars 1.24k forks source link

Tons of errors after upgrading to the latest version of Nswag Studio #3568

Closed franconeilglovasa closed 3 years ago

franconeilglovasa commented 3 years ago

Hi!

I've got tons of errors after upgrading Nswag Studio. How can I fix it? How can I downgrade or if anyone has the link of the previous version of NSwag Studio?

this is one of the functions generated by NSWAG:

return _observableOf<AddOrEditUserResponseVM>(<any[]>null);

before it was:

return _observableOf<boolean>(<any>null);

Im trying to generate Typescript client code from c#

Error:

{
    "resource": "/c:/Users/admin/source/repos/TaskApp/ClientApp/src/app/swagger-generated.ts",
    "owner": "typescript",
    "code": "2769",
    "severity": 8,
    "message": "No overload matches this call.\n  Overload 1 of 20, '(...args: (SchedulerLike | AddOrEditUserResponseVM)[]): Observable<AddOrEditUserResponseVM>', gave the following error.\n    Argument of type 'any[]' is not assignable to parameter of type 'SchedulerLike | AddOrEditUserResponseVM'.\n      Type 'any[]' is missing the following properties from type 'AddOrEditUserResponseVM': init, toJSON, clone\n  Overload 2 of 20, '(a: AddOrEditUserResponseVM): Observable<AddOrEditUserResponseVM>', gave the following error.\n    Argument of type 'any[]' is not assignable to parameter of type 'AddOrEditUserResponseVM'.\n  Overload 3 of 20, '(...args: AddOrEditUserResponseVM[]): Observable<AddOrEditUserResponseVM>', gave the following error.\n    Argument of type 'any[]' is not assignable to parameter of type 'AddOrEditUserResponseVM'.",
    "source": "ts",
    "startLineNumber": 142,
    "startColumn": 55,
    "endLineNumber": 142,
    "endColumn": 66
}

image

jeremyVignelles commented 3 years ago

Please report your issue properly :

jeremyVignelles commented 3 years ago

For your question about downgrading, you can find all the versions in the github releases of this repo.

franconeilglovasa commented 3 years ago

All fixed after downgrading to Nswag Build 1134.

OzBob commented 3 years ago

Thank you @jeremyVignelles for your support and time. Please confirm that you do not also see a vast increase in errors when running your test suite to compare your snapshots comparing the outputs of previous versions of, for example, generated typescript service client.

I perssonally noticed that I now have lots of :

 Argument of type 'any[]' is not assignable to parameter of type 'AjaxSuccessResponse'.
         return _observableOf<AjaxSuccessResponse>(<any[]>null);

which has been changed from the previously OK code of:

         return _observableOf<AjaxSuccessResponse>(<any>null);

I too am downgrading back to the pervious version of nSwagStudio. Please remove the latest version from your distribution point, so that the NswagStudio users around the world avoid the current errors.

jeremyVignelles commented 3 years ago

Typescript with which template? I know there has been some work done around rxJS, and there is a regression that is being fixed by an open PR. Is that the same issue as yours?

RicoSuter commented 3 years ago

See https://github.com/RicoSuter/NSwag/issues/3566

RicoSuter commented 3 years ago

Fixed in v13.12.1

didii commented 3 years ago

I don't think this was properly fixed, since the errors I get with version 13.12.1.0 are very similar. Upgrading and simply generating the Angular TypeScript files now changes return _observableOf<T>(<any>null); to return _observableOf(null); for a method that has a return type of Observable<T>. This generates the following error:

error TS2322: Type 'Observable<null>' is not assignable to type 'Observable<T>'.

Note that this error should only occur when strictNullChecks is set to true for the TypeScript compiler. This sets null and undefined as separate types instead of adding it implicitly to every type.

For it to work with that strict flag enabled, the return type should be Observable<T | null>.


Type errors aside, I also don't think the behavior is entirely correct. If you take the process method (see below) the return _observableOf(null) statement is actually undefined behavior and should not return a null value, but rather throw an error. When any status code is encoutered that was not defined, it should go through return throwException(...) I feel. There might be some extra code to handle the 204 response (which was not defined, so I don't think it belongs here since we are also now adding | null to the contract which isn't necessarily the case), but the last else if statement should just be an else statement so the last return statement can be removed.

Original code

protected processCall(response: HttpResponseBase): Observable<string[]> {
    const status = response.status;
    const responseBlob =
        response instanceof HttpResponse ? response.body :
        (<any>response).error instanceof Blob ? (<any>response).error : undefined;

    let _headers: any = {}; if (response.headers) { for (let key of response.headers.keys()) { _headers[key] = response.headers.get(key); }}
    if (status === 200) {
        return blobToText(responseBlob).pipe(_observableMergeMap(_responseText => {
        let result200: any = null;
        result200 = _responseText === "" ? null : <string[]>JSON.parse(_responseText, this.jsonParseReviver);
        return _observableOf(result200);
        }));
    } else if (status !== 200 && status !== 204) {
        return blobToText(responseBlob).pipe(_observableMergeMap(_responseText => {
        return throwException("An unexpected server error occurred.", status, _responseText, _headers);
        }));
    }
    return _observableOf(null);
}

Proposed code

protected processCall(response: HttpResponseBase): Observable<string[]> {
    const status = response.status;
    const responseBlob =
        response instanceof HttpResponse ? response.body :
        (<any>response).error instanceof Blob ? (<any>response).error : undefined;

    let _headers: any = {}; if (response.headers) { for (let key of response.headers.keys()) { _headers[key] = response.headers.get(key); }}
    if (status === 200) {
        return blobToText(responseBlob).pipe(_observableMergeMap(_responseText => {
            let result200: any = null;
            result200 = _responseText === "" ? null : <string[]>JSON.parse(_responseText, this.jsonParseReviver);
            return _observableOf(result200);
        }));
    } else { // Removed special 204 handling since it was never defined in the first place
        return blobToText(responseBlob).pipe(_observableMergeMap(_responseText => {
            return throwException("An unexpected server error occurred.", status, _responseText, _headers);
        }));
    }
    // We can now safely remove the return _observableOf(null) since the method can never arrive here
}
leonandroid commented 3 years ago

@RicoSuter @jeremyVignelles it seems this issue is still present, any ETA on when is going live?

I have tested this with release 1136, I get the same issue, but when downloading the 1134, it works fine as expected.

as mentioned by @didii in previous post, the code generated now is return _observableOf(null); ...which is not a problem in Angular9 but it does in angular11 and 12, where the option strict is enabled.

RicoSuter commented 3 years ago

Reverted to the previous code: See https://github.com/RicoSuter/NSwag/issues/3575#issuecomment-892916102 and the linked commit... v13.13.0 available soon - will this fix it for now? I'm open for PRs which improves that in a way which works for everyone...

OzBob commented 3 years ago

Maybe adding a UI configuration option about the Ng target version or whether Strict is implemented in the target Angular project, be an idea?

didii commented 3 years ago

I'm not entirely sure what the original issue was. But declaring the correct return type should suffice. If the method can return null, it should declare it in its signature: operation(...): Observable<ReturnType | null> { ... }. Then you can keep the simple return: _observableOf(null). This should work for both strict and non-strict users since adding a null type gets ingored in non-strict mode, so no change there and explicitely adds null to the return type in strict mode so that the signature is correct.

Note that this is a breaking change for strict users, but I don't think there is a way around it.

I'm looking at creating a PR for this.

...which is not a problem in Angular9 but it does in angular11 and 12, where the option strict is enabled.

Note that strict is a TypeScript feature and it can be enabled in any version of Angular. Even before Angular officially supported it (and it does work).

RicoSuter commented 3 years ago

Please, anyone with bigger project who had problems, please review, discuss, etc. in the PR: https://github.com/RicoSuter/NSwag/pull/3586 you can also easily test the changes by overwriting the templates locally and try this proposed change.

GeorgDangl commented 3 years ago

I'm not sure if the project qualifies for a "bigger project", it's got around 15k lines in the generated client after formatting with Prettier.

I've just tested it, and everything seems to work for me.

OzBob commented 3 years ago

I've re-generated and mine is looking good