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

[TypeScript] add new generator for Axios #892

Merged

Conversation

nicokoenig
Copy link
Contributor

@nicokoenig nicokoenig commented Aug 24, 2018

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: master, 3.3.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

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

@nicokoenig
Copy link
Contributor Author

Ping @wing328

@wing328
Copy link
Member

wing328 commented Aug 24, 2018

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@wing328
Copy link
Member

wing328 commented Aug 24, 2018

cc the technical committee for review

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

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.

I can't find differences to the typescript-fetch generator. What is the new feature of this generator?

@@ -0,0 +1,45 @@
## {{npmName}}@{{npmVersion}}

This generator creates TypeScript/JavaScript client that utilizes [Fetch API](https://fetch.spec.whatwg.org/). The generated Node module can be used in the following environments:
Copy link
Member

Choose a reason for hiding this comment

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

is this correct? shouldn't it be about axios?

@@ -0,0 +1,368 @@
/// <reference path="./custom.d.ts" />
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I believe @nicokoenig has made some updates to use axios.

@nicokoenig have you committed those changes to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet, changes to come. I made the PR on behalf of @wing328 so that he could check for initial mistakes 👍

@wing328 wing328 added the WIP Work in Progress label Aug 25, 2018
@nicokoenig
Copy link
Contributor Author

I pushed some updates... could someone please have a look? ☺️

"build": "tsc --outDir dist/",
"prepublishOnly": "npm run build"
},
"dependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, sorry I missed that.

Commit is already pushed.

"description": "OpenAPI client for {{npmName}}",
"author": "OpenAPI-Generator Contributors",
"keywords": [
"fetch",
Copy link
Member

Choose a reason for hiding this comment

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

Minor suggestion: replace "fetch" with "axios"?

@nicokoenig nicokoenig force-pushed the feature/typescript-axios-generator branch from 2ff3c41 to ecf2f60 Compare August 29, 2018 07:55
@nicokoenig
Copy link
Contributor Author

@wing328 Any updates here? 😺

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.

@wing328
Copy link
Member

wing328 commented Sep 2, 2018

@nicokoenig to elaborate on @macjohnny feedback a little bit, here are the scripts/batch files for TypeScript Fetch:

➜  openapi-generator git:(master) ✗ ls -l ./bin/typescript-fetch*
-rwxr-xr-x  1 williamcheng  staff  190 Aug  3 14:12 ./bin/typescript-fetch-petstore-all.sh
-rwxr-xr-x  1 williamcheng  staff  904 Aug  3 14:12 ./bin/typescript-fetch-petstore-interfaces.sh
-rw-r--r--  1 williamcheng  staff  181 Aug  3 14:12 ./bin/typescript-fetch-petstore-target-es6.json
-rwxr-xr-x  1 williamcheng  staff  925 Aug  3 14:12 ./bin/typescript-fetch-petstore-target-es6.sh
-rw-r--r--  1 williamcheng  staff  158 Aug  3 14:12 ./bin/typescript-fetch-petstore-with-npm-version.json
-rwxr-xr-x  1 williamcheng  staff  937 Aug  3 14:12 ./bin/typescript-fetch-petstore-with-npm-version.sh
-rwxr-xr-x  1 williamcheng  staff  873 Aug  3 14:12 ./bin/typescript-fetch-petstore.sh

Windows batch file:

➜  openapi-generator git:(master) ✗ ls -l ./bin/windows/typescript-fetch*
-rw-r--r--  1 williamcheng  staff  238 Aug  3 14:12 ./bin/windows/typescript-fetch-petstore-all.bat
-rw-r--r--  1 williamcheng  staff  430 Aug  3 14:12 ./bin/windows/typescript-fetch-petstore-interfaces.bat
-rw-r--r--  1 williamcheng  staff  439 Aug  3 14:12 ./bin/windows/typescript-fetch-petstore-target-es6.bat
-rw-r--r--  1 williamcheng  staff  451 Aug  3 14:12 ./bin/windows/typescript-fetch-petstore-with-npm-version.bat
-rw-r--r--  1 williamcheng  staff  394 Aug  3 14:12 ./bin/windows/typescript-fetch-petstore.bat

Please create something similar to generate the Petstore sample and you can copy the tests from TS Fetch Petstore.

If you need any help, please let me know.

@macjohnny
Copy link
Member

it might also be worth adding some integration test to ensure the generated code does not break in the future.
See e.g. https://github.com/OpenAPITools/openapi-generator/tree/master/samples/client/petstore/typescript-fetch/tests/default and https://github.com/OpenAPITools/openapi-generator/blob/master/pom.xml#L1027

{{/hasFormParams}}
{{#bodyParam}}
const needsSerialization = (<any>"{{dataType}}" !== "string") || localVarRequestOptions.headers['Content-Type'] === 'application/json';
localVarRequestOptions.body = needsSerialization ? JSON.stringify({{paramName}} || {}) : ({{paramName}} || "");

Choose a reason for hiding this comment

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

This needs to go in localVarRequestOptions.data - there is no body option in Axios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint.

@nicokoenig nicokoenig force-pushed the feature/typescript-axios-generator branch from ca156fb to 0e08315 Compare September 13, 2018 10:14
@wing328
Copy link
Member

wing328 commented Sep 13, 2018

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny please have another look as @nicokoenig has added test cases (petstore integration tests) and updated Travis CI config to run these tests as part of the build.

@wing328 wing328 added this to the 3.3.0 milestone Sep 13, 2018
@wing328 wing328 removed the WIP Work in Progress label Sep 13, 2018
@wing328
Copy link
Member

wing328 commented Sep 16, 2018

If no further feedback, I'll merge this PR into master on Monday so that we can start collecting feedback from TypeScript developers.

@wing328 wing328 merged commit 3027514 into OpenAPITools:master Sep 18, 2018
@wing328 wing328 changed the title feature: add scaffold for typescript axios template [TypeScript] add new generator for Axios Sep 18, 2018
@wing328
Copy link
Member

wing328 commented Sep 18, 2018

Tweet to promote the new generator: https://twitter.com/oas_generator/status/1041939441109983232

jaumard pushed a commit to jaumard/openapi-generator that referenced this pull request Sep 21, 2018
* feature: add generator for typescript/axios

* feature: add sample scripts and sample code

* fix: set request body in data property

* feature: add samples and tests for typescript axios client

* test: add tests for typescript axios client
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* feature: add generator for typescript/axios

* feature: add sample scripts and sample code

* fix: set request body in data property

* feature: add samples and tests for typescript axios client

* test: add tests for typescript axios client
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.

None yet

4 participants