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

Improve TypeScript Template #1334

Closed
aersam opened this issue Oct 5, 2015 · 13 comments
Closed

Improve TypeScript Template #1334

aersam opened this issue Oct 5, 2015 · 13 comments

Comments

@aersam
Copy link
Contributor

aersam commented Oct 5, 2015

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:

  • No more use var keyword. Use let and cost instead (TsLint Config Value: no-var-keyword)
  • Use namespace keyword instead of module keyword (TsLint Config Value: no-internal-module)
  • Why are classes used for models? I think interfaces would be better

I'd make a pull request if desired.

@wing328
Copy link
Contributor

wing328 commented Oct 5, 2015

@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
Copy link
Contributor Author

aersam commented Oct 5, 2015

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: microsoft/TypeScript#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
Copy link
Contributor

wing328 commented Oct 5, 2015

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
Copy link

agoncal commented Mar 26, 2017

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
Copy link
Contributor

wing328 commented Mar 26, 2017

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

@wing328 wing328 reopened this Mar 26, 2017
@wing328 wing328 added this to the Future milestone Mar 26, 2017
@agoncal
Copy link

agoncal commented Mar 27, 2017

@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
Copy link
Contributor

wing328 commented Mar 27, 2017

@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
Copy link
Contributor

wing328 commented Apr 19, 2017

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

@agoncal
Copy link

agoncal commented Apr 20, 2017

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
Copy link
Contributor

wing328 commented Apr 20, 2017

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

@iain17
Copy link

iain17 commented May 14, 2017

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.

iain17 added a commit to iain17/swagger-codegen that referenced this issue May 14, 2017
iain17 added a commit to iain17/swagger-codegen that referenced this issue May 14, 2017
iain17 added a commit to iain17/swagger-codegen that referenced this issue May 14, 2017
iain17 added a commit to iain17/swagger-codegen that referenced this issue May 14, 2017
iain17 added a commit to iain17/swagger-codegen that referenced this issue May 15, 2017
iain17 added a commit to iain17/swagger-codegen that referenced this issue May 15, 2017
iain17 added a commit to iain17/swagger-codegen that referenced this issue May 17, 2017
…ons with non 204 (http status code) but empty http responses. swagger-api#1334
@kenisteward
Copy link
Contributor

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
Copy link

iain17 commented May 25, 2017

Replied in the PR. Thanks though for replying :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants