Skip to content

[typescript-rxjs] drop unneeded function wrapping #2332

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

Merged
merged 6 commits into from
Mar 8, 2019

Conversation

denyo
Copy link
Contributor

@denyo denyo commented Mar 8, 2019

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, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/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: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This PR removes unneeded wrapping functions within api controllers.

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10)

@auto-labeler
Copy link

auto-labeler bot commented Mar 8, 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.

Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@macjohnny
Copy link
Member

@denyo btw. can you remind we why there is this separation get...() and get...Raw()?

@denyo
Copy link
Contributor Author

denyo commented Mar 8, 2019

@macjohnny I am not really sure. I just took what was already there in typescript-fetch which this generator is based on.

The only thing that I can think of and which happened to me in my current project is that you need the raw xhr response in order to extract a certain header (e.g. for pagination).

I was able to achieve this by extending the BaseAPI's protected request() by ResponseOpts:

protected request<T>(reqOpts: RequestOpts, resOpts?: ResponseOpts): Observable<T> {
...
    if (resOpts && resOpts.response === 'raw') {
        return res as any as T;
    } else {
        return res.response as T;
    }
...
}

In the api controller I then have something like

private loginUsingPOSTRaw(requestParameters: LoginUsingPOSTRequest, responseOptions?: ResponseOpts): Observable<AjaxResponse> {
    ...
    return this.request<AjaxResponse>({
        ...
    }, responseOptions);
}

loginUsingPOST(requestParameters: LoginUsingPOSTRequest): Observable<UserDto>
loginUsingPOST(requestParameters: LoginUsingPOSTRequest, responseOptions: ResponseOpts): Observable<AjaxResponse>

loginUsingPOST(requestParameters: LoginUsingPOSTRequest, responseOptions?: ResponseOpts): Observable<any> {
    return this.loginUsingPOSTRaw(requestParameters, responseOptions);
}

And where ever I call this function with { response: 'raw' } I can then extract a header with res.xhr.getResponseHeader(SOME_KEY).

I did these modifications manually. So they aren't part of the generator as of now.

@macjohnny
Copy link
Member

ok, but currently there is no difference between the two, right?

@denyo
Copy link
Contributor Author

denyo commented Mar 8, 2019

Yes, right now they have the same signature and the public one is calling the private RAW one.

@denyo
Copy link
Contributor Author

denyo commented Mar 8, 2019

It looks like in the typescript-fetch client this was used to unwrap the response promise and have the right type since promises don't support generics? If thats the case I can simplify this client.

@macjohnny
Copy link
Member

It looks like in the typescript-fetch client this was used to unwrap the response promise and have the right type since promises don't support generics? If thats the case I can simplify this client.

sounds reasonable, with rxjs, the flow is straight-forward and nicely typed

@denyo denyo changed the title [typescript-rxjs] make internal functions private [typescript-rxjs] drop unneeded function wrapping Mar 8, 2019
@denyo
Copy link
Contributor Author

denyo commented Mar 8, 2019

There we go.

Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wing328 wing328 added this to the 4.0.0 milestone Mar 8, 2019
@wing328 wing328 merged commit ab8ee71 into OpenAPITools:master Mar 8, 2019
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Mar 9, 2019
* master: (758 commits)
  Add support for free form requests (OpenAPITools#2288)
  [typescript-rxjs] drop unneeded function wrapping  (OpenAPITools#2332)
  [typescript-fetch] Guard array mapping against undefined on optional array model properties (OpenAPITools#2324)
  Fix regex in Python server model code (OpenAPITools#2314)
  Add .travis.yml and Gemfile.lock to ruby security test folder (OpenAPITools#2330)
  Add a link to CSDN article (OpenAPITools#2331)
  [Maven] fix Spaces in Windows user path breaks build on test goal (OpenAPITools#2318)
  [PHP] fix bad links in Model docs (OpenAPITools#2316)
  [java]: fix datatype for non-multipart file request body (OpenAPITools#2271)
  Removed JFCote from core team (OpenAPITools#2315)
  [R sample] fix CircleCI error of outdated sample (OpenAPITools#2313)
  [Java] Bean Validation for decimalmin/max incorrect when exclusive set (OpenAPITools#2115)
  Java Spring : fix defaultValue annotation double quoted in api operation (OpenAPITools#2267)
  Java RESTEASY : fix defaultValue annotation double quoted in api operation (OpenAPITools#2268)
  [PHP] Username checks OpenAPITools#1408 (OpenAPITools#1892)
  [typescript-fetch] remove namespaces in enums (OpenAPITools#2123)
  [java-server-msf4j] fix and upgrade (OpenAPITools#2303)
  fix test script path in CONTRIBUTING.md (OpenAPITools#2290)
  Dart queryargs (OpenAPITools#2250)
  add Blueplanet language (OpenAPITools#2184)
  ...
@denyo denyo deleted the typescript-rxjs-private branch March 11, 2019 07:37
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.

3 participants