Skip to content

[TypeScript][Fetch] Change return type from any -> Response#6774

Merged
wing328 merged 3 commits intoswagger-api:masterfrom
masaeedu:returntype
Oct 25, 2017
Merged

[TypeScript][Fetch] Change return type from any -> Response#6774
wing328 merged 3 commits intoswagger-api:masterfrom
masaeedu:returntype

Conversation

@masaeedu
Copy link
Copy Markdown
Contributor

@masaeedu masaeedu commented Oct 20, 2017

Fixes #6772

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 langauge.

Description of the PR

Change the return type of API methods for which a response schema is not available to reflect that they return a Promise<Response> (instead of Promise<any>).

Technical committee reviewers

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx

@kenisteward
Copy link
Copy Markdown
Contributor

@masaeedu have you checked this with a generated library to make sure Response was valid? I'm not sure if our tests cover it yet.

@masaeedu
Copy link
Copy Markdown
Contributor Author

masaeedu commented Oct 23, 2017 via email

@kenisteward
Copy link
Copy Markdown
Contributor

yup i gathered that. I just wanted to make sure you tried this locally with an api you have and everything turned out fine.

Just a safety net until we get better integration tests online.

@kenisteward
Copy link
Copy Markdown
Contributor

Also, to get ci to pass it seems as someone else's changes are needed. I had a pr as well and I just re-merged with master and that fixed it.

@masaeedu
Copy link
Copy Markdown
Contributor Author

@kenisteward Rebased, thanks. The client library I'm generating compiles fine with the change when lib: [..., "dom"] is specified in tsconfig.json (which is already a requirement for getting the current isomorphic-fetch typings to work). This doesn't have any effect at runtime that I can think of, since all the types will get erased, so I'm not sure how else I can test it. Do the integration tests not run tsc on the petstore sample?

@kenisteward
Copy link
Copy Markdown
Contributor

that's exactly what I wanted to know. As long as the one you generate for yourself works that's perfect :) I hope to in the future have a sample app that calls each of the endpoints.

Copy link
Copy Markdown
Contributor

@kenisteward kenisteward left a comment

Choose a reason for hiding this comment

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

Thanks for locally testing and the changes.

@masaeedu
Copy link
Copy Markdown
Contributor Author

Hi @wing328. Any chance of getting this merged today? I have some other stuff that depends on this.

@wing328 wing328 merged commit 49974c5 into swagger-api:master Oct 25, 2017
@wing328 wing328 added this to the v2.3.0 milestone Oct 25, 2017
@wing328 wing328 changed the title Change return type from any -> Response [TypeScript][Fetch] Change return type from any -> Response Oct 25, 2017
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