[typescript-fetch] namespacing the generated request object types #3695

jgiles · 2019-08-19T22:21:30Z

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
Filed the PR against the correct branch: master, 4.1.x, 5.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

Fix #1998 by namespacing the generated request object types used in the typescript-fetch client codegen.

Fix OpenAPITools#1998 by namespacing the generated request object types used in the typescript-fetch client codegen.

auto-labeler · 2019-08-19T22:21:34Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

macjohnny · 2019-08-20T06:00:30Z

cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10) @akehir (2019/07)

macjohnny

this is a breaking change, so this should either be configurable or we have to wait until 5.x to include it.

macjohnny · 2019-08-20T06:07:49Z

cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10) @akehir (2019/07)

macjohnny · 2019-08-20T06:08:23Z

@jgiles thanks for your PR!

jgiles · 2019-08-20T16:29:42Z

samples/client/petstore/typescript-fetch/builds/default/.openapi-generator/VERSION

I'm not sure why the version is clobbered when I run the sample code re-generation. I can manually undo if necessary.

are you sure you ran mvn clean package before generating the samples? are you based on the latest master?

I'm based on 024f468 (master circa 2019-08-18).

If I run

mvn clean package ./bin/typescript-fetch-petstore-all.sh

on my current branch (where the .openapi-generator/VERSION files already have "unset"), no sample files are changed. If I run it on master, the .openapi-generator/VERSION files are overwritten to "unset".

FWIW, I (but no one else on my team) had this problem when running a brew-installed jar of of the old swagger-codegen, which is part of why we switched to running it in Docker... there could just be something weird in my local env.

fixed manually

jgiles · 2019-08-20T16:39:00Z

@macjohnny thanks for the speedy review!

Per your suggestion, I have introduced a new CLI option for namespacing the request objects. I patterned the added option after existing options in the codegen class, let me know if adjustments are needed.

Currently, the option defaults to false so that this would be mergeable into master for inclusion in the next patch release (I'm interested in making this available as soon as possible, since the fixed issue blocks my team's migration from the old swagger-codegen). However, it might be worth considering:

Making the option default to true in the next minor release branch
Removing the option in the 5.0 major release branch

so that the fix is present by default.

macjohnny · 2019-08-20T16:40:58Z

@jgiles thanks for the changes! I will review them soon

macjohnny

LGTM

macjohnny · 2019-08-20T16:52:44Z

@jgiles I think we can merge this and have this released in 4.1.1
if you want you could copy and adapt the existing builds and/or tests to have your feature compiled/tested by CI and prevent regressions, see e.g.

openapi-generator/pom.xml

Lines 834 to 845 in 4575b30

    
           <profile> 
        
               <id>typescript-fetch-client-builds-with-npm-version</id> 
        
               <activation> 
        
                   <property> 
        
                       <name>env</name> 
        
                       <value>java</value> 
        
                   </property> 
        
               </activation> 
        
               <modules> 
        
                   <module>samples/client/petstore/typescript-fetch/builds/with-npm-version</module> 
        
               </modules> 
        
           </profile>

https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/typescript-fetch/builds/with-npm-version/pom.xml

macjohnny · 2019-08-20T16:53:43Z

@jgiles by the way, would you like to add your company to https://github.com/OpenAPITools/openapi-generator#4---companiesprojects-using-openapi-generator ?

jgiles · 2019-08-20T17:03:06Z

I will add the Maven builds, and will check about adding company name :-)

(I'm dropping offline for a little bit though, so it will be this evening probably)

jgiles · 2019-08-20T21:29:14Z

@macjohnny added the maven builds, fixed the version files.

jgiles · 2019-08-21T04:49:18Z

There's a build failure in the Maven stuff. The error looks to be for a different typescript-fetch sample, but I think the new one might also have problems. There's a bit of messiness in the setup of the sample tests, I might need some help getting that bit right.

macjohnny · 2019-08-21T06:30:41Z

@jgiles the travis-error seems unrelated, but you need to register your pom.xml here:

openapi-generator/pom.xml

Lines 834 to 845 in 4575b30

    
           <profile> 
        
               <id>typescript-fetch-client-builds-with-npm-version</id> 
        
               <activation> 
        
                   <property> 
        
                       <name>env</name> 
        
                       <value>java</value> 
        
                   </property> 
        
               </activation> 
        
               <modules> 
        
                   <module>samples/client/petstore/typescript-fetch/builds/with-npm-version</module> 
        
               </modules> 
        
           </profile>

moreover, the package.json is missing to successfully run npm install or npm run build in https://github.com/OpenAPITools/openapi-generator/tree/dddc201f05e7167651c012c55742bae0179b5b30/samples/client/petstore/typescript-fetch/builds/namespace-parameter-interfaces, so I would recommend setting the npmVersion in https://github.com/OpenAPITools/openapi-generator/blob/dddc201f05e7167651c012c55742bae0179b5b30/bin/typescript-fetch-petstore-multiple-parameters.json similar to https://github.com/OpenAPITools/openapi-generator/blob/dddc201f05e7167651c012c55742bae0179b5b30/bin/typescript-fetch-petstore-with-npm-version.json

If we don't manage to have the CI tests run, I would recommend to merge it without them, so this can be part of the next release, and add the CI tests in a separate PR.

jgiles · 2019-08-21T15:21:48Z

Thanks @macjohnny for the pointers!

I am registering the pom.xml here: https://github.com/OpenAPITools/openapi-generator/pull/3695/files#diff-600376dffeb79835ede4a0b285078036R846

I'm now doing the codegen for CI in the same way as the with-npm-version variant, which does produce the needed package.json. Hopefully that will resolve the build issues.

jgiles · 2019-08-21T15:22:29Z

I would also be OK merging without the CI. What needs to happen in order to merge?

jgiles · 2019-08-21T15:42:15Z

@macjohnny also added Paxos to the list of companies using openapi-generator!

macjohnny · 2019-08-21T15:47:49Z

I would also be OK merging without the CI. What needs to happen in order to merge?

I will merge it tomorrow if the ci build succeeds

macjohnny · 2019-08-21T15:50:04Z

Can you please merge the most recent master into your branch? The unrelated typescript-axios build failure should be disabled

jgiles · 2019-08-21T16:11:21Z

Merged in latest master

jgiles · 2019-08-22T04:57:09Z

Using the "Requests" name invariably led to name conflicts between the Requests namespaces when there were multiple services (tags). This issue did reproduce in petstore - I'm not sure why CI didn't catch the compilation error.

I've updated to use the service basename as a prefix (like "PetRequests")

macjohnny · 2019-08-22T05:21:36Z

...generator/src/main/java/org/openapitools/codegen/languages/TypeScriptFetchClientCodegen.java

            operations.put("namespaceParameterInterfaces", true);
            operations.put("paramIfaceIndent", "    ");
            operations.put("paramIfaceSuffix", "");
-            operations.put("paramIfaceNsPrefix", "Requests.");
+            operations.put("paramIfaceNsPrefix", operationList.get(0).baseName + "Requests.");


maybe this has to be sanitized, but we will see later

jgiles added 2 commits August 19, 2019 18:12

[typescript-fetch] Fix OpenAPITools#1998: namespace reqs.

83fa8b6

Fix OpenAPITools#1998 by namespacing the generated request object types used in the typescript-fetch client codegen.

Update petstore samples.

78a36f4

auto-labeler bot added the Client: TypeScript label Aug 19, 2019

jgiles mentioned this pull request Aug 19, 2019

[BUG] [typescript-fetch] Request Parameter Interfaces Conflict with remove-operation-id-prefix #1998

Closed

6 tasks

macjohnny requested changes Aug 20, 2019

View reviewed changes

jgiles added 2 commits August 20, 2019 12:20

Add default-false namespacing config option.

b292338

Re-run code sample generation.

021cc2e

jgiles commented Aug 20, 2019

View reviewed changes

macjohnny approved these changes Aug 20, 2019

View reviewed changes

macjohnny added this to the 4.1.1 milestone Aug 20, 2019

macjohnny added the Enhancement: Feature label Aug 20, 2019

jgiles added 3 commits August 20, 2019 17:15

Add maven build configurations.

5669877

Fix sample VERSION files.

c970d10

Update typescript-fetch docs.

dddc201

Emulate with-npm-version, adding package.json.

8b2fa3c

Add Paxos to companies using.

8973995

Merge branch 'master' into fix-1998-namespace-request-objects

6104479

Prefix namespace with basename to avoid collisions.

df38be9

macjohnny reviewed Aug 22, 2019

View reviewed changes

macjohnny merged commit b323b0a into OpenAPITools:master Aug 22, 2019

wing328 changed the title ~~[typescript-fetch] Fix #1998: namespace reqs.~~ [typescript-fetch] namespace reqs. Aug 26, 2019

wing328 changed the title ~~[typescript-fetch] namespace reqs.~~ [typescript-fetch] namespacing the generated request object types Aug 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[typescript-fetch] namespacing the generated request object types #3695

[typescript-fetch] namespacing the generated request object types #3695

jgiles commented Aug 19, 2019 •

edited

Loading

auto-labeler bot commented Aug 19, 2019

macjohnny commented Aug 20, 2019

macjohnny left a comment

macjohnny commented Aug 20, 2019

macjohnny commented Aug 20, 2019

jgiles Aug 20, 2019

macjohnny Aug 20, 2019

jgiles Aug 20, 2019

jgiles Aug 20, 2019

jgiles commented Aug 20, 2019

macjohnny commented Aug 20, 2019

macjohnny left a comment

macjohnny commented Aug 20, 2019

macjohnny commented Aug 20, 2019

jgiles commented Aug 20, 2019

jgiles commented Aug 20, 2019

jgiles commented Aug 21, 2019

macjohnny commented Aug 21, 2019

jgiles commented Aug 21, 2019

jgiles commented Aug 21, 2019

jgiles commented Aug 21, 2019

macjohnny commented Aug 21, 2019

macjohnny commented Aug 21, 2019

jgiles commented Aug 21, 2019

jgiles commented Aug 22, 2019

macjohnny Aug 22, 2019

[typescript-fetch] namespacing the generated request object types #3695

[typescript-fetch] namespacing the generated request object types #3695

Conversation

jgiles commented Aug 19, 2019 • edited Loading

PR checklist

Description of the PR

auto-labeler bot commented Aug 19, 2019

macjohnny commented Aug 20, 2019

macjohnny left a comment

Choose a reason for hiding this comment

macjohnny commented Aug 20, 2019

macjohnny commented Aug 20, 2019

jgiles Aug 20, 2019

Choose a reason for hiding this comment

macjohnny Aug 20, 2019

Choose a reason for hiding this comment

jgiles Aug 20, 2019

Choose a reason for hiding this comment

jgiles Aug 20, 2019

Choose a reason for hiding this comment

jgiles commented Aug 20, 2019

macjohnny commented Aug 20, 2019

macjohnny left a comment

Choose a reason for hiding this comment

macjohnny commented Aug 20, 2019

macjohnny commented Aug 20, 2019

jgiles commented Aug 20, 2019

jgiles commented Aug 20, 2019

jgiles commented Aug 21, 2019

macjohnny commented Aug 21, 2019

jgiles commented Aug 21, 2019

jgiles commented Aug 21, 2019

jgiles commented Aug 21, 2019

macjohnny commented Aug 21, 2019

macjohnny commented Aug 21, 2019

jgiles commented Aug 21, 2019

jgiles commented Aug 22, 2019

macjohnny Aug 22, 2019

Choose a reason for hiding this comment

jgiles commented Aug 19, 2019 •

edited

Loading