-
Notifications
You must be signed in to change notification settings - Fork 6k
[TS][Angular] Updating typescript-angular to export api classes #4589
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
Conversation
@fzakaria this is the discussed change for angularjs exports @wing328 there is some duplication between the typescript-angular and typescript-angular2 generators now, which could be cleaned up in another PR if this pattern was wanting to be applied to all typescript generators. In that case, this functionality would move into |
@damienpontifex thanks for the PR but the CI (travis) reports the following issue:
Ref: https://s3.amazonaws.com/archive.travis-ci.org/jobs/192884966/log.txt Please take a look when you've time. |
Thanks @wing328 small update required to the sample client tsconfig source files. Now passing. |
👍 I definitely prefer smaller changes for easier review. @Vrolijkx can you please review this when you've time? |
@@ -0,0 +1,9 @@ | |||
import * as api from './api/api'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason why you do the * as api
import instead of import {PetApi, StoreApi,...} from './api/api';
We also made this choice at first in angular2 API's but due to problems we needed to change this later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this as the angular2 client was doing the similar thing such as https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/typescript-angular2/api.mustache#L10
Open to suggestion to change or leave as is.
An explicit import could be made. Out of interest, what were the problems you ran into?
* Do not edit the class manually. | ||
*/ | ||
|
||
import * as models from '../model/models'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idem
Look great aligns the angular1 code more with the angular2 code. Maybe in the future, we can have some common code like the models. They are roughly the same between angular1 and angular 2 ;) |
@Vrolijkx next PR from me would be a move to reducing duplicate code/templates for all typescript generators (maybe just for angular, but potentially for typescript-node also). Thanks for the review and feedback :) Do you agree that this generated structure could also be applied to the typescript-node generator in the future? |
Certainly, agree they al should use the same models and structure. The only diff between them should be how the API is generated (fetch/Http angular2[observables]/Http angular1[promisses]/node) and their |
Is it possible that we could get this merged? I think i need to start from here for #2694 |
PR merged into master. Please pull the latest master to give it a try. |
Thanks! |
looks great. |
* Updating typescript-angular to export api classes * Fixing tsconfig for typescript-angular test case
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)2.3.0
branch for breaking (non-backward compatible) changes.Description of the PR
Exporting generated api classes with typescript-angular generator. This removes the namespace wrapping as discussed in #1487