Restructure TypeScript Node generation into separate files (PHNX-1041) #363

gbrown-ce · 2018-06-20T20:49:02Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./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). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

The current version of TypeScript node generation puts all of the generated code in a single file. This causes issues when using inheritance as each model may depend on another and they must be included in the proper order.

Split the mustache template into pieces so that it can properly separate the models and different apis to allow for proper compilation. I also removed some compiled JS files from the petstore example generation that shouldn't have been in the repository.

Related issue: #350

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny

macjohnny · 2018-06-21T06:09:09Z

@gbrown-ce thanks for the PR! will have a look at it soon

gbrown-ce · 2018-06-21T12:32:15Z

@macjohnny thanks! And don't be afraid of the ~50 files changed. Only the first 7 are really changes. The rest are just generated samples.

gbrown-ce · 2018-06-21T14:01:38Z

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny I believe the necessary CI step has passed here and this should be ready for review/merge.

macjohnny

although I haven't tested it, the changes look good. I think this is a nice improvement.
@gbrown-ce can you confirm that you generated your project with this code and everything worked correctly?

macjohnny · 2018-06-22T06:38:37Z

...-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptNodeClientCodegen.java

+
+    @Override
+    public String getSchemaType(Schema p) {
+        String openAPIType = super.getSchemaType(p);


could you please explain in the comments why these type mapping methods are needed / what they do?

@macjohnny I believe the idea for this is to allow the "Buffer" type to be used properly. Initially when I did this, it tried to import it as ./models/Buffer which is wrong. There is similar work done in the angular client for the "Blob" type.

macjohnny · 2018-06-22T06:40:01Z

...-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptNodeClientCodegen.java

+        return languageSpecificPrimitives.contains(type);
+    }
+
+    private boolean isLanguageGenericType(String type) {


could you give an example input in the method docs?
I guess you expect something like Array<string>, right?

@macjohnny I'm not sure of the exact purpose of this one, again, it is taken from the angular generator. But yes, I'd assume you're correct

macjohnny · 2018-06-22T06:41:14Z

...-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptNodeClientCodegen.java


    private String getPackageRootDirectory() {
        String indexPackage = modelPackage.substring(0, Math.max(0, modelPackage.lastIndexOf('.')));
        return indexPackage.replace('.', File.separatorChar);
    }
+
+    private String getApiFilenameFromClassname(String classname) {
+        String name = classname.substring(0, classname.length() - "Api".length());


how about setting the "Api" suffix as a member property, which later could possibly be customized with a configuration option?

Sure, I can move that

macjohnny · 2018-06-22T06:47:01Z

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx
I would be happy to get a second opinion

gbrown-ce · 2018-06-22T11:21:28Z

@macjohnny yes I tested it with my own project and it works great.

gbrown-ce · 2018-06-22T12:16:22Z

@macjohnny I have updated the PR with your requested changes. Even went so far as to add the api suffix as a parameter. I also went through and ran all 5 of our swagger documents through it and the newly generated clients worked perfectly.

macjohnny · 2018-06-22T12:37:38Z

@gbrown-ce thanks for the changes, I think now it should be good to go.

gbrown-ce · 2018-06-22T20:56:22Z

@macjohnny I'm about to head on a week long vacation. Can I trust you to get this merged before I get back? That'd be a huge help.

macjohnny · 2018-06-22T20:58:24Z

@gbrown-ce This PR will eventually be merged by @wing382, I am not able to do that.

macjohnny · 2018-06-22T20:59:28Z

Anyway, you could also temporarily build from your branch and publish the artifact in your local repository

gbrown-ce · 2018-06-22T21:22:52Z

@macjohnny good to know. Can you take a look @wing328 ?

As far as local artifacts, yes I could do that (and have for testing), but we are integrating our Jenkins pipeline with the openapi-generator-cli docker image, so to be fully finished on my end, I need to final code to be in (or make my own docker image, which I'd rather avoid)

gbrown-ce · 2018-06-22T21:23:19Z

Oh and the failures in the Travis build seem to be related to a problem that already exists in master.

wing328 · 2018-06-25T07:34:15Z

Please ignore the failure related to rust-server for the time being as the failure is due to CI pulling a newer version of the dependency.

wing328 · 2018-06-25T07:34:55Z

For "/" in the file path, please replace it with File.separator so that the generator works with Windows platform as well.

wing328 · 2018-06-25T07:36:48Z

If I understand the PR correctly, this enhancement is a breaking change without fallback so the PR should target the 4.0.x branch instead.

macjohnny · 2018-06-25T07:39:09Z

@wing328 @gbrown-ce

For "/" in the file path, please replace it with File.separator so that the generator works with Windows platform as well.

This change should not be applied, since the imports in typescript files always use / as a file separator. This also works e.g. in the typescript-angular generator, see e.g.
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptAngularClientCodegen.java#L445

macjohnny · 2018-06-25T07:53:36Z

If I understand the PR correctly, this enhancement is a breaking change without fallback so the PR should target the 4.0.x branch instead.

this PR could be turned into a non-breaking change by simply re-exporting the files in the api.ts, where they were originally located:

https://github.com/OpenAPITools/openapi-generator/pull/363/files#diff-808fb691981e56904bccf3634e507ce2R11

@gbrown-ce would you like to apply this change?

gbrown-ce · 2018-07-02T15:57:59Z

@macjohnny Just got back from vacation and made the suggested changes. Just waiting for CI to run through.

gbrown-ce · 2018-07-02T17:31:38Z

@macjohnny @wing328 looks like CI is happy. Do the changes look good to you guys?

macjohnny · 2018-07-02T20:03:24Z

...-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptNodeClientCodegen.java

@@ -110,10 +211,12 @@ public void setNpmRepository(String npmRepository) {
    @Override
    public void processOpts() {
        super.processOpts();
-        supportingFiles.add(new SupportingFile("api.mustache", null, "api.ts"));
+        supportingFiles.add(new SupportingFile("models.mustache", modelPackage().replace('.', File.separatorChar), "models.ts"));
+        supportingFiles.add(new SupportingFile("apis.mustache", apiPackage().replace('.', File.separatorChar), "api.ts"));


isn't there a collision for the destination filename api.ts, since both apis.mustache and index.mustache are generated to api.ts?
anyway I would suggest to rename api.mustache to api-single.mustache and apis.mustache to api.mustache in order to avoid confusions.

No collision there, the apis.mustache goes into a subfolder, but yes, I can clean that up a bit.

@macjohnny

I'm going to go with this:

api-all.mustache = all of the apis re-exported together (output as /apis/apis.ts)
api-single.mustache = single api (output as /apis/{name}Api.ts)
api.mustache = entry point (output as /api.ts to prevent breaking change)

I think this should avoid any confusion

sounds good.

@macjohnny done. Just awaiting CI again

macjohnny · 2018-07-03T11:03:12Z

@gbrown-ce could you please merge the current master and resolve the conflict?
@wing328 could you please add this to the 3.1.0 milestone? it should not be a breaking change, since the api.ts re-exports all classes that are now in separate files.
@TiFu could you please have a quick look at this?

jmini · 2018-07-03T11:33:25Z

@macjohnny: thank you a lot for the coordination and all the feedback on this PR.

It is nice that you found a way to make this a non-breaking change. With this it can be merged into master for 3.1.0-SNAPSHOT. This week would be the right timing to do it.

Motivation ------------- The current version of TypeScript node generation puts all of the generated code in a single file. This causes issues when using inheritance as each model may depend on another and they must be included in the proper order. Modifications ------------------ Split the mustache template into pieces so that it can properly separate the models and different apis to allow for proper compilation. I also removed some compiled JS files from the petstore example generation that shouldn't have been in the repository.

- Set up the API suffix to be a member variable for future parameterization - Deleted some more compiled files from the sample directory

…m agnostic.

gbrown-ce · 2018-07-03T13:04:02Z

@macjohnny @wing328 I sorted out the merge conflicts. Ready to merge into master.

jmini · 2018-07-03T15:03:55Z

Maybe the change is important enough to be mentioned in the Migration guide between OpenAPI-Generator versions. => This can be filed as a separated PR.

OpenAPITools#363)

gbrown-ce force-pushed the PHNX-1041-SplitGeneration branch 2 times, most recently from 2022a20 to 2cda3f0 Compare June 20, 2018 21:12

gbrown-ce force-pushed the PHNX-1041-SplitGeneration branch 4 times, most recently from 86caebf to 0f4b3ea Compare June 21, 2018 12:19

gbrown-ce force-pushed the PHNX-1041-SplitGeneration branch from 0f4b3ea to 0372e9e Compare June 21, 2018 13:05

wing328 added Client: TypeScript Enhancement: General labels Jun 21, 2018

macjohnny approved these changes Jun 22, 2018

View reviewed changes

gbrown-ce closed this Jun 22, 2018

gbrown-ce reopened this Jun 22, 2018

gbrown-ce closed this Jun 22, 2018

gbrown-ce reopened this Jun 22, 2018

gbrown-ce closed this Jun 22, 2018

gbrown-ce reopened this Jun 22, 2018

gbrown-ce force-pushed the PHNX-1041-SplitGeneration branch 2 times, most recently from 6abb83f to 5603988 Compare June 22, 2018 20:36

gbrown-ce force-pushed the PHNX-1041-SplitGeneration branch from edfa7d2 to 5d5929b Compare July 2, 2018 15:52

gbrown-ce force-pushed the PHNX-1041-SplitGeneration branch from 5d5929b to eebcdab Compare July 2, 2018 16:58

macjohnny reviewed Jul 2, 2018

View reviewed changes

gbrown-ce added 5 commits July 3, 2018 09:02

- Added some additional comments

192e339

- Set up the API suffix to be a member variable for future parameterization - Deleted some more compiled files from the sample directory

Made suggested changes to prevent breaking change and make it platfor…

142e0dd

…m agnostic.

Updated naming of mustache files to avoid confusion in the future

8be8e20

Fixing separator issue in resulting typescript

f2e1c02

gbrown-ce force-pushed the PHNX-1041-SplitGeneration branch from f24195a to f2e1c02 Compare July 3, 2018 13:03

jmini added this to the 3.1.0 milestone Jul 3, 2018

jmini merged commit 960412a into OpenAPITools:master Jul 3, 2018

jmini mentioned this pull request Jul 3, 2018

#80: ensure typescript samples are up to date #444

Merged

4 tasks

macjohnny mentioned this pull request Jul 12, 2018

[typescript-fetch] Add support for file (application/octet-stream) response types #531

Open

macjohnny mentioned this pull request Aug 30, 2018

[TypesScript] Models are sorted incorrectly in output #350

Closed

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019

Restructure TypeScript Node generation into separate files (PHNX-1041) (

708920b

OpenAPITools#363)

bassaer mentioned this pull request Jun 27, 2020

[typescript-node] generate typescript-node client file separately swagger-api/swagger-codegen#10350

Open

4 tasks

Restructure TypeScript Node generation into separate files (PHNX-1041) #363

Restructure TypeScript Node generation into separate files (PHNX-1041) #363

Conversation

gbrown-ce commented Jun 20, 2018 • edited Loading

PR checklist

Description of the PR

macjohnny commented Jun 21, 2018

gbrown-ce commented Jun 21, 2018 • edited Loading

gbrown-ce commented Jun 21, 2018

macjohnny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macjohnny commented Jun 22, 2018

gbrown-ce commented Jun 22, 2018

gbrown-ce commented Jun 22, 2018

macjohnny commented Jun 22, 2018

gbrown-ce commented Jun 22, 2018

macjohnny commented Jun 22, 2018

macjohnny commented Jun 22, 2018

gbrown-ce commented Jun 22, 2018

gbrown-ce commented Jun 22, 2018

wing328 commented Jun 25, 2018

wing328 commented Jun 25, 2018

wing328 commented Jun 25, 2018

macjohnny commented Jun 25, 2018

macjohnny commented Jun 25, 2018

gbrown-ce commented Jul 2, 2018

gbrown-ce commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbrown-ce Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

macjohnny Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macjohnny commented Jul 3, 2018

jmini commented Jul 3, 2018

gbrown-ce commented Jul 3, 2018

jmini commented Jul 3, 2018

gbrown-ce commented Jun 20, 2018 •

edited

Loading

gbrown-ce commented Jun 21, 2018 •

edited

Loading

gbrown-ce Jul 2, 2018 •

edited

Loading

macjohnny Jul 2, 2018 •

edited

Loading