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

Support Angular 13 #10877

Merged
merged 1 commit into from
Dec 3, 2021
Merged

Support Angular 13 #10877

merged 1 commit into from
Dec 3, 2021

Conversation

cghislai
Copy link
Contributor

@cghislai cghislai commented Nov 17, 2021

Handle Angular 13 version

  • handles angular v13.0.1 with rxjs v7.4,
  • adds supportsES2020 flag to typescript generators, and updates tsconfig template accordingly,
  • Modify the api service template to pass DELETE body in the options parameters object, rather than as an additional second argument (see angular/angular@26ee5c9)
  • adds unit tests for ng12 and ng13 clients to build a simple angular app importing the generated client module.

Fixes #10831

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@cghislai
Copy link
Contributor Author

@petejohansonxo, @amakhrov, can you have a look?

@cghislai
Copy link
Contributor Author

cghislai commented Nov 17, 2021

re: the DELETE body parameterr: the change was originally introduced in edb88d9. Howver, the generated client with typescript-angular v12 does not adds that body parameter; while with v13 in this change, a null paramter was present breaking compilation.
@topce can you check ?

@topce
Copy link
Contributor

topce commented Nov 17, 2021

Hi @cghislai
Thank you for contribution !
For Angular version >= 12.1.0 it should allowed delete body .
So for version prior to 12.1.0 it should use isMethodPutOrPatchOrPost
and for version after 12.1.0 deleteAcceptsBody is true
it should use isBodyAllowed (because body is allowed in delete method)
in angular template

@cghislai
Copy link
Contributor Author

So for version prior to 12.1.0 it should use isMethodPutOrPatchOrPost and for version after 12.1.0 deleteAcceptsBody is true it should use isBodyAllowed (because body is allowed in delete method) in angular template

That is still the behavior with this change. However, the documentation at https://angular.io/api/common/http/HttpClient#delete lists an optional body property in the options parameters, while the template I modified passed it as a standalone parameter like for patch/put/posts. So Im not sure how it could work or if I missed something.

@topce
Copy link
Contributor

topce commented Nov 17, 2021

@cghislai
Maybe you are right, unfortunately there was not example in yaml
file for delete with body to test immediately .
And delete has 15 overloads !
Can you fix template this to work in all cases ?

@cghislai
Copy link
Contributor Author

@topce
the requested change here should work for both v12 and v13.

Re: the build error, im not sure what is going on, the test pass here, but travis is running an old chrome version im not sure if that is the cause. The run-in-docker script complained about missing make for the python client.

Local logs
[INFO] 
[INFO] --- exec-maven-plugin:1.2.1:exec (npm-test) @ TypeScriptAngular13PestoreClientTests ---

> [email protected] test
> ng test "--progress=false" "--no-watch" "--browsers" "ChromeHeadless"

17 11 2021 20:09:55.171:INFO [karma-server]: Karma v6.3.9 server started at http://localhost:9876/
17 11 2021 20:09:55.173:INFO [launcher]: Launching browsers ChromeHeadless with concurrency unlimited
17 11 2021 20:09:55.185:INFO [launcher]: Starting browser ChromeHeadless
17 11 2021 20:09:58.366:INFO [Chrome Headless 95.0.4638.69 (Linux x86_64)]: Connected on socket TfRlM78hbnuSTcx8AAAB with id 13635488
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 0 of 23 SUCCESS (0 secs / 0 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 1 of 23 SUCCESS (0 secs / 0.009 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 2 of 23 SUCCESS (0 secs / 0.017 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 3 of 23 SUCCESS (0 secs / 0.02 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 4 of 23 SUCCESS (0 secs / 0.022 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 5 of 23 SUCCESS (0 secs / 0.025 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 6 of 23 SUCCESS (0 secs / 0.027 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 7 of 23 SUCCESS (0 secs / 0.032 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 8 of 23 SUCCESS (0 secs / 0.034 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 9 of 23 SUCCESS (0 secs / 0.037 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 10 of 23 SUCCESS (0 secs / 0.04 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 11 of 23 SUCCESS (0 secs / 0.041 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 12 of 23 SUCCESS (0 secs / 0.043 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 13 of 23 SUCCESS (0 secs / 0.046 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 14 of 23 SUCCESS (0 secs / 0.049 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 15 of 23 SUCCESS (0 secs / 0.052 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 16 of 23 SUCCESS (0 secs / 0.055 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 17 of 23 SUCCESS (0 secs / 0.057 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 18 of 23 SUCCESS (0 secs / 0.097 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 19 of 23 SUCCESS (0 secs / 0.103 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 20 of 23 SUCCESS (0 secs / 0.108 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 21 of 23 SUCCESS (0 secs / 0.113 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 22 of 23 SUCCESS (0 secs / 0.116 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 23 of 23 SUCCESS (0 secs / 0.119 secs)
Chrome Headless 95.0.4638.69 (Linux x86_64): Executed 23 of 23 SUCCESS (0.148 secs / 0.119 secs)
TOTAL: 23 SUCCESS
TOTAL: 23 SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  9.826 s
[INFO] Finished at: 2021-11-17T20:10:00+01:00
[INFO] ------------------------------------------------------------------------

@topce
Copy link
Contributor

topce commented Nov 23, 2021

@cghislai Thank you for contribution !
It is quite big PR , so not easy to check
but LGTM

Maybe there is some infrastructure problem so build fail!

@topce
Copy link
Contributor

topce commented Nov 26, 2021

Hi @wing328
maybe to merge this one @cghislai add support for angular 13
also fix my bug when delete have body .
For me it looks good, but If needed also other typescript expert can review code.

@cghislai
Copy link
Contributor Author

I will cleanup this mr to ease reviewing and hopefully make the tests pass.

  • i will remove the es2020 config flag and raise and issue to consider the best way to support various javascript versions targets
  • i will remove my changes to the delete request body and raise an issue as for the best way to support this: the openapi documentation mention The requestBody is only supported in HTTP methods where the HTTP 1.1 specification RFC7231 has explicitly defined semantics for request bodies., and the http spec mention A payload within a DELETE request message has no defined semantics. A requestBody element under a DELETE method invalidates the spec.
  • i will create separate pull requests for the tests.

@amakhrov
Copy link
Contributor

amakhrov commented Dec 2, 2021

@cghislai this looks good and clean - only has bare minimum of changes now. I like the idea of having separate PRs to address other outstanding issues

@macjohnny macjohnny merged commit b915ad9 into OpenAPITools:master Dec 3, 2021
@macjohnny macjohnny added this to the 5.3.1 milestone Dec 3, 2021
@cghislai cghislai deleted the ng13 branch December 3, 2021 08:41
@marinus-suniram
Copy link
Contributor

I'm getting a compile error on Angular 13.1.1.

It seems that httpClient.delete expects only 2 parameters, the body is part the options.

return this.httpClient.delete<any>(`${this.configuration.basePath}/item/loader`,
            null,
            {
                context: localVarHttpContext,
                params: localVarQueryParameters,
                responseType: <any>responseType_,
                withCredentials: this.configuration.withCredentials,
                headers: localVarHeaders,
                observe: observe,
                reportProgress: reportProgress
            }
        );
    delete(url: string, options?: {
        headers?: HttpHeaders | {
            [header: string]: string | string[];
        };
        context?: HttpContext;
        observe?: 'body';
        params?: HttpParams | {
            [param: string]: string | number | boolean | ReadonlyArray<string | number | boolean>;
        };
        reportProgress?: boolean;
        responseType?: 'json';
        withCredentials?: boolean;
        body?: any | null;
    }): Observable<Object>;

@macjohnny
Copy link
Member

@marinus-suniram thanks for reporting. Do you want to fix that?

@amakhrov
Copy link
Contributor

Also there is an open PR reverting body support in DELETE, fixing the compilation issue: #10976

@nieprzecietny
Copy link

if you want it back working put inside openapitools.json
"generator-cli": { "version": "5.3.0" }

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

Successfully merging this pull request may close these issues.

[REQ] [typescript-angular] Support Angular 13
6 participants