swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
17.09k stars 6.03k forks source link

Improve TypeScript Template #1334

Closed aersam closed 10 months ago

aersam commented 9 years ago

The templates in https://github.com/swagger-api/swagger-codegen/tree/master/modules/swagger-codegen/src/main/resources/TypeScript-Angular should be updated to use TS 1.6 best practices:

I'd make a pull request if desired.

wing328 commented 9 years ago

@aersamkull thanks for offering contribution to the TypeScript generator. Would you please elaborate on your third point about using interfaces instead of models?

For best practices, I assume you're referring to http://definitelytyped.org/guides/best-practices.html.

Later we want to support inheritance in all generators: https://github.com/swagger-api/swagger-spec/blob/master/versions/2.0.md#composition-and-inheritance-polymorphism

aersam commented 9 years ago

I just looked at Rules from Tslint: https://github.com/palantir/tslint The Best practices of DefinitelyTyped do just include things for declaring interfaces, not more. let and const are supported in TypeScript and are transpiled to var. There is no need for var anymore in TypeScript. var is function-scoped (evil), let is block-scoped (good)

The namespace keyword is a replacement for the module keyword: https://github.com/Microsoft/TypeScript/pull/2923 module is no more encouraged

On classes: An api returns some JSON which consists of arrays and object, but never of functions. Classes in TypeScript generate functions. We expect the instanceof Keyword to work with a class. So I just think it is more idiomatic to use simplier interfaces over Classes.

interface a_i { b: string};
class a_c {b: string;}
let v1 : a_i = {"b": "asdf"}; //Works
let v2: a_c = {"b": "asdf"}; //Does not Work

http://www.typescriptlang.org/Playground#src=interface%20a_i%20%7B%20b%3A%20string%7D%3B%0Aclass%20a_c%20%7Bb%3A%20string%3B%7D%0Alet%20v1%20%3A%20a_i%20%3D%20%7B%22b%22%3A%20%22asdf%22%7D%3B%20%2F%2FWorks%0Alet%20v2%3A%20a_c%20%3D%20%7B%22b%22%3A%20%22asdf%22%7D%3B%20%2F%2FDoes%20not%20Work%0A%0Ainterface%20i_c%20extends%20a_i%20%7B%20c%3A%20string%20%7D%3B%0A%0Alet%20v3%20%3A%20i_c%20%3D%20%7B%22b%22%3A%22as%22%2C%20%22c%22%3A%20%22asdasdfasd%22%7D%3B

wing328 commented 9 years ago

Copying @mhardorf (creator of the TypeScript generator) and @tandrup (core contributor to TypeScript generator) to see if they've any comments on the proposed change.

agoncal commented 7 years ago

Hi @aersamkull @wing328, I would like to come back to this issue.

At the moment the Angular Codegen template generate interfaces for the model. To be able to use interfaces instead of classes, you need to use the Elvis operator in the Angular HTML files.

Let's say we have a Book interface :

export interface Book {
    title: string;
    description?: string;
}

We have to make sure book is not null using the Elvis operator so we can display the values :

    <input class="form-control" type="text" [value]="book?.title" readonly>
    <textarea class="form-control" rows="3" readonly>{{book?.description}}</textarea>

But for forms, this is not possible. Elvis operator is not possible in two-way binding. But using class, is fine.

So when Codegen generates interfaces, I always change them to classes. If you look at the official Angular documentation, they use classes for the model, not interfaces. And in my understanding, Codegen used to use classes before.

Are you sure about interfaces ? If you want to keep interfaces, what about generating at least one implementation (Book) for the interface (IBook) ?

wing328 commented 7 years ago

@agoncal may I know if you've time to contribute a PR to use class instead of interface? If yes, I can show you some good starting point.

Please base the fix on 2.3.0 branch, which contains breaking changes.

agoncal commented 7 years ago

@wing328 I'm ok to contribute.... but I'm totally new in contributing to Swagger Codegen. So I wouldn't mind a bit of help with some starting points.

wing328 commented 7 years ago

@agoncal I think a good starting point is https://github.com/swagger-api/swagger-codegen/blob/2.3.0/modules/swagger-codegen/src/main/resources/typescript-angular/model.mustache#L11 (ts-angular model template in 2.3.0 branch)

After you modify it, you can run the following commands to regenerate the Petstore sample.

mvn clean package
./bin/typescript-angular-petstore.sh
wing328 commented 7 years ago

@agoncal may I know if you need help with the PR/enhancement?

agoncal commented 7 years ago

I'm sorry @wing328 I won't have time to work on this for a few weeks. Hopefully I'll be available from mid-may. It's in my TODO list ;o)

wing328 commented 7 years ago

@agoncal no problem. Let us know if you need any help with the PR/enhancement.

iain17 commented 7 years ago

I'm using this generation in one of my projects and my app won't compile due to a few simple typescript errors generated by the code generator. I'm looking into it right now.

@mhardorf and @tandrup could one of you guys perhaps explain why the generated models are interfaces? Would it not make more sense to make those classes? I'm up for changing to classes because in my experience an interface doesn't contain attributes. But maybe there is something I'm not seeing right now.

Repo so far: https://github.com/iain17/swagger-codegen/tree/improvement/typescript-angular2 PR coming soonish. I'm going to use it first for a bit and fix problems that arise.

kenisteward commented 7 years ago

What are your compilation errors you are getting due to interfaces? You have to remember typescript compiles to js which has no clue about interfaces which means interfaces are a compile time thing. In that case interfaces allow you to create objects with type safety without needing a new operator. See above for example.

Please provide the issue of why you get errors due to interfaces

iain17 commented 7 years ago

Replied in the PR. Thanks though for replying :)