Openapi-generator: Rewrite of TypeScript Generators

[TiFu]

Motivation

We currently have 7 (!) different generators for TypeScript, which do not share common code. This makes introducing bug fixes to the TypeScript generator family very hard.

Goal

Create a general framework which can be used in any OOP language - unifying the generator structure and making the project easier to work with. At the beginning, I would like to look into merging the TypeScript generators into one and improve the architecture during the implementation.

Feedback

Tear the architecture apart! Focus on the bigger picture first (e.g. missing some important feature? something which is hard to implement with this architecture?) and then go into details (e.g. missing variables).
The TODOs in the text below mark parts which I did not think (enough) about yet. Feel free to fill in what's missing.

Architecture

Architecture (looks weird if I include it as an image. sorry!)

The key idea is to extract everything into its own component and hide it behind an interface. The key advantage of this architecture is

• extendability e.g. new http libraries or more modifiers for requests e.g. a request is fed through multiple components which modify it according to their rules e.g. server sets the correct request url
• testability - the components can be tested in isolation and can be mocked
• less duplication of code for different frameworks in the same language

I'll go through the different components one by one and try to explain the idea.

HTTP Library

This part should be clear - hide the details of making a HTTP request behind an interface so that we can use different libraries without changing the API Client. To achieve this we create our own data structures for requests and responses.

TODO: Use promises/futures?

Authentication

Same idea as HTTP library.
I want to highlight one part: the authentication interface has a method apply(Request r). This method applies the authentication scheme to the given request e.g. in the case of an API Key it would append the api key.

TODO: OAuth work flow with this implementation?

Response Models & Param Models

TODO: Should this be separated or can response & param models be used interchangeably? i.e. param Pet can also be a response model? If yes these two components should be merged.

This component contains model classes for each parameter & response as well as a (object) serializer and deserializer for these models (e.g. Array<JsonObject> in the API response is transformed to Array<Pet> by the ObjectDeserializer).

Server

This component contains the different servers as defined in OAS.

TODO: handling of variables. I would propose to offer setters for each variable.
Variables are not yet supported in OpenAPITools. See #793

Config

See current implementation.

API Client

The API client coordinates all other components.

1. Serializes the input parameters
2. creates a new request r
3. calls Server#apply(r)
4. calls `Authentication#apply(r)
5. deserializes the response
6. returns a promise with the deserialized response

Callbacks in OAS 3

TODO: create server, hide it behind an interface to make the library exchangeable and create one method for each callback endpoint.

Framework Specific Code

Anything framework specific goes here and acts as a proxy to APIClient.

Technical Committee

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

Project

William created a project for this issue: https://github.com/OpenAPITools/openapi-generator/projects/4

[jimschubert]

@TiFu I went through a similar (smaller) refactor for the C# generator to allow users to customize the generator's underlying HTTP client.

That's #737, if you're interested.

[macjohnny]

@TiFu I suggest to consider using RxJS / Observables. Maybe there is an easy way to make this configurable?

[TiFu]

What would be the use case for Observables in this case? What should be observable?

[macjohnny]

@TiFu the return value of the api call. see e.g. https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/typescript-angular-v6-provided-in-root/builds/with-npm/api/pet.service.ts#L70

the use case is that observables are the standard for http interaction in angular. Therefore in the template it should be easy to have a configuration option to either have a promise-based generator or an observable-based generator.

[TiFu]

I will think about that some more. It looks quite complicated to include Observables together with promises internally (especially observing the different options like body, response and events).

One option is to simply wrap promises in Observables e.g.

import { from } from 'rxjs';
var observable =  from(promise);

I'm just not sure how this could be integrated into the current HTTPLibrary design.

[macjohnny]

@TiFu it is very important to handle observables correctly and not only wrap promises, since promises can‘t be cancelled, while observables can, which is an essential feature.

[macjohnny]

However it is possible to convert an observable into a promise. This should fit all needs. Therefore internally it should be buolt on observables

[TiFu]

Oh okay. I have an idea how to solve this issue.

1. For each api/operation: generate a method which returns a RequestContext (this describes 'how' a request should be executed e.g. url, header params, query params etc)
2. if angular: Generate a class/method for each api/operation which gets the RequestContext from 1. and uses the normal angular http client (which returns an observable - exactly as in the old client) to make the request
3. if not angular: Generate a class/method for each api/operation which gets the RequestContext from 1. and uses the HttpLibrary to make the request

This encapsulates all api logic in 1. and just defers making the request to the library specific code section.

Does that make sense?

[TiFu]

Also #727.

The past 1 1/2 months have been insanely busy for me. I hope I will have some more time to work on this again.

And #347

[Place1]

After my fork is merged in I’ll be very keen to throw myself and time at this work. My company has run into many issues with the ng clients and node clients so I’m very motivated to help unify the generators.

[TiFu]

@Place1 Thanks! Let me know when you are ready/have time to look into it and I will give you a quick overview over the code base - you can already find it on the typescript-refactor branch. The name of the new generator is typescript

I have finished a basic prototype for ts-fetch today (still has a ton of todos/bugs/only basic features, but it outlines the direction I want to take for the other generators). As a next step, I will go through the other generators step by step and try to find the most common features in all generators and create issues for them. There's also a project for the ts-refactor, so I'll assign all relevant issues to this project.

I will be away from tomorrow until Monday evening, so I won't be able to do much until then.

CLI Arguments I currently use to generate the client:

generate -i openapi-generator/modules/openapi-generator/src/test/resources/2_0/petstore.yaml -g typescript -o openapi-generator/samples/client/petstore/typescript/builds/default

[acornett]

Not sure if this has been brought up elsewhere, but one issue I'm having adopting the generated typescript client is the use of namespaces for enums. If the typescript generator is being overhauled could alternate code styles be considered?
Babel 7 doesn't support namespaces in typescript (https://babeljs.io/docs/en/babel-plugin-transform-typescript) and advises the use of ES2015 modules instead. Some parts of the code are already using string unions as instead of a defined enum, or the enum could be declared without the namespace and use a prefix of the enclosing type for uniqueness.

[madshargreave]

@acornett Do you know of any temporary workarounds for this or will I have to avoid using enums all-together?

[TiFu]

@acornett noted. Although I have no timeline for finishing the prototype of the overhaul at this time.

[acornett]

@madshargreave I have not been able to dig into it enough to develop a work around. I will see about working on a more concrete proposal (or if my schedule really opens up maybe a pull request!)

[macjohnny]

see also https://github.com/OpenAPITools/openapi-generator/pull/1884

[ceefour]

@TiFu generally the idea is good. However I would strongly disagree on making more abstractions (like HTTP library) on the generated files. Generated files should use directly the chosen HTTP library (e.g. fetch), using an abstraction will make the files more complex and less readable especially to those new to openapi-generator (which they shouldn't need to understand). The goal is, as long as they know fetch then they can read, use, and modify the generated code.

[TiFu]

@ceefour Thanks for the feedback. Unfortunately not using an abstraction for HTTP makes the generator code far more complex e.g. we would have to change how we create the requests (including header params, query params, ...) for every HTTP library and wherever we use it. This would lead to duplicated code on our site which is harder to maintain and prone to bugs.

In the end, the goal would be for people to not have to modify the code directly, but make it extensible through middleware (like we plan on doing for HTTP - you can simply modify the HTTP Request and before its being parsed/send).

Other opinions regarding @ceefour's feedback?

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

[ceefour]

@TiFu is it possible to put the "library" on the generator instead.

So parent generators can call implementation-specific "child" generators, keeping the overall project structure uniform.

I'm afraid that by having a runtime library, while it does reduce generator complexity but overall complexity is the same, just moved from the generator to the runtime library.

[macjohnny]

@TiFu I support your point that without runtime http abstraction (which is just a single layer and easy to understand) the complexity of the codegen cannot be decreased.
@ceefour can you give an example of your idea about "putting the library on the generator instead"?

Personally, I never have a look at the generated code in a project, since it does its job.

[ceefour]

@ceefour can you give an example of your idea about "putting the library on the generator instead"?

I apologize if this is just a simple analogy not real code, I'm not familiar with how it works.

Library on runtime, generates:

runtime.request(method, params, headers)

then the runtime handles most of the logic, generator is simple.

Library on generator:

{{#requestfunc}}
  ..method, params, headers..
{{/requestfunc}}

requestfunc implementation is dependent on generator. More logic on generator, but runtime is simple.

So in my view, both approaches have similar overall complexity, but the proportion is different.

Note: I have to support that the current runtime.js in typescript-fetch is of reasonable complexity, which is good. I just hope that it doesn't get bigger and even more complex with the rewrite.

Personally, I never have a look at the generated code in a project, since it does its job.

I hope so too. Recently I got #1133 (echoed by #1873, #1577, #1446, #457) so I needed to patch the generated code as a workaround in the meantime before the generator is fixed. It's the "leaky abstraction" problem which is hard to avoid, but understandable.

[dalinarkholin]

Any timeline on this? I just ran into this issue trying to upgrade to babel 7 only to realize that namespaces are not supported.

[TiFu]

@dalinarkholin Unfortunately, I don't have an ETA at this time. Totally depends on how much time I have in the next few weeks and afterwards.

[bodograumann]

Seems like there hasn’t been any progress in some time on this!? What is the status, @TiFu ?

Have you decided about using rxjs, cancelable promises polyfill or no cancelation?
How do you intend to build the different framework-specific variants onto this template?

[fuzzzerd]

This is a really good idea, and will greatly help the typescript community using openapi generator. The ecosystem is quite fractured. Having a single typescript generator with the various abstractions being optional (http, promise/observable, authentication) arguments would really be a game changer.

While my ability to work on the underling java platform is practically none, I'd be more than happy to help with testing and I could contribute to the templates once some of the decisions are made here.

[TiFu]

@bodograumann unfortunately i don't have any updates and for the foreseeable future, I also don't know when I will have time to work on it.

@fuzzzerd I'll create an overview of where we currently stand and what the major decision points are this weekend. I think I mostly stuck to what I originally proposed and I think I even implemented one of the generators already. I'll tag you when I post the update.

[TiFu]

Soo as promised, an overview of the current implementation. Feedback & Discussions welcome!

Major Decision Points

• Buffer (Node) vs Blob (Browser) support
  
  
  
  • Idea: Add a flag which indicates whether Blob or Buffer should be used for files
  
  
• Cleanup TypeScriptClientCodegen.java (our new generator class)
• Integration of OAuth2 (How?)
• Support for Inversify
  
  
  
  • How can inversify be integrated into the current design?
  
  
• Integration of different current generators (I had a list but forgot to take it with me to the US)
  - most just use different http implementations anyway - so the only change is adding a new HTTP implementation to ./typescript/http (see below) and adding a generator file importing the right dependencies
• Structure of tests
  
  
  
  • less code duplication
  
  
• Add interfaces for models?

Directory Structure:

Tree for main/resources/typescript

├── api  # Contains parser for incoming / outgoing requests (e.g. param serialization/deserialization)
│   ├── api.mustache
│   ├── baseapi.mustache
│   ├── exception.mustache
│   └── middleware.mustache
├── auth  # Contains one class for each type of auth (No Auth, ApiKey, HttpBasic, ...)
│   └── auth.mustache
├── configuration.mustache # Configuration File
├── generators # Actual implementation of generators
│   ├── fetch-api.mustache # Implementation of fetch-api generator
│   ├── jquery.mustache # Implementation of jQuery generator
│   └── types
│       ├── ObservableAPI.mustache  # Base API, uses RequestFactory/ResponseProcessor from /api/*
│       └── PromiseAPI.mustache # Wrapper around ObservableAPI
├── git_push.sh.mustache # Not updated at all so far
├── http # HTTP Implementations (e.g. for jquery/isomorphic-fetch/...) based on generic http interface defined in http.mustache
│   ├── http.mustache
│   ├── isomorphic-fetch.mustache
│   ├── jquery.mustache
│   └── servers.mustache # Implementation of OAI 3's server definition
├── licenseInfo.mustache
├── model # Model classes 
│   ├── model.mustache
│   ├── models_all.mustache
│   └── ObjectSerializer.mustache
├── package.mustache
├── README.mustache
├── tsconfig.mustache
└── util.mustache # Utility function to check if a response code is in a specific range

Current Test Structure

├── default # ts-fetch
│   ├── package.json
│   ├── package-lock.json
│   ├── pom.xml
│   ├── test
│   │   ├── api
│   │   │   ├── PetApi.test.ts
│   │   │   └── pet.png
│   │   ├── auth
│   │   │   └── auth.test.ts
│   │   ├── http
│   │   │   └── isomorphic-fetch.test.ts
│   │   └── models
│   │       └── ObjectSerializer.test.ts
│   └── tsconfig.json
├── jquery # jquery / incomplete
│   ├── package.json
│   ├── package-lock.json
│   ├── pom.xml
│   ├── test
│   │   ├── api
│   │   │   ├── PetApi.test.ts
│   │   │   └── pet.png
│   │   └── http
│   │       └── jquery.test.ts
│   ├── test-runner.ts
│   ├── tests.ts
│   ├── tsconfig.json
│   └── webpack.config.js
└── package-lock.json

[bodograumann]

I had a bit of a problem with the git history, seems you did some rewrite after my last pull.

So now to answer my own question, as I understand the code: The intention is to only have a single generator lang “typescript” in the future. To choose a variant, you can define the framework config option. Currently there is fetch-api and jquery.

The actual api client seems to be in the types directory. Is there a specific reason, that it is called that way, @TiFu ?

[TiFu]

Mhh that's weird - I definitely didn't change anything since early May.

So now to answer my own question, as I understand the code: The intention is to only have a single generator lang “typescript” in the future. To choose a variant, you can define the framework config option. Currently there is fetch-api and jquery.

That is correct. Both fetch-api and jquery are still missing some features compared to the current generators in typescript-fetch / typescript-jquery but generally work.

The actual api client seems to be in the types directory. Is there a specific reason, that it is called that way, @TiFu ?

The main reason for this was to separate the frameworks (which export the appropriate Api OR an Api with a different interface (but compatible with) either ObservableApi or PromiseApi) from a more meta look at how the Api works behind the scene. If you feel like there is a better/clearer way to structure this, feel free to change it.

[bodograumann]

There is really a lot to consider and it’s not easy to get a proper understanding of all the factors involved. So that’s what I’m currently doing.

I’m using typescript-inversify currently, with some modifications. Afaics adding inversify support should not be too difficult. Just add a new option for it and insert some code for import and class / constructor decorators. Before we do that, we should be certain of the architecture though, to avoid unnecessary refactoring.

I’m a little worried about the current use of rxjs. The way the PromiseAPI is based on the ObserverAPI means that rxjs will always be a dependency. I can imagine that quite a few people would not want that. No idea though, how to better handle it, as of now.

[TiFu]

I’m using typescript-inversify currently, with some modifications. Afaics adding inversify support should not be too difficult. Just add a new option for it and insert some code for import and class / constructor decorators. Before we do that, we should be certain of the architecture though, to avoid unnecessary refactoring.

Sounds good.

I’m a little worried about the current use of rxjs. The way the PromiseAPI is based on the ObserverAPI means that rxjs will always be a dependency. I can imagine that quite a few people would not want that. No idea though, how to better handle it, as of now.

The main reason I used Observables was that I did not want to have to differentiate between Promise & Observable internally (e.g. in the HTTPLibrary).

One possible solution is writing a small wrapper which implements a bare minimum version of the Observer api (only the actual method calls used internally) using promises. We could then replace the rxjs imports with an import for our wrapper based on the generator we are currently generating (or if a flag like --without-rxjs (or --with-rxjs) is set )

[bodograumann]

Note regarding https://github.com/OpenAPITools/openapi-generator/issues/3814:

Generally most people probably don’t care very much about linting the generated code. Some (including me) might want to enable it though, while others want to disable it.
I would have thought it should be easy enough to enable/disable linting outside of the generated code. E.g. through .eslintignore or some other config file. The above issue and accepted PR seem to show that is not generally the case.
The solution in PR https://github.com/OpenAPITools/openapi-generator/pull/4110 now disables linting inline and thus makes it impossible to control linting through the config and ignore files. Afaik it’s now only possible by overriding the template. This does not seem desirable.

Conclusion:
We need a config option to enable or disable linting inline.

[snebjorn]

Regarding this effort. It should probably be allowed to submit general typescript issues. e.g. [BUG][typescript]

Currently it requires the format typescript-

https://github.com/OpenAPITools/openapi-generator/blob/c2ad14ea02f47dcca2acd8458428777b38e752fa/.github/auto-labeler.yml#L140

[jimschubert]

@snebjorn excellent suggestion! I've applied it in #4180.

[bodograumann]

I see you are using the form-data package. It seems that this is meant as an alternative to the browser built-in FormData object. It depends on the node packages stream and http. This seems similar to the issue of Buffer vs. Blob.

How should we handle this?

1. The generated code should run in the browser as well as in node. So we should generate isomorphic code.
2. The code is generated for either node or the browser and we define an option to select between the two.
3. The selected framework implies whether the code will run in the browser or in node.

[fuzzzerd]

@bodograumann I think your point about running on node vs browser is a good one. There's more at play than just packages. I have one issue: https://github.com/OpenAPITools/openapi-generator/issues/3833 but there could be others.

[TiFu]

@bodograumann

I think a mix between 2 & 3 would good i.e. if the option is not passed, we use the defaut (browser or node) for the selected framework, if the option is passed, this default is overwritten.

The implementation could be as simple as not importing form-data if the target is browser - both the package and the browser should be using the same interface anyway.

[bodograumann]

Sounds good to me, @TiFu, though I can imagine that we want to generate isomorphic code at some point. So for now having platform = "browser" | "node" and maybe later adding | "isomorphic" seems sensible.

Afaics of the existing typescript templates only typescript-node targets node, while all the other target the browser. (No idea if any are isomorphic)
So I think I will make the default platform for the existing fetch and jquery frameworks browser.

Also, not sure how to write tests for all the possible variations yet.

[TiFu]

The reason why I am against isomorphic code although it makes maintenance a bit easier is that that would increase the package size in the browser - especially if you take into account how many dependencies most js libraries have.

Obviously this only applies if we use a library but given the complexity of most browser/node APIs I don't see us implementing everything ourselves.

Also, not sure how to write tests for all the possible variations yet.

As long as we can hide the varying functionality behind an interface (similar to what you did for rxjs or how HTTPLibraries are currently implemented) it could be as easy as testing these interfaces for both browser and node + one integration test ( which we have anyway when testing end-to-end).

[bodograumann]

I am with you all the way there, @TiFu. Only the code told me a different story, using isomorphic-fetch and the like :-D

Not sure how much time I will have for the rest of the year, but if anybody wants to join in:
One thing that I would want to do for browser-support is to use the interface HttpFile = Blob & { readonly name: string } so we can use the File API in browsers that support it. For node Blob would mean Buffer.

After that I guess it would be good to create samples for fetch in browser and add tests for them to the current jquery test setup.

[TiFu]

:D fair enough :D

Sounds good. I will probably only have time to review PRs/comment on issues until end of January, but if anyone wants to give what @bodograumann proposed a shot, I'd be happy to assist.

[bodograumann]

I found an issue with the models.
When a model contains a file property, that property is correctly typed as HttpFile.
The problem is that the generated model file does not import HttpFile.

I tried to look through the code, but could not definitely understand yet, where and how best to fix this problem. Do you have a suggestion @TiFu?
We probably need to change the model.mustache file and define file paths in toTsImports.
Also it seems like HttpFile needs to be listed in reservedWords, should it not?

[TiFu]

Ah sorry! I'll have a look tomorrow and see how I can fix it. I wanted to start working on this again anyway, now that things have settled a bit for me.

[bodograumann]

The easiest solution would be to simply include the HttpFile type in all model declaration files.
What do you think about that, @TiFu ?

[bodograumann]

I noticed that we are exporting an object of all the models, an object of all the api modules, etc.
Was this a conscious decision, @TiFu?
As far as I have seen, all existing typescript-* generators export everything on the top level. So you can just

import { MyModel } from "my-api-client";
const myValue: MyModel;

The way we currently do it, switching from one of the existing generators, means replacing those imports with

import { models } from "my-api-client";
const myValue: models.MyModel;

Apart from the changes needed to existing code, this would also make it less clear what is actually imported from the package.

So, do we want to switch to top-level exports, have both or make it configurable?

[bodograumann]

Good news: With a few changes to the generated code I was able to get one of my applications running with it! :fireworks:

The changes were needed because of the following minor issues, but should be quite easy to fix:

• At some places unclean model names are used. E.g. Error instead of ModelError.
• For serialization toString is possibly called on undefined.
• RequestContext.body must default to undefined, because "" is not the same and causes an error when issuing a GET request with the fetch api.
• The types from servers.ts are not exported.

Apart from that I guess binary downloads will not work, but that is to be expected, as the http client class is quite rudimentary.

[TiFu]

The easiest solution would be to simply include the HttpFile type in all model declaration files.
What do you think about that, @TiFu ?

Agreed.

So, do we want to switch to top-level exports, have both or make it configurable?

Yeah that sounds good. I just didn't fix that yet.

Good news: With a few changes to the generated code I was able to get one of my applications running with it! fireworks

Oh that's great! :)

[bodograumann]

So, do we want to switch to top-level exports, have both or make it configurable?

Yeah that sounds good. I just didn't fix that yet.

Uhm, sorry to bother you, but which one do you think is best, @TiFu ?

[TiFu]

No worries, could've been far clearer in my last reply.

From a quick google search, it seems like there is no way of doing something like import api.* from 'ts-generator'. So using nested modules would make using the library a nightmare.

So, top-level exports?

[bodograumann]

In my experience top-level exports have been fine and all the other generators I have looked at are using them. So let's do that.
If someone really needs different exports, they can always shadow the index.mustache with their own version.

[bodograumann]

Currently we throw an exception in RequestContext.setBody when the request method is GET.

Adding bodies to GET requests has been discussed before:

• https://stackoverflow.com/a/16339271
• https://github.com/whatwg/fetch/issues/44
• https://github.com/whatwg/fetch/issues/83
• https://github.com/httpwg/http-core/issues/202

Setting a body on a HTTP GET, HEAD, DELETE or CONNECT request is discouraged and sending a body on a TRACE request is forbidden under the current HTTP semantics draft.

In the browser context, the fetch standard forbids bodies on GET and HEAD requests (step 34.) and does not allow CONNECT and TRACE requests at all.

So in most cases the browser itself will already block the request and our check would be redundant. Other http clients might be more lenient.

I think our generated code is not the place to police, which methods can take a request body and which cannot.
Showing a warning during code generation or even validation of the spec would be a good idea though.

[bodograumann]

I have a bit of a problem with binary data in responses.
Currently this is not supported. The http libraries (isomorphic-fetch and jquery) always parse the response body as UTF-8 data and thus make a mess of it, when it is actually binary.
Also the ResponseProcessor currently decodes everything as json, so that needs to be changed as well, when a method returns binary data (as HttpFile). I started working on this in #6177.

My main problem now is that the choice, whether to get binary or text data, can depend on the response code. E.g. an api might define a success response with binary data and an error response with json. That means we cannot know how to decode the data when creating the request.

With the fetch api this is pretty clear cut. It returns a Body object which has asynchronous text() and blob() functions which resolve to the string or binary content.
With XHR, on which jquery, axios and others are based, this is not easily possible. There the responseType needs to be set before sending the request and it determines what kind of data we get back.

So in order to be able to support binary response bodies with XHR http libraries, we would need to always set responseType to either blob or arrayBuffer and then, if we actually need string data, manually convert it. For ArrayBuffer there is TextDecoder.decode, but it is not available in internet explorer. (What is our stance on browser support and polyfills anyway?) For Blob there is Blob.text(), but there the browser support is even more spotty. So we would probably need to use FileReader.
Do we want that?

[TiFu]

Thanks for the detailed explanations!

Regarding your first comment:
My take-away is that throw Exception should be removed in RequestContext?
Additionally, we should think about adding a check in whatever part of OAI Gen checks the spec for validity?

Regarding your second comment:
From my own research on this, it seems like TextDecoder.decode is the best solution - particularly if we also want to focus on performance. To support IE, we could check if TextDecoder exists in window and if not fallback to using Filereader. Does that sound reasonable to you?

[TiFu]

Do you want to merge #6177 before we file the PR to merge the generator into master?

[bodograumann]

Yes, agreed on the GET-body check. I have already removed it in my unpushed work for #6177.
Adding a warning to spec validation has a lower priority for me than getting this new generator ready. So if anybody wants to have a look at it; maybe create a separate ticket for it(?); please go ahead.

---

As for XHR binary response bodies: Your suggestion seems fine for the technical implementation.
What I am worried about is blowing up the generated code for everybody unnecessary with all these polyfills.
Personally I am not using XHR and feel that it is made mostly obsolete by the fetch api, but with jquery, axios, angular?, etc. there are obviously still a lot of users out there. That is why I asked whether we actually need binary response bodies in that case?
Afaics none of the current generators typescript-jquery, typescript-angular and typescript-axios seem to support it. typescript-rxjs has an implementation, but chooses binary vs. text at the request creation time.

(Note for implementation: Use CodegenResponse.isFile, CodegenResponse.isBinary and maybe even these attributes on CodegenOperation.)

So we could:

1. Not support binary responses for XHR at all.
2. Support binary responses in an operation based way. So either all responses are binary or none are. That way we can set the responseType before sending the request and can use the build-in utf-8 decoding for text data.
3. Fully support binary responses. That means always setting responseType = "arrayBuffer" and then doing any decoding ourselves with polyfills for older browsers, which adds some amount of additional code.

It is also possible to add generator config options to select between these variants or to select whether the polyfills are needed, which would allow to better tailor the generated code to the users requirements, but would also make our mustach templates more complicated.

I'm tempted to go with option 1 for now and later implement option 3, to reduce work and code overhead, but I also know that “later” will probably never happen then.
I feel like having the binary support could be a factor for adoption of our new generator.
So I guess, even though none of them are perfect, option 3 would be the best approach imho.

What do you think?

---

Feel free to merge into master before we finish #6177. I can easily change the PR target to master and there is already a change in master I would like to use.

[TiFu]

So if anybody wants to have a look at it; maybe create a separate ticket for it(?); please go ahead.

Yeah sounds good.

Feel free to merge into master before we finish #6177. I can easily change the PR target to master and there is already a change in master I would like to use.

Alright. I'll file the PR and CC you as well as the technical committee and whoever else we need.

What do you think?

Where would we need polyfills?

My understanding is that most up-to-date browsers support TextDecoder.decode out of the box. The only culprit is IE which we could support by automatically falling back to FileReader without any polyfill.

In node, we could use string_decoder instead of TextDecoder.decode and FileReader.

[bodograumann]

By polyfill I meant the fallback. Replacing missing functionality for some platforms with an alternative implementation.

Node seems fine to me, I think Buffer can natively return UTF-8 decoded strings.

[TiFu]

But as long as that alternative implementation is supported by default, this wouldn't increase the code size because we don't have to include any additional dependency. Am I missing something?

[bodograumann]

Maybe I'm just overthinking this and adding a few lines (calling FileReader) which are only used in IE is not such a big deal ^^
Btw. latest browsers support Blob.text() so then we might not even need to call TextDecoder.

[TiFu]

Yeah, I think adding a couple lines for that should be fine - probably doesn't even make any noticeable difference after minifying the code. We could support disabling this and just not supporting IE (i.e. a flag) but to be honest I doubt anyone would use that because the difference is so small.

[bodograumann]

Alright then let's go for that. Should not even be so hard to implement. I'll see what I can build tomorrow.

[bodograumann]

Can you tell me, what the use case for NoAuthentication is @TiFu ?

[TiFu]

The original plan was to use it as the auth provider instead of null or undefined - however it looks like the code actually checks null anyway so it's completely useless as far as I can tell.

I think I'll just remove it.

[mocsy]

This reminds me of this: Standards

[bodograumann]

That is an apt observation, @mocsy.
Neverless I hope, that by being better maintained and providing more features, we can convince many people to switch.

[Bessonov]

@bodograumann the gain is huge to have a common implementation and small specific implementations. It's just a matter of time when everyone _want_ to switch to the shiny new version of generator(s). We have some bugs with current implementation (for example nulls for nullable fields are treated like undefined) and just can't await to get this rewrite done to contribute.

[TiFu]

@mocsy Constructive feedback is always appreciated ;-)

@Bessonov :-) We are trying to get this merged as soon as possible - see #6341 . @bodograumann is also working on adding inversify support (#6489).

Are you missing any specific features in the rewrite? Happy to have a look at implementing whatever's missing or guiding you in the right direction to contributing to the rewrite :)

[hypdeb]

typescript-angular used to convert string with date-time format to Date in typescript. Not working anymore.
Is there a workaround or should I go back to a version that had this working ?

[macjohnny]

@hypdeb see https://github.com/OpenAPITools/openapi-generator/pull/5314#issuecomment-586979702

[hleb-albau]

Also there is issue with json. typescript-axios generator for objects with string with format=binary produces value:any; for format=byte - value:str. We have Blob and ArrayBuffer in js to make it work like in java, where api classes handles base64 and binary conversions behind scene. Right now users have to deal with this conversions explicitly around clients.