[bug][typescript] Fix node client generator import file paths #7410

mahirk · 2020-09-14T00:55:05Z

Bug and Recap

Issue: Resolves #7385

For mapping such as:

  "importMappings": {
    "Pet": "../dataModel/petModel"
  }

Actual

import { AModelPetModel } from '../../dataModel/petModel';

Expected

import { Pet } from '../dataModel/petModel';

For a mapping such as:

  "importMappings": {
    "Pet": "@company/prefix-zoo-store"
  }

Actual

import { NyPrefixZooStore } from '../@company/prefix-zoo-store';

Expected

import { Pet } from '@company/prefix-zoo-store';

Method

Below is the map in operations.get("imports") based on the import location:

Model is created by generator

[{import=model/pet, classname=Pet}]

BYOModel

[{import=@company/prefix-zoo-store, classname=Pet}]

Therefore the logic was updated to a simpler version, assuming the mapping is created by prior areas.

for (Map<String, Object> im : imports) {
     im.put("filename", im.get("import").toString());
}

This resulted in the following import for:

Created Model

import { Pet } from '../model/pet';

BYOModel

import { Pet } from '../@company/prefix-zoo-store';

Per the above, this fixed the issue of the improper render which led to the bad classname i.e. what led to the following NyPrefixZooStore

The next issue was the extra ../ which is embedded as part of the template. See: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/typescript-node/api-single.mustache#L8

For this purpose, the extra directory listing was remove from the template and added into the Codegen itself, where the ../ is appended by default ONLY when the model is derived from an auto generated file.

After this change here are the mappings in operations.get("imports")

Model is created by generator

[{import=../model/pet, classname=Pet}]

BYOModel

[{import=@company/prefix-zoo-store, classname=Pet}]

This resulted in the following import for:

Created Model

import { Pet } from '../model/pet';

BYOModel

import { Pet } from '@company/prefix-zoo-store';

Testing

Unit

Updated tests for defaultModelImportTest
Added tests for postProcessOperationsWithModels

Manual

Ran the above expected v/s actual output comparisons to ensure the values provided are accurate

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

…s; add tests

mahirk · 2020-09-14T01:51:22Z

Tagging the most recent contributors to the typescript generator per the contributor list:
@petejohansonxo
@amakhrov
cc: @wing328

mahirk · 2020-09-15T17:46:22Z

Tagging in a few more TS contributors from the technical commitee to help have a look at this:
@Vrolijkx @macjohnny @topce

mahirk · 2020-09-17T05:14:23Z

@jimschubert as another active maintainer for a review
@sebastianhaas as a TS Contributor

amakhrov

Thanks for your contribution! The fix looks reasonable to me (just some minor suggestions).

I'm also wondering if this issue (and the fix) are typescript-node specific, or other typescript generators suffer from the same bug.

amakhrov · 2020-09-17T05:45:22Z

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

@@ -222,8 +223,7 @@ public String toModelImport(String name) {
        // Add additional filename information for model imports in the apis
        List<Map<String, Object>> imports = (List<Map<String, Object>>) operations.get("imports");
        for (Map<String, Object> im : imports) {
-            im.put("filename", im.get("import"));
-            im.put("classname", getModelnameFromModelFilename(im.get("filename").toString()));


looks like now the getModelnameFromModelFilename method is no longer used and can be removed?

amakhrov · 2020-09-17T05:52:10Z

...java/org/openapitools/codegen/typescript/typescriptnode/TypeScriptNodeClientCodegenTest.java

+        Assert.assertEquals(extractedImports.get(0).get("filename"), importName);
+    }
+
+    private Map<String, Object> postProcessOperationsHelper(String importName) {


Perhaps just nitpicking - but maybe there could be a better name for this method? Right now it's kind of unclear what its intention is (unless I read the actual method body).

…n name

mahirk · 2020-09-17T08:30:44Z

Thanks for the review @amakhrov! Made the suggestions as recommended above.
The bug is likely other ts generators. Though it does mean more reviews, once this is approved, I was hoping to break down a pr for the current ts generators (and additionally try and add a sample as well so that it can be well tested)
Here is my testing so far:

Generator	Issue	LoE
node	does not use import mappings correctly	low
axios	does not support import mappings	medium
fetch	does not use import mappings correctly + expects "to" and "from" json methods	medium to high (will need to think of a better way to do this)
angular	the template assumes everything to be in the mode directory, hence it fails https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/typescript-angular/api.service.mustache#L11	low

I'll potentially look at a quick fix for angular next and then attempt to evaluate axios and fetch since those are popular libraries.

Another issue for typescript node is: #7440

macjohnny

LGTM

jimschubert · 2020-09-18T01:43:58Z

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

@@ -162,7 +163,7 @@ public String toModelImport(String name) {
            return importMapping.get(name);
        }

-        return modelPackage() + "/" + camelize(toModelName(name), true);
+        return DEFAULT_MODEL_IMPORT_DIRECTORY_PREFIX + modelPackage() + "/" + camelize(toModelName(name), true);


@mahirk not a blocker on this pr, but wanted to comment since you said you were planning to look at ts implementation in other generators...

since modelPackage is passed by the user, this could result in models// when passed with trailing slash. It might be best to use StringUtils, something like:

return DEFAULT_MODEL_IMPORT_DIRECTORY_PREFIX + StringUtils.appendIfMissing(modelPackage(), "/") + camelize(toModelName(name), true);

jimschubert

looks good. Just left a small comment if you're looking to do the same in other generators.

tomh4 · 2021-01-20T22:21:07Z

was this ever

Thanks for the review @amakhrov! Made the suggestions as recommended above.
The bug is likely other ts generators. Though it does mean more reviews, once this is approved, I was hoping to break down a pr for the current ts generators (and additionally try and add a sample as well so that it can be well tested)
Here is my testing so far:

Generator Issue LoE
node does not use import mappings correctly low
axios does not support import mappings medium
fetch does not use import mappings correctly + expects "to" and "from" json methods medium to high (will need to think of a better way to do this)
angular the template assumes everything to be in the mode directory, hence it fails https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/typescript-angular/api.service.mustache#L11 low
I'll potentially look at a quick fix for angular next and then attempt to evaluate axios and fetch since those are popular libraries.

Another issue for typescript node is: #7440

Did you ever figure out how to get this working with typescript-fetch ? The importMappings still seem to be ignored completely

fix TypeScriptNodeClientCodegen import filenames and related template…

295408b

…s; add tests

mahirk mentioned this pull request Sep 14, 2020

[BUG] [Typescript-Node] Import mapping seems to be working incorrectly with typescript node #7385

Closed

6 tasks

quick imports cleanup

f76fde6

amakhrov reviewed Sep 17, 2020

View reviewed changes

per pr suggestion: remove unused function; change test helper functio…

f112e67

…n name

macjohnny approved these changes Sep 17, 2020

View reviewed changes

jimschubert reviewed Sep 18, 2020

View reviewed changes

jimschubert approved these changes Sep 18, 2020

View reviewed changes

jimschubert added this to the 5.0.0 milestone Sep 18, 2020

jimschubert added Client: TypeScript Server: TypeScript Issue: Bug and removed Server: TypeScript labels Sep 18, 2020

jimschubert changed the title ~~Fix TypeScriptNodeClientCodegen import filenames and related templates~~ [bug][typescript] Fix node client generator import file paths Sep 18, 2020

jimschubert merged commit ca5d384 into OpenAPITools:master Sep 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bug][typescript] Fix node client generator import file paths #7410

[bug][typescript] Fix node client generator import file paths #7410

mahirk commented Sep 14, 2020 •

edited

Loading

mahirk commented Sep 14, 2020

mahirk commented Sep 15, 2020

mahirk commented Sep 17, 2020

amakhrov left a comment

amakhrov Sep 17, 2020

amakhrov Sep 17, 2020

mahirk commented Sep 17, 2020

macjohnny left a comment

jimschubert Sep 18, 2020

jimschubert left a comment

tomh4 commented Jan 20, 2021

[bug][typescript] Fix node client generator import file paths #7410

[bug][typescript] Fix node client generator import file paths #7410

Conversation

mahirk commented Sep 14, 2020 • edited Loading

Bug and Recap

Method

Testing

Unit

Manual

PR checklist

mahirk commented Sep 14, 2020

mahirk commented Sep 15, 2020

mahirk commented Sep 17, 2020

amakhrov left a comment

Choose a reason for hiding this comment

amakhrov Sep 17, 2020

Choose a reason for hiding this comment

amakhrov Sep 17, 2020

Choose a reason for hiding this comment

mahirk commented Sep 17, 2020

macjohnny left a comment

Choose a reason for hiding this comment

jimschubert Sep 18, 2020

Choose a reason for hiding this comment

jimschubert left a comment

Choose a reason for hiding this comment

tomh4 commented Jan 20, 2021

mahirk commented Sep 14, 2020 •

edited

Loading