Add optional AxiosRequestConfig parameter to typescript-nestjs service functions#20222
Conversation
|
Hi @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @topce @akehir @petejohansonxo @amakhrov @davidgamero @mkusaka @joscha |
joscha
left a comment
There was a problem hiding this comment.
I think this makes sense to have? @macjohnny WDYT?
| {{#useSingleRequestParameter}} | ||
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}): Observable<AxiosResponse<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>>; | ||
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}): Observable<any> { | ||
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}options?: AxiosRequestConfig): Observable<AxiosResponse<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>>; |
There was a problem hiding this comment.
lets make it a named parameter and a less generic name to avoid clashes with other parameters:
{{nickname}}RequestConfig: {options?: AxiosRequestConfig}
@joscha wdyt?
There was a problem hiding this comment.
Yes, good feedback, I like it.
a1e86e2 to
1b84522
Compare
| {{#useSingleRequestParameter}} | ||
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}): Observable<AxiosResponse<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>>; | ||
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}): Observable<any> { | ||
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}{{nickname}}RequestConfig?: { options?: AxiosRequestConfig }): Observable<AxiosResponse<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>>; |
There was a problem hiding this comment.
is there a better name, now that it is a pojo with an options config object?
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}{{nickname}}RequestConfig?: { options?: AxiosRequestConfig }): Observable<AxiosResponse<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>>; | |
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}{{nickname}}Opts?: { options?: AxiosRequestConfig }): Observable<AxiosResponse<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>>; |
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}{{nickname}}RequestConfig?: { options?: AxiosRequestConfig }): Observable<AxiosResponse<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>>; | |
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}{{nickname}}Config?: { options?: AxiosRequestConfig }): Observable<AxiosResponse<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>>; |
or something? Given the AxiosRequestConfig is now only one of potentially many items?
Also:
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}{{nickname}}RequestConfig?: { options?: AxiosRequestConfig }): Observable<AxiosResponse<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>>; | |
| public {{nickname}}({{#allParams.0}}requestParameters: {{classname}}{{operationIdCamelCase}}Request, {{/allParams.0}}{{nickname}}RequestConfig?: { config?: AxiosRequestConfig }): Observable<AxiosResponse<{{#returnType}}{{{returnType}}}{{#isResponseTypeFile}}|undefined{{/isResponseTypeFile}}{{/returnType}}{{^returnType}}any{{/returnType}}>>; |
given the name of the class/interface?
There was a problem hiding this comment.
Yes, it's a good idea, so more like this?
{{nickname}}Opts?: { config?: AxiosRequestConfig }
@macjohnny wdyt ?
| withCredentials: this.configuration.withCredentials, | ||
| headers: headers | ||
| headers: {...headers, ...{{nickname}}RequestConfig?.options?.headers}, | ||
| ...{{nickname}}RequestConfig?.options, |
There was a problem hiding this comment.
shouldnt the lines be swapped to make sure the original headers are not completely overwritten if some headers are set in the options?
There was a problem hiding this comment.
Oh yes right !
| import { HttpService, Injectable, Optional } from '@nestjs/common'; | ||
| {{/useAxiosHttpModule}} | ||
| import { AxiosResponse } from 'axios'; | ||
| import { AxiosRequestConfig, AxiosResponse } from 'axios'; |
There was a problem hiding this comment.
I just noticed: should this be be a type only import if we assume TS > 3.8?
refs #20064
macjohnny
left a comment
There was a problem hiding this comment.
thanks for applying the changes!
Closes #20221
PR checklist
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*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)TypeScript committee members: @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04) @joscha (2024/10)