[Typescript][Fetch] client refactoring #569
Conversation
|
Sorry for messing up the first PR. I use gitlab all day at work and they have the source/target selection the other way around 🤣 |
|
@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) |
There was a problem hiding this comment.
Oh :D Worked out in the end. Thanks for the PR!
From what I saw in the code, this change mostly removes files and changes comments, but does not fix any issues? (correct me if i'm wrong). Is this PR missing some changes you did on your fork?
e.g. package.json, tsconfig.json etc.
| * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). | ||
| * https://openapi-generator.tech | ||
| * NOTE: This class is auto generated by the swagger code generator program. | ||
| * https://github.com/swagger-api/swagger-codegen.git |
There was a problem hiding this comment.
Could you please revert this change?
| export enum {{classname}} { | ||
| {{#allowableValues}} | ||
| {{#enumVars}} | ||
| {{{name}}} = <any> {{{value}}}{{^-last}},{{/-last}} | ||
| {{{name}}} = {{{value}}}{{^-last}},{{/-last}} {{#description}}// {{description}}{{/description}} |
There was a problem hiding this comment.
Is there a reason why <any> was removed?
This was not removed in modelGeneric.mustache (see below)
There was a problem hiding this comment.
No I think I must have just sweept it away when changeing things. I can't see a reason for it though, enum values should be true to what they really are without type erasure.
| @@ -1,32 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
I think we should still have a package.json and tsconfig.json
There was a problem hiding this comment.
@TiFu I'm happy to put these back in if required. I removed them because i've found that there's a general level of simplicity gained from just having a single file output. But perhaps with typescript project references coming in TS 3.0 it might be worth while to have.
There was a problem hiding this comment.
package.json just makes it simpler to include in packages just as a dependency.
tsconfig.json should definitely be included, e.g. specify EcmaScritp version.
| * {{{description}}} | ||
| * @export | ||
| * @interface {{classname}} | ||
| */ |
There was a problem hiding this comment.
I think this comment section should remain instead of the new one.
| * {{{description}}} | ||
| * @type {{=<% %>=}}{<%&datatype%>}<%={{ }}=%> | ||
| * @memberof {{classname}} | ||
| */ |
There was a problem hiding this comment.
See above - i'd prefer this comment over the new one
| const localVarHeaderParameter = {} as any; | ||
| const localVarQueryParameter = {} as any; | ||
| {{#hasFormParams}} | ||
| const localVarFormParams = new url.URLSearchParams(); |
There was a problem hiding this comment.
@TiFu my fork does away with the dependency on the URL module and uses browser native APIs such as FormData and encodeURIComponent.
This solves issues such as swagger-api/swagger-codegen#6403
From memory some users encountered issues with @types definitions for isomorphic fetch as well, but I can't find the bug reports :/
| {{#returnType}} | ||
| responseType: this.getResponseType('{{{returnType}}}'), | ||
| {{/returnType}} | ||
| modelPropertyNaming: '{{modelPropertyNaming}}', |
There was a problem hiding this comment.
@TiFu We use the modelPropertyNaming flag to do response model property renaming. Currently it uses a runtime method to do the conversion but a better implementation would modify this to use a property name map.
The existing codegen, using the default camelCase for model property naming just causes broken code for clients. This is a major barrier for new users IMO when their first experience is a generator results in a broken default.
| body: formData, | ||
| {{/hasFormParams}} | ||
| {{#returnType}} | ||
| responseType: this.getResponseType('{{{returnType}}}'), |
There was a problem hiding this comment.
@TiFu this fork uses the response type of the request to support handling "string" response types. The original generator just calls response.json() which crashes when the response is plain text.
I'm sure this issue has been recoded in the original swagger codegen issue tracker but someone picked it up on my fork as well: Place1/swagger-codegen-typescript-browser#2
There was a problem hiding this comment.
@TiFu This change also allows the client to support HTTP 204 no-content responses. The original generator will call response.json() which throws a SyntaxError when the response has no content because of JSON.parse("")
| this.middleware = configuration.middleware; | ||
| } | ||
|
|
||
| withMiddleware<T extends BaseAPI>(this: T, ...middlewares: Middleware[]) { |
There was a problem hiding this comment.
@TiFu our generator supports middleware. This is a very useful feature and something that most HTTP clients contain. My colleagues have been able to make good use of this feature across different projects to implement logging, automatic request retries, timeouts, circuit breakers, automatic token refresh logic, automatic logout logic, complex authentication/authorization challenges that are custom or out-of-scope for the built in security definitions, etc.
| {{#operations}} | ||
| {{#operation}} | ||
| {{#allParams.0}} | ||
| export interface {{operationIdCamelCase}}Request { |
There was a problem hiding this comment.
@TiFu this fork also generates interfaces for all API parameters. This is very helpful as a consumer because it allows you to reference them in your own methods/variables. The original generator uses inline type signatures which aren't reusable.
| {{/isOAuth}} | ||
| {{/authMethods}} | ||
| {{#hasFormParams}} | ||
| const formData = new FormData(); |
There was a problem hiding this comment.
@TiFu because this fork uses FormData multipart/form-data requests work as expected.
This fixes:
|
Oh thanks for the write up! No idea how I missed these changes earlier. Gonna do an in-depth review tonight. |
|
haha no worries, it's hard to see anything with a diff like this one 🤓 |
|
@macjohnny hey i've had a look at that PR, my changes here implement all the changes over there so I think it's covered (hopefully I didn't miss anything). |
|
@Place1 great, so it should work out |
|
Something I think we should try to add to this PR is to change the modelPropertyNaming implementation. Currently it's a runtime method that generally works, but will fail in certain cases such as properties with a leading underscore. I'm not familiar with how other generators (outside of typescript/js) implement this feature. We should probably copy the approach so we can make sure this part of the generator is bullet proof. |
|
Alright the PR looks good to me (and I realized why I missed all those changes - Shippable complains that the samples aren't up to date: Appveyor seems to be missing a README.mustache? travis complains that the code doesn't compile |
|
@TiFu thanks for the breakdown. I'll try to fix these CI issues in the next few days. I'll also confirm that my fork supports:
Other things i'd like to address if people are still onboard is: Perhaps we could adopt an implementation similar to the |
|
@emreavsar the additional headers in you are asking for in swagger-api/swagger-codegen#8444 could be solved by using this changed typescript-fetch generator and provide an appropriate middleware, i.e. an interceptor that sets your headers. |
|
@macjohnny done. I'll be away for a few days do i'll have to get to the other action items later. I'll add them as a check list in this PR |
|
If anyone else has time i'd be great to hear some feedback for the API around middleware. One concern I have is that there's too many positional arguments: Does anyone have an opinion on a better API? |
|
Maybe move the positional arguments into a separate class or interface? and then the same thing for |
|
Ok CI is now passing. I'm working on a commit for the last check list item |
…equest bodies. added support for blob data type for files/binary.
|
@wing328 any updates? |
|
If no further question/feedback from anyone, I'll merge this PR into 4.0.x branch (breaking changes without fallback) this coming weekend. |
|
@Place1 thanks for the contribution, which has been included in the 4.0.0-beta release: https://twitter.com/oas_generator/status/1079727020374806529. Happy New Year and looking forward to more collaboration and contributions in 2019! |
* added my fork from https://github.com/Place1/swagger-codegen-typescript-browser * ran bin/typescript-fetch-petstore-all.sh * use FormData.append rather than .set for IE11 compat * reverted change to licenseInfo.mustache * reverted some comments * added package.json and tsconfig.json back to the generator * added support for blob (application/octet-stream) responses * models and apis are now in folders * added support for modelPropertyNaming based on the spec * bug fix * updated samples * Restore pom.xml for typescript project * Restore samples/client/petstore/typescript-fetch/tests/default/package.json ≈ * added support for response type Date conversion * updated samples * Rework pom in "samples.ci" * Restore "samples/client/petstore/typescript-fetch/tests/default" * updated configuration class to use property getters to allow clients to implement configuration values as getters * added {{datatype}}ToJSON functions to handle serialization and naming conversions * fixed missing import * fixed compilation error. updated samples * 1 character change to get CI to run again * updated samples * added support for array type request body * updated tests * support for optional request bodies * updated models json converters to handle undefined inputs (to simplify usage in optional contexts like optional request bodies) * updated samples * updated tests * changed to typescript version 2.4 * updated samples * support for optional properties being null, undefined or omitted * updated samples * bug fix * bug fix * updated samples * ran npm install in test project * patch to get tests running * added support for retrieving raw response. added support for binary request bodies. added support for blob data type for files/binary.
|
@Place1 @TiFu @wing328 I'm not sure I understand this new fetch generator. It is breaking my entire code and I might not be able to upgrade to the latest version of OpenApi Generator because of this. It seems all the signature have changed. For exemple, before this, my function With the new generator, I need to pass a The new interface is defined that way: It looks to me like a wrapper. But by doing this, we do not respect the API spec and new object type needs to be passed. Objects that the user of the API / Client might not even know. I suggest we rollback this merge completely or that we remove the part that create new object type out of thin air. Unless there is a way or an option in the generator to use the real method signature but it doesn't seems that way in the generator code. |
How could I do this??? I need to send the version API into the header for all request. |
|
@mhidalgop see e.g. |
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif 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\.master,3.1.x,4.0.x. Default:master.Description of the PR
This PR applies a fork of typescript-fetch that I originally created here: https://github.com/place1/swagger-codegen-typescript-browser
The notable differences to the original typescript-fetch generator are outlines in the README.md
The changes in this fork also address the following open issues on this repo:
TODO:
Confirm the this PR fixes the following known issues:
ensure that application/octet-stream responses come back as a blob
ensure that dates are deserialized
ensure that date params and date-array params are serialized
Add support for:
add method overload to allow caller to receive native Response
General improvements:
modelPropertyNaming.The current runtime conversion has known edge cases. Only a perfect implementation (i.e. based on the spec itself) should be acceptable for an option like modelPropertyNaming to exist.