PR-1334 Improved angular 2 - typescript generation #5695
Conversation
…erfaces, but instead as classes. swagger-api#1334
…double. Updated test: enumArrayMdoelTest swagger-api#1334
…ons with non 204 (http status code) but empty http responses. swagger-api#1334
iain17
changed the title
|
@iain17 thanks for the PR. It looks to me it's a breaking change so it should be filed against 2.3.0 branch instead. Also you may want to checkout 2.3.0 to generate TS Angular2 client, which has a lot of enhancements and bug fixes. |
|
@Vrolijkx @taxpon @TiFu @kenisteward , please take a look when you've time. Thanks! |
| return response.json() || {}; | ||
| }catch(e){ |
There was a problem hiding this comment.
There should be a space between } and catch.
| @@ -1,9 +1,9 @@ | |||
| import { {{{injectionToken}}} } from '@angular/core'; | |||
|
|
|||
| export const BASE_PATH = new {{{injectionToken}}}('basePath'); | |||
| export const BASE_PATH = new {{{injectionToken}}}<String>('basePath'); | |||
There was a problem hiding this comment.
Can you elaborate on this change? Shouldn't it be string like before?
There was a problem hiding this comment.
Please see: https://angular.io/docs/ts/latest/api/core/index/InjectionToken-class.html
I changed it because my app just wouldn't compile otherwise.
There was a problem hiding this comment.
That's weird. Can you post a minimum working example which doesn't compile?
string is the TypeScript string type and String is the default JavaScript string type, which is why I am wondering if the underlying issue is something else.
See e.g. here
| * | ||
| * OpenAPI spec version: 1.0.0 | ||
| * Contact: apiteam@swagger.io | ||
| * Contact: apiteam@wordnik.com |
There was a problem hiding this comment.
This change was probably not intended. Could you please revert it?
There was a problem hiding this comment.
The latest master is using petstore.yaml instead of petstore.json (which still references wordnik.com) so rebasing the PR on the latest master should avoid the above change.
|
Hello @iain17 please look at my comment for #1334 and provide an explanation of why your code doesn't compile using the latest changes. I have had no issues using interfaces with my application so I worry you will be making everyone rewrite code when they upgrade for a reason that Is lovalized and not broad. |
|
@wing328 Thanks for your comment. I'll look into it and apply the necessary changes. |
|
@kenisteward I don't specifically get errors due to interfaces, there were other issues why it didn't compile unrelated to the interface change. The main issue we had with using interfaces was the fact that we couldn't create a new instance of a model. The other was that in most languages it is bad practice to use interfaces to describe data types. Usually, you have structs or classes that do that job. However, I do understand your point and reading more into this, I see that it's common practice in Typescript to use interfaces to describe data types (see for instance: https://www.typescriptlang.org/docs/handbook/advanced-types.html#type-aliases). I'll make the necessary changes and try and apply them to the current project we are doing. See how it works out for us. Right now, it feels more natural to us to say: |
|
It's much more succinct to just do
Const u : IUser = {
}
As a is developer I can tell you more and more you get used to not using
new cause honestly it mostly isn't needed unless you want to actually
define custom logic or functions
…On Wed, May 24, 2017, 8:09 PM Iain Munro ***@***.***> wrote:
@kenisteward <https://github.com/kenisteward> The main issue I had with
using interfaces was the fact that I couldn't create an new instance of
that model. The other was that in most languages it is bad practice to use
interfaces to describe data types. Usually, you have structs or classes
that do that job.
However, I understand your point and reading more into this, I see that
it's common practice in Typescript to use interfaces to describe data types
(see for instance:
https://www.typescriptlang.org/docs/handbook/advanced-types.html#type-aliases
).
I'll make the necessary changes and try and apply them to the current
project I'm doing, see how it works out for me. Right now it feels more
natural to me to say: new User(); which cannot be done with an interface.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5695 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMPLtQELkbS8WV3GCEma5wWfHF4orQa5ks5r9MatgaJpZM4NkRpI>
.
|
That's the thing, we sometimes do need to define different behaviours and alternative implementations based on inheritance. To do that in the right way, I believe there should be classes and sometimes both the classes and interfaces needed, whether it creates some upgrading work for some or not. By the way, I recently figured out using I was actually considering to make this change and file a PR, as it looks like I'm a bit late. @wing328 you marked the milestone to @kenisteward Should the new model classes work consistently with the new |
|
If you want to make classes just have them implement the generated ones.
You'll get the properties that are defined in the interface ( that's a TS
thing ) with no need to change the generation model
…On Sun, May 28, 2017, 2:00 AM kirpit ***@***.***> wrote:
honestly it mostly isn't needed unless you want to actually define custom
logic or functions
That's the thing, we sometimes do need to define different behaviours and
alternative implementations based on inheritance. To do that in the right
way, I believe there should be classes and sometimes both the classes and
interfaces needed, whether it creates some upgrading work for some or not.
I was actually considering to make this change and file a PR, as it looks
like I'm a bit late.
@wing328 <https://github.com/wing328> you marked the milestone to v2.2.3
but you also mentioned this should go against v2.3.0. Is it intentional?
@kenisteward <https://github.com/kenisteward> Should the new model
classes work consistently with the new withInterfaces parameter I
introduced recently here in #5519
<#5519>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5695 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMPLtXcy0Qbi41iJ8hHf00Cz6xyuhcHBks5r-Q13gaJpZM4NkRpI>
.
|
|
@wing328 I too would like to vote against this change. I don't see any reason to move from interfaces to classes, and as @kenisteward points out
@Vrolijkx what's your opinion on this?
Would you care to elaborate on this @iain17 ? While I don't think this is necessarily true for any of the programming languages/OOP paradigms I know, I sincerely have my doubts on this when it comes to Typescript. An Interface in Typescript will just disappear after compiling, why throw that away? Also, you don't even have to have a class implement an Interface to create an object of that type.
I feel like this is not true. Whats wrong with this code? interface A {
abc: string;
}
let test: A = {
abc: "def"
}; |
|
@iain17 when you've time, I wonder if you can file another PR to just do the following:
Please pull the latest master as there're some enhancements to TS generators (and see if the above issues have been addressed) |
A bunch of fixes for the angular 2 typescript client generation. I had to make these changes to make use of this project. Large problems fixed, such as making it typescript compliant (my app just refused to compile without these fixes). I've updated the tests as well :)
Other problems that have now been fixed:
Issue #1334 .