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

TypeScript and Packages #1487

Closed
aersam opened this issue Nov 2, 2015 · 27 comments · May be fixed by qsays/swagger-codegen#942
Closed

TypeScript and Packages #1487

aersam opened this issue Nov 2, 2015 · 27 comments · May be fixed by qsays/swagger-codegen#942

Comments

@aersam
Copy link
Contributor

aersam commented Nov 2, 2015

If I use a config file for TypeScript-Angular and I set modelPackage and apiPackage to different values, I get an error. The generated api File does no longer find the models. Maybe we should use absolut Types? Eg, services.Bla instead of just Bla?

@wing328
Copy link
Contributor

wing328 commented Nov 12, 2015

@aersamkull Yes, I think that's one way to do it.

Another approach (a bit more difficult) is to import the models in the Api files, e.g. Java/api.mustache#L9

@aersam
Copy link
Contributor Author

aersam commented Nov 12, 2015

I think the import approach is cleaner. However the syntax is different from Java. Is there a way to add Mustache Variables for a specific language? Or to modify them?
A third approach would be Type Aliases:

type Person = ModelNamespace.Person;

@wing328
Copy link
Contributor

wing328 commented Nov 12, 2015

Using the Petstore example, can you please show me what the import statement looks like?

@aersam
Copy link
Contributor Author

aersam commented Nov 12, 2015

Sorry, import statements are not an option. They are bound to modules and we should not rely on a module system, I think. Type Aliases could be used like imports in Java:

//PetAPI.ts
namespace API.Access {
    'use strict';
    type Pet = API.Model.Pet;

@aersam
Copy link
Contributor Author

aersam commented Nov 12, 2015

Somehow, api.d.ts is always location in API/Client folder and has wrong paths. This had to be fixed as well. I would expect it to be in the "apiPackage"-Folder

@wing328
Copy link
Contributor

wing328 commented Nov 13, 2015

Also we need to apply the same fix to typescript-node as well but I'm not sure which one (type aliases or fully-qualified name) is preferable in typescript-node.

@aersam
Copy link
Contributor Author

aersam commented Nov 13, 2015

I personally prefer fully qualified names. Even though it is not very likely, we could produce name collisions with aliases.
In addition, we can write unit tests for full qualified names, isnt' it?

@wing328
Copy link
Contributor

wing328 commented Nov 13, 2015

Yes, unit testing is something we need for Typescript Petstore client. I'll create a separate task for tracking that.

wing328 added a commit that referenced this issue Feb 15, 2016
Fix #1487, TypeScript-Angular output path wrong if apiPackage not the…
@wing328
Copy link
Contributor

wing328 commented Feb 15, 2016

@aersamkull #1487 merged into master. Please pull the latest and let us know if you still encounter the issue.

Thanks @daveholladay for the fix.

@pixelshaded
Copy link
Contributor

pixelshaded commented Apr 18, 2016

Still an issue. Execute this command in master:

java -jar "./modules/swagger-codegen-cli/target/swagger-codegen-cli.jar" generate -i modules/swagger-codegen/src/test/resources/2_0/petstore.json -l typescript-angular -o samples/client/petstore/typescript-angular --additional-properties modelPackage=api.clients.petstore.models

And then run npm install / test in the typescript-angular samples (you should probably delete the api folder before running the above command).

error TS6053: File 'API/Client/Category.ts' not found.
error TS6053: File 'API/Client/Order.ts' not found.
error TS6053: File 'API/Client/Pet.ts' not found.
error TS6053: File 'API/Client/Tag.ts' not found.
error TS6053: File 'API/Client/User.ts' not found.

The output references don't look in the subfolders when using namespaces.

The api.d.mustache is using classname to render which may not take in to account namespace.

{{#models}}
{{#model}}
/// <reference path="{{{classname}}}.ts" />
{{/model}}
{{/models}}

{{#apiInfo}}
{{#apis}}
{{#operations}}
/// <reference path="{{classname}}.ts" />
{{/operations}}
{{/apis}}
{{/apiInfo}}

Generates

/// <reference path="Category.ts" />
/// <reference path="Order.ts" />
/// <reference path="Pet.ts" />
/// <reference path="Tag.ts" />
/// <reference path="User.ts" />

/// <reference path="PetApi.ts" />
/// <reference path="StoreApi.ts" />
/// <reference path="UserApi.ts" />

@wing328 wing328 reopened this Apr 18, 2016
@wing328 wing328 added this to the v2.2.0 milestone Apr 18, 2016
@wing328
Copy link
Contributor

wing328 commented Apr 18, 2016

@pixelshaded so the following will work?

/// <reference path="API/Client/Category.ts" />
/// <reference path="API/Client/Order.ts" />
/// <reference path="API/Client/Pet.ts" />
/// <reference path="API/Client/Tag.ts" />
/// <reference path="API/Client/User.ts" />

/// <reference path="API/Client/PetApi.ts" />
/// <reference path="API/Client/StoreApi.ts" />
/// <reference path="API/Client/UserApi.ts" />

@pixelshaded
Copy link
Contributor

No. The models exist in a completely different folder from the api if you define a model package (which is ideal since folder structure should match namespacing).

They are in api/clients/petstore/models when passing modelPackage=api.clients.petstore.models

@wing328
Copy link
Contributor

wing328 commented Apr 18, 2016

so the following should work, right?

/// <reference path="api/clients/petstore/models/Category.ts" />
/// <reference path="api/clients/petstore/models/Order.ts" />
/// <reference path="api/clients/petstore/models/Pet.ts" />
/// <reference path="api/clients/petstore/models/Tag.ts" />
/// <reference path="api/clients/petstore/models/User.ts" />

/// <reference path="api/clients/petstore/models/PetApi.ts" />
/// <reference path="api/clients/petstore/models/StoreApi.ts" />
/// <reference path="api/clients/petstore/models/UserApi.ts" />

@pixelshaded
Copy link
Contributor

pixelshaded commented Apr 18, 2016

this depends. The reference path is the relative path of the file from the api.d.ts. That depends on the apiPackage if one is defined. Based on the command I gave, this would be valid:

/// <reference path="../clients/petstore/models/Category.ts" />
/// <reference path="../clients/petstore/models/Order.ts" />
/// <reference path="../clients/petstore/models/Pet.ts" />
/// <reference path="../clients/petstore/models/Tag.ts" />
/// <reference path="../clients/petstore/models/User.ts" />

/// <reference path="PetApi.ts" />
/// <reference path="StoreApi.ts" />
/// <reference path="UserApi.ts" />

But using this command

java -jar "./modules/swagger-codegen-cli/target/swagger-codegen-cli.jar" generate -i modules/swagger-codegen/src/test/resources/2_0/petstore.json -l typescript-angular -o samples/client/petstore/typescript-angular --additional-properties modelPackage=api.clients.petstore.models,apiPackage=api.clients.petstore

api.d.ts would need this

/// <reference path="models/Category.ts" />
/// <reference path="models/Order.ts" />
/// <reference path="models/Pet.ts" />
/// <reference path="models/Tag.ts" />
/// <reference path="models/User.ts" />

/// <reference path="PetApi.ts" />
/// <reference path="StoreApi.ts" />
/// <reference path="UserApi.ts" />

The Api classes themselves are always right next to the api.d.ts for the typescript-angular generator, so they won't need anything to be valid.

@pixelshaded
Copy link
Contributor

Also, the models files include a reference path to the api.d.ts, but they also expect api.d.ts to be in the same folder. Just seems like namespaces that deviate is just not supported atm.

@pixelshaded
Copy link
Contributor

Whats worse is that the api implementations don't use the fully qualified name of the type either (aka namespace is not included), so even if we resolved the reference issues, we'd still have challenges when trying to compile.

@wing328 wing328 modified the milestones: v2.3.0, v2.2.0 Jul 7, 2016
@wing328 wing328 modified the milestones: v2.2.1, v2.2.2 Aug 8, 2016
@damienpontifex
Copy link
Contributor

I was generating an angular-typescript client and just wondering why are we namespacing the TypeScript classes? The TypeScript docs even has a section against Needless Namespacing.

It also creates very verbose and maybe difficulties when just doing a simple import {} from './ClientApi'. @wing328 was there a specific reason/PR for the name spacing? And could we move all TypeScript classes to follow the pattern such as the angular2 client that exports the class and then the apis.mustache file exports all the classes from there?

@fzakaria
Copy link

fzakaria commented Jan 8, 2017

@damienpontifex i'm not even sure how to import the generated client.
The namespace that it wraps with is not exported, so its not a module AFAIK

@damienpontifex
Copy link
Contributor

@fzakaria I have run into the same issue. I posted my comment to gather information about providing a potential change/PR to either:

  1. Export the namespace or
  2. Remove the namespacing and export the class as per usual

I agree with the 'Needless Namespacing' article and generally just export my classes so would be in favour of that route.

@fzakaria
Copy link

fzakaria commented Jan 9, 2017

@damienpontifex i tried to do a PR but couldn't figure out the mustache for getting it to work.
(I needed the generic type for ReturnTypes when they are in a container like Array to import them)
I am +1 on removing the namespace and just exporting the class/interface.

@damienpontifex
Copy link
Contributor

The angular1 client needs work in the Java generator code to be able to do that. See comparison of TypeScriptAngular2ClientCodegen.java vs TypeScriptAngularClientCodegen.java in which we are able to do this in angular2

Basically more context needs to be passed into the generator to map the model. prefix for custom types. I had plans to provide a PR to also update the angular1 client to do this, but haven't quite got to it yet. Or more favourably to pull that logic into the AbstractTypeScriptClientCodegen so that all TypeScript generators get this benefit.

@fzakaria
Copy link

fzakaria commented Jan 9, 2017 via email

@fzakaria
Copy link

@damienpontifex did you get a chance to make some changes?

@damienpontifex
Copy link
Contributor

@fzakaria not yet. Just finishing off #4409 and then would look at using those updated templates for this. Hopefully get onto it in the next day or two

@damienpontifex
Copy link
Contributor

@fzakaria Made some initial changes here damienpontifex/swagger-codegen@53f02fc80d8c32f52b41adb1a760ad7b3b4d5bf3 and will work on the branch 'tsc-consolidate-patterns'

Still a bit of work to do, especially around the non-angular typescript generators, but angular2 has maintained all functionality and angular1 has gained the appropriate barrel modules for model and api as I believe is the desired outcome here (see the samples/client/petstore/typescript-angular for generated examples of this in the linked commit above)

@damienpontifex
Copy link
Contributor

@wing328 @fzakaria any opinions on closing this now that #4589 is merged into master?

@wing328
Copy link
Contributor

wing328 commented Jan 23, 2017

Thanks for the PR from @damienpontifex. Please pull the latest master to give it a try.

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

Successfully merging a pull request may close this issue.

5 participants