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)

• 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
• 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 #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.

[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 #5314 (comment)

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

[forresthopkinsa]

IMO it's critical that the generalized Typescript generator has a lot more explanation in its readme pertaining to what it is and whether or not people should use it.

It's pretty confusing seeing "Typescript" and "Typescript-Fetch" right next to each other, with only this to distinguish them:

	typescript	typescript-fetch
stability	experimental	stable
type	client	client
language	typescript	typescript
templating	mustache	mustache
helpTxt	Generates a TypeScript client library using Fetch API (beta).	Generates a TypeScript client library using Fetch API (beta).

[aeropagz]

Hello,
Is there an option to only generate the models of an API. I am working on a nuxt project and only the the type/interfaces of the responses without any runtime or api code.