[typescript] Avoid code duplication by bodograumann · Pull Request #20978 · OpenAPITools/openapi-generator

bodograumann · 2025-03-26T18:16:00Z

In #20430 a lot of new code was added in the output. Most of it can be avoided by reusing central functions.

These changed should not change the functionality.
There is however one minor detail that is different now: If _options is given, but without any middleware, the existing middleware is kept. To clear it { middleware: [] } needs to be passed. I think the previous behaviour was an oversight, becacuse all other items fall back properly as well.

Extract reusable logic into central functions
Regenerate samples
Update test dependencies to make them run

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.
Run the following to build the project and update samples:
```
./mvnw clean package || exit
./bin/generate-samples.sh ./bin/configs/*.yaml || exit
./bin/utils/export_docs_generators.sh || exit
```
(For Windows users, please run the script in Git BASH)
Commit all changed files.
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*.
IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

CC @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04) @joscha (2024/10)

joscha

The diff is hard to check - can we revert the whitespace only and quote changes?

Also, regarding the changed default behavior, did you revert that to the behavior before the change in this pull request or did you just mention it for a follow up? I think we should probably restore it to what it was, and involve the original author and @macjohnny when doing so.
Possibly two PRs, one clean refactor and one restore ot the previous behavior might be good. Don't mind the order.

davidgamero · 2025-03-26T23:56:53Z

this significantly cleans up the logic.

If _options is given, but without any middleware, the existing middleware is kept.

this was done to preserve backward compatibility with the previous pattern in which the entire Configuration object passed in _options replaced the api's internal configuration. In that scenario, passing an _options with undefined middleware would still result in no middleware.

the default use of the 'replace' middlewareMergeStrategy really only makes sense for middelware values that are arrays, so I'd be in favor of this change to the default behavior.

to be clear, the change is the following:

{middleware: undefined }  // preserves existing middleware

{middleware: []} // replaces existing middleware with empty array, same as
{middleware: [], middlewareMergeStrategy: 'replace'} // same as above since default middlewareMergeStrategy is 'replace'

this would benefit greatly from adding tests to cover both cases here's where i put the current tests

bodograumann · 2025-03-27T07:43:05Z

You are absolutely right, @joscha. I probably crammed in to much into a single commit in the flow of things.
I can also split up the logic changes into a separate PR and try to do things more clearly.
Are you ok with looking at individual commits? Then I would simply split the whitespace changes into a separate commit.
Also, would you prefer one commit to generate the samples, one sample commit per template change commit or for me to regenerate samples in the same commit as the template change?

@davidgamero Thanks for your support :-) Adding tests is definitely a good idea.
Actually, if I'm not mistaken, originally you could not pass undefined for middleware, so it would always take the new array. Your change to make it merge the configs seemed to suggest that now we should keep the original middlewares if none are given. So I'm glad we are on the same page. 🚀

bodograumann · 2025-03-27T15:12:59Z

Ok, I now made smaller commits, @joscha . I hope this works for you.
The behaviour should be the same as before, but I inadvertantly fixed one bug and added a test for that:
When no options are given, the static middleware was dropped before. Now it correctly uses the static middleware.

Btw (you probably know this), but there is an option in github to hide whitespace changes.

I'll cerate a separate PR for more refactoring and the behaviour change when options are given, but without a middleware field.

joscha

The net negative diff length speaks for itself, well done 👏

joscha · 2025-03-31T23:51:24Z

samples/openapi3/client/petstore/typescript/tests/jquery/package.json

@@ -22,8 +22,8 @@
  "devDependencies": {
    "@types/node": "^8.10.48",
    "raw-loader": "^4.0.0",
-    "ts-loader": "^4.3.1",
-    "typescript": "^2.4.1"
+    "ts-loader": "^8.0.0",


how do these changes relate?

The tests were broken. I think they are not being executed in CI.
I just wanted to verify that my changes work, so I played with the dependency versions in the test setups.
We are generating our code with typescript-4 in mind, so I thought it might sense to also use that version in the tests.
The ts-loader was updated to the latest version that supports the webpack version we use.
I honestly can't say whether every single change here is necessary or works for all systems other than mine, but at least this way I could run most sample tests.

Also, now that I am confident, that the changes work, I don't want to spend more time on the test setup. So if the explanation is good enough for you, great. Otherwise we can also throw this commit away.

Your explanation makes sense. Thanks for testing thoroughly. I added some test automation a while back, but it doesn't pick up all the packages as some of them don't have lock files, etc. - it's not in an amazing state, so if you made changes to make them run/compile then that is great, thank you.

joscha · 2025-03-31T23:51:30Z

samples/openapi3/client/petstore/typescript/tests/object_params/package.json

@@ -15,11 +15,10 @@
    "@types/chai": "4.0.1 - 4.1.x",
    "@types/isomorphic-fetch": "0.0.34",
    "@types/mocha": "^2.2.41",
-    "@types/node": "8.10.38 - 8.10.62",
+    "@types/node": "^22",


I think there were some type conflicts with the old version. 22 is the latest stable node version.

joscha · 2025-03-31T23:51:34Z

samples/openapi3/client/petstore/typescript/tests/object_params/package.json

    "chai": "^4.1.0",
    "mocha": "^5.2.0",
-    "ts-loader": "^2.3.0",
-    "typescript": "^2.4.1"
+    "typescript": "^4.0.0"


Same explanation as above :-D

joscha · 2025-03-31T23:52:05Z

modules/openapi-generator/src/main/resources/typescript/configuration.mustache

 export interface MiddlewareMergeOptions {
-  // default is `'replace'` for backwards compatibility
-  middlewareMergeStrategy?: 'replace' | 'append' | 'prepend';
+    // default is `"replace"` for backwards compatibility


revert these to reduce diff?

I can understand that is it better to have smaller PRs, but cleaning up the code format is also useful.
On the other hand I would not want to create a separate PR just for formatting changes (right?).
So I think keeping them here as a dedicated commit still makes the most sense to me.

On the other hand I would not want to create a separate PR just for formatting changes (right?).

I think that's exactly what we should do - separate meaningful changes like this in here (refactor) from cosmetic only (style). It makes it easy to revert the chunk that causes trouble, if there is an issue (the refactor potentilly) and leave the things that are non-contentious and not risky (style) in.
A style-change-only PR is also easy to review and merge. And the noise for review is less as well, which means both PRs separately can be reviewed much faster than one big one.

So I think keeping them here as a dedicated commit still makes the most sense to me.

see above

Ok, I understand. I'll try to further split the changes over more PRs.

joscha · 2025-03-31T23:53:21Z

modules/openapi-generator/src/main/resources/typescript/configuration.mustache

+            authMethods: options.authMethods,
+        }
+    }
+    return;


superfluous?

Typescript requires an explicit return on all code paths.

TIL, we had this enabled. Not personally a fan, but 🤷 . Thanks for explaining.

joscha · 2025-03-31T23:53:28Z

modules/openapi-generator/src/main/resources/typescript/configuration.mustache

+            ),
+            middlewareMergeStrategy: options.middlewareMergeStrategy,
+            authMethods: options.authMethods,
+        }


Suggested change

}

};

joscha · 2025-03-31T23:57:21Z

modules/openapi-generator/src/main/resources/typescript/types/PromiseAPI.mustache

-	}
-        {{/useInversify}}
-        const result = this.api.{{nickname}}WithHttpInfo({{#allParams}}{{paramName}}, {{/allParams}}observableOptions);
+        const result = this.api.{{nickname}}WithHttpInfo({{#allParams}}{{paramName}}, {{/allParams}}{{#useInversify}}_options{{/useInversify}}{{^useInversify}}wrapOptions(_options){{/useInversify}});


After looking at the diff below, should we keep the observableOptions local, that we then pass to the api call?

i.e. instead of

const result = this.api.listWithHttpInfo(wrapOptions(_options));

we would have:

const observableOptions = wrapOptions(_options) const result = this.api.listWithHttpInfo(observableOptions);

this would reduce the diff a tiny bit, keep the api call separate from the wrap call and still convey the logic of what happens in the wrappng.

Take or leave.

Not quite sure what the goal is.
The samples are ok like this I think.
If you want to make the template more readable, we could wrap the parameters on separate lines.

I am not hooked on this, but basically two goals:

reduce the diff in this pull request, as the second line would stay as-is if we don't inline and/or change the variable name

reduce template complexity - the less we nest and wrap, etc. the easier to maintain. Extra lines in the output will be minified away. The current change brings the length of this line from 120 characters to 200 characters, which means it's almost twice as long, possible more than twice as complex with all the useInversify conditionals.

joscha · 2025-03-31T23:58:17Z

samples/client/others/typescript/builds/null-types-simple/types/ObservableAPI.ts

@@ -1,5 +1,5 @@
 import { ResponseContext, RequestContext, HttpFile, HttpInfo } from '../http/http';
-import { Configuration, ConfigurationOptions } from '../configuration'
+import { Configuration, ConfigurationOptions, mergeConfiguration } from '../configuration'


is there a simple way for us to detect if it is used or not and not produce this diff if it is not used?

This function is used always (if there is at least a single operation), so I think it makes sense to always have the import.

macjohnny · 2025-04-01T08:14:37Z

@joscha please ping me when this PR is ready to be merged :-)

bodograumann · 2025-04-01T08:55:56Z

Thanks so much for taking the time to review my changes, @joscha.
I rebased on master and added the sample changes for the new "echo_api" sample.

joscha · 2025-04-01T08:59:42Z

I rebased on master and added the sample changes for the new "echo_api" sample.

Thank you. Are you able to revert the diff in this discussion: #20978 (comment) please? Not because I don't like the change, but because this PR already has a absolute line count change of ~30,000 lines. The less we add additional changes into it, the better.

bodograumann · 2025-04-01T10:52:18Z

I reverted the independant formating changes and the inlining, to reduce the diff somewhat.
Does that work for you, @joscha ?

joscha

@macjohnny this one is ready I think, unless you have other feedback.

joscha reviewed Mar 26, 2025

View reviewed changes

bodograumann force-pushed the typescript-avoid-code-duplication branch from bdfe45c to e085d8e Compare March 27, 2025 15:07

bodograumann mentioned this pull request Mar 27, 2025

[typescript] Use static middleware when calltime options exist but specify no middleware #20985

Merged

5 tasks

joscha approved these changes Mar 31, 2025

View reviewed changes

bodograumann added 18 commits April 1, 2025 10:27

Update test dependencies to make them run

5d8d611

Use spaces instead of tabs

5fe7f2d

Remove duplicate test

9018cec

Remove unused variable

1fafe5e

Add failing test for missing options

e9d7e4a

Move config merge logic into shared function

7f4fc63

Regenerate samples

d25fa1d

Move option wrap logic into shared function

3882845

Regenerate samples

c6e9aa8

Inline wrapOptions call

13b0b84

Regenerate samples

20c07e7

Clean up code formatting

72c269b

Regenerate samples

8daf8af

Separate mergeConfiguration definitions

f3d821f

Regenerate samples

46a6046

Rename parameters

3f7b9cb

Regenerate samples

47f98c7

Add missing semicolon

67a153c

bodograumann force-pushed the typescript-avoid-code-duplication branch from a4a9b44 to 67a153c Compare April 1, 2025 08:52

Revert inlining wrapOptions call

5c7ea8f

Revert format changes in untouched lines

8a6895b

bodograumann force-pushed the typescript-avoid-code-duplication branch from adb84bb to 8a6895b Compare April 1, 2025 10:37

joscha approved these changes Apr 1, 2025

View reviewed changes

macjohnny merged commit 7881152 into OpenAPITools:master Apr 1, 2025
18 checks passed

wing328 added this to the 7.13.0 milestone Apr 27, 2025

Uh oh!

Conversation

bodograumann commented Mar 26, 2025

PR checklist

Uh oh!

joscha left a comment

Choose a reason for hiding this comment

Uh oh!

davidgamero commented Mar 26, 2025

Uh oh!

bodograumann commented Mar 27, 2025

Uh oh!

bodograumann commented Mar 27, 2025

Uh oh!

joscha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

macjohnny commented Apr 1, 2025

Uh oh!

bodograumann commented Apr 1, 2025

Uh oh!

joscha commented Apr 1, 2025

Uh oh!

bodograumann commented Apr 1, 2025

Uh oh!

joscha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants