[Typescript][Angular]Issue #6902 Return Client Response for Angular 4.3 and above with Observe string by kenisteward · Pull Request #6955 · swagger-api/swagger-codegen

kenisteward · 2017-11-14T05:44:15Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Fixes #6902 with some added bonuses
@bedag-moo @HermenOtter Gonna need you 2 to test this as i'm not currently running angular ^4.3 Will be upgrading to 5 very soon though.

Note: For overloads to work with proper intellisense the first overload must always be for 'body'. if any changes are made to the overload that rearranges them intellisense will break and confuse consumers

Techies!

@TiFu @taxpon @sebastianhaas @Vrolijkx

…les to the response for each api call for angular 4.3 and above

macjohnny · 2017-11-14T07:06:16Z

samples/client/petstore/typescript-angular-v2/default/api/pet.service.ts

        let requestOptions: RequestOptionsArgs = new RequestOptions({
            method: RequestMethod.Get,
-            headers: headers,
+            headers,


I suggest not to remove the explicit headers: part, since without it, refactoring variable names can easily lead to problems

I'm not exactly sure hat you mean. Please elaborate so i can understand. Thanks!

@kenisteward when creating objects like this:

{ something }

instead of

{ something: something }

the name of the property of the object changes when refactoring variable names, leading to potential problems if one does not notice it.

Ahh. Unfortunately, mostly due to how our templates are setup, that will be a big breaking change either way.

For the 4.3 client and the non 4.3 client they both use the same name for a variable however if someone changed the name for 1 client thinking it would be fine that would also cause errors.

I'm also assuming you mean refactoring via our mustache templates as one should not be refactoring their code directly as new versions should be automatically generated.

HermenOtter · 2017-11-14T12:16:23Z

@kenisteward When generating with Angular typescript 4.3, typescript returns this error:

severity: 'Error'
message: Argument of type '{ params: HttpParams; withCredentials: boolean; headers: HttpHeaders; observe: "body" | "response...' is not assignable to parameter of type '{ headers?: HttpHeaders; observe?: "body"; params?: HttpParams; reportProgress?: boolean; respons...'.
  Types of property 'observe' are incompatible.
    Type '"body" | "response" | "events"' is not assignable to type '"body"'.
      Type '"response"' is not assignable to type '"body"'.
source: 'ts'

All my httpClient actions have this problem, but as example this snippet:

return this.httpClient.get<InlineResponse200>(`${this.basePath}/subj/v1/subjecten/`,
            {
                params: queryParameters,
                withCredentials: this.configuration.withCredentials,
                headers,
                observe,
                reportProgress
            }
        );

HermenOtter · 2017-11-14T12:53:04Z

@kenisteward Could it have something to do with the order of elements in the options object? It might be the intellisense in vscode messed up.

kenisteward · 2017-11-14T14:46:25Z

@HermenOtter turns out i missed up the mustache template for when there are no previous fileds but we still need observe / report progress looking into this now.

HermenOtter · 2017-11-14T15:24:44Z

@kenisteward Did you push a fix to the PR?

…nd should not have been.

kenisteward · 2017-11-14T15:41:30Z

@HermenOtter Not yet. It turns out that was a bug but the actual bug causing your problem was because it doesn't like typing the function as 'body' | 'response' | 'events' for correlation with passing it to the httpclient. I'm going to switch to any right now and try again. I'm not getting any errors locally with any and we get type safety with the overloads

kenisteward · 2017-11-14T15:43:17Z

@macjohnny I think i''ll change to what you suggested for clarity in the future for others.

kenisteward · 2017-11-14T16:01:03Z

@HermenOtter Fixed!

HermenOtter · 2017-11-15T09:02:37Z

@kenisteward @macjohnny I can confirm it works as expected for my application. However I did not test all combinations.

Thank you for your effort.

kenisteward · 2017-11-15T11:57:20Z

@HermenOtter thanks for testing! Yay new features! If there are no other issues @wing328 please merge at you leisure.

@bedag-moo would love for you to test as well if you have time!

bedag-moo · 2017-11-15T22:25:12Z

samples/client/petstore/typescript-angular-v4.3/npm/api/pet.service.ts

+    public findPetsByStatus(status: Array<string>, observe?: 'body', reportProgress?: boolean): Observable<Array<Pet>>;
+    public findPetsByStatus(status: Array<string>, observe?: 'response', reportProgress?: boolean): Observable<HttpResponse<Array<Pet>>>;
+    public findPetsByStatus(status: Array<string>, observe?: 'events', reportProgress?: boolean): Observable<HttpEvent<Array<Pet>>>;
+    public findPetsByStatus(status: Array<string>, observe: any = 'body', reportProgress: boolean = false ): Observable<any> {


The type of observe seems too permissive here, because it would allow invocations like:

findPetsByStatus(status, observe: HttpResponse) findPetsByStatus(status, observe: "Body")

Angular's HttpClient restricts this parameter to permitted values:

observe?: HttpObserve,

where

/** * @stable */ export type HttpObserve = 'body' | 'events' | 'response';

So we should be able to import that type, and use it ourselves? Is there a reason we can't follow angular's lead here?

you can't import it. I tried it myself and it did not work. You can try it in your project if you'd like and tell me if i'm doing something wrong.

also it doens't allow those invocations. you are only allowed to pass thoes specific things cause it's the string type.

the last observe: any is not counted for the invocations. only the overloads before it.

@bedag-moo from angular public_api.d.ts
export { HttpClient } from './src/client';

they don't export HttpObserve

My apologies, I didn't know that typescript treats does not consider the last signature part of the overload list. Your code is fine then, sorry for not checking.

bedag-moo · 2017-11-15T22:29:46Z

I'm afraid I won't have time for tests in the near future, but if it works for Hermann Otter, it very likely works for me, too.

bedag-moo · 2017-11-15T22:45:47Z

This PR looks good to me.

HermenOtter · 2017-11-16T08:57:37Z

@kenisteward @bedag-moo I had more time and I came across a few warnings and a problem.
TSLint is complaining about the use of let's instead of const, as well if (httpContentTypeSelected != undefined) { != should be !== and [tslint] " should be ' (quotemark)

@kenisteward Since you changed the implementation to in the method overload, I doesn't return HttpResponse<Subject> anymore, but instead HttpResponse<any>. I am not sure if it has something to do with my version of typescript, my intellisense or a different problem.

I might be able to find some time to commit fixes to those warnings, but I don't know why the typings don't work.

kenisteward · 2017-11-16T13:04:09Z

In general , Please don't commit changes for tslint as everyone's linting can be different . In this case changing the != To !== Is programmatically different and correct but others like the quote marks are preferential. I'll fix that in my changes but don't worry about linting errors as much . Ingneral they are for code you touch (imo) not generated code. You can either exclude the project from linting or simply compile and include as a dependency from a repo

…

On Thu, Nov 16, 2017, 3:57 AM Hermen Otter ***@***.***> wrote: @kenisteward <https://github.com/kenisteward> @bedag-moo <https://github.com/bedag-moo> I had more time and I came across a few warnings and a problem. TSLint is complaining about the use of let's instead of const, as well if (httpContentTypeSelected != undefined) { != should be !== and [tslint] " should be ' (quotemark) @kenisteward <https://github.com/kenisteward> Since you changed the implementation to in the method overload, I doesn't return HttpResponse<Subject> anymore, but instead HttpResponse<any>. I am not sure if it has something to do with my version of typescript or my intellisense. I might be able to find some time to commit fixes to those warnings, but I don't know why the typings don't work. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6955 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMPLtdVd20LanjsmwWCSRhJ70yzpbFMVks5s2_kLgaJpZM4Qc1SV> .

HermenOtter · 2017-11-21T15:06:08Z

@kenisteward Thanks for your reply, did you read the second paragraph of my answer? It seems like the typings are broken, while it should not be possible, did you experience the same problem?

kenisteward · 2017-11-21T15:29:04Z

@HermenOtter I did and I'm wondering what's going on. The only reason HttpResponse would be returned is if Subject is not applied as a response type on the return. The template that generates the returntype is this:

{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}

which would mean that returnType is not set for that operation.

Could i get a peak at the endpoint yaml you are using to confirm whether my hypothesis is correct?

kenisteward · 2017-11-21T15:33:06Z

@HermenOtter I also have a sneaking suspicion I've introduced a bug with the return type for file based requests. Do you have any endpoints that return files by any chance?

HermenOtter · 2017-11-22T12:51:50Z

@kenisteward Strange, I remember it generating the method overloads with Subject as type. I do not have endpoints that return files, so I can't help you with that. I have sent you the yaml of the endpoint.

kenisteward · 2017-11-22T12:55:25Z

@HermenOtter what do you mean when you change the types it works? You have an example?

HermenOtter · 2017-11-22T13:17:40Z

@kenisteward In a previous version it seemed to work, but it did not: causing my confusion. Could it be an allof is causing my problem(see snippet below)?

responses:
        200:
          description: "A subject"
          headers:
            ETag:
              type: string
          schema:
            type: "object"
            allOf:
            - $ref: "#/definitions/Subject"
            - type: "object"
              properties:
                links:
                  $ref: 'https://api.swaggerhub.com/...

HermenOtter · 2017-11-22T13:50:26Z

The allOf problem seems to be an issue outside the scope of this PR. I think that if @kenisteward can confirm the returning of a file in an endpoint works, the PR can be merged. @wing328

wing328 · 2017-11-22T16:22:36Z

I'll merge it tomorrow (Thur) if no one has further feedback or question on this PR.

kenisteward · 2017-11-23T17:54:46Z

@wing328 thanks!

…

On Thu, Nov 23, 2017, 12:50 PM William Cheng ***@***.***> wrote: Merged #6955 <#6955>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6955 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMPLtb5mWIL2TUA2YsjWuVBiHji79eTSks5s5bBagaJpZM4Qc1SV> .

HermenOtter · 2017-11-24T09:12:49Z

Thank you @wing328!

kenisteward · 2017-11-24T15:16:59Z

@HermenOtter As a note, the master branch currently only supports features for OAS 2.0. allOf, oneOf, anyOf, not are all OAS 3.0 features. That should be implemented on the 3.0.0 branch if we were to support it.

docs https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/

kenisteward added 2 commits November 13, 2017 20:50

Updated swagger-ui wget url to https

fd09a13

Issue swagger-api#6902 Add Observe/ReportProgress pass through variab…

7f49a1d

…les to the response for each api call for angular 4.3 and above

kenisteward mentioned this pull request Nov 14, 2017

[Typescript][Angular] Response headers are not available. #6902

Closed

macjohnny reviewed Nov 14, 2017

View reviewed changes

Issue swagger-api#6902 Fixed problem where extra comma as generated a…

6a69b8a

…nd should not have been.

Issue swagger-api#6902 Fixed compiltion issue for Angular 4.3

99bdde1

wing328 added Client: TypeScript Enhancement: General labels Nov 15, 2017

wing328 added this to the v2.3.0 milestone Nov 15, 2017

bedag-moo reviewed Nov 15, 2017

View reviewed changes

wing328 merged commit 8eb33f9 into swagger-api:master Nov 23, 2017

Conversation

kenisteward commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Techies!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HermenOtter commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HermenOtter commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenisteward commented Nov 14, 2017

Uh oh!

HermenOtter commented Nov 14, 2017

Uh oh!

kenisteward commented Nov 14, 2017

Uh oh!

kenisteward commented Nov 14, 2017

Uh oh!

kenisteward commented Nov 14, 2017

Uh oh!

HermenOtter commented Nov 15, 2017

Uh oh!

kenisteward commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedag-moo commented Nov 15, 2017

Uh oh!

bedag-moo commented Nov 15, 2017

Uh oh!

HermenOtter commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenisteward commented Nov 16, 2017 via email

Uh oh!

HermenOtter commented Nov 21, 2017

Uh oh!

kenisteward commented Nov 21, 2017

Uh oh!

kenisteward commented Nov 21, 2017

Uh oh!

HermenOtter commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenisteward commented Nov 22, 2017

Uh oh!

HermenOtter commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HermenOtter commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented Nov 22, 2017

Uh oh!

kenisteward commented Nov 23, 2017 via email

Uh oh!

HermenOtter commented Nov 24, 2017

Uh oh!

kenisteward commented Nov 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

kenisteward commented Nov 14, 2017 •

edited

Loading

HermenOtter commented Nov 14, 2017 •

edited

Loading

HermenOtter commented Nov 14, 2017 •

edited

Loading

HermenOtter commented Nov 16, 2017 •

edited

Loading

HermenOtter commented Nov 22, 2017 •

edited

Loading

HermenOtter commented Nov 22, 2017 •

edited

Loading

HermenOtter commented Nov 22, 2017 •

edited

Loading

kenisteward commented Nov 24, 2017 •

edited

Loading