Typescript saga immutablejs enhancements and small fixes #10444

bflamand · 2021-09-21T15:51:16Z

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 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
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*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
@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)

Changes:

Changes affecting all typescript fetch projects:

found and fixed issue caused by someone else in apis generation (wrongly placed coma causing compilation error in some circumstance).

Changes affecting only typescript fetch saga&record mode:

code reractoring between operation and property processing.
improved generated typescript typings when dealing with uniqueIds in some circumstances (| null)
exposed entire ConfigurationParameter object to generated sagaApiManager.init() to make it more versatible by host application.
improved support for more complex array immutable types with proper typings: 2d list

…mustache test file.

…sUniqueId.

Sync with latest openapi master

…utablejs # Conflicts: # modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenModel.java # modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenParameter.java # modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenProperty.java # modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptFetchClientCodegen.java # modules/openapi-generator/src/main/resources/typescript-fetch/models.index.mustache # modules/openapi-generator/src/test/java/org/openapitools/codegen/options/TypeScriptFetchClientOptionsProvider.java # modules/openapi-generator/src/test/java/org/openapitools/codegen/typescript/fetch/TypeScriptFetchClientOptionsTest.java

…Model used exclusively by TypescriptFetchClient. Adding missing samples files.

…egenOperation used exclusively by TypescriptFetchClient.

…genProperty used exclusively by TypescriptFetchClient.

…egenParameter used exclusively by TypescriptFetchClient.

…l concept of "operation return passthrough"

…dependencies in models and other special cases. Also fixed issues with default values for some records properties.

…s in some cases. Fix issues with enum default values.

merge latest openapi

…utablejs

… that cannot be used in Records. Added missing export to record: toApi().

… is an array.

…uilt-in "reserved words" feature. Fix minor issues with typings in generated files.

…licts. Added generated ApiEntities (record, reducer & selector) files.

Merge openapi master

bflamand · 2021-09-21T15:55:45Z

...api-generator/src/test/java/org/openapitools/codegen/protobuf/ProtobufSchemaCodegenTest.java

@@ -14,6 +14,7 @@
 * limitations under the License.
 */

+/*
 package org.openapitools.codegen.protobuf;


Same here:
This is not part of my work, I only commented this test file because it caused errors during "mvm clean install"... The tests were crashing and preventing the build to complete.

bflamand · 2021-09-21T16:02:46Z

modules/openapi-generator/src/main/resources/typescript-fetch/apis.mustache

@@ -337,7 +337,7 @@ export class {{classname}} extends runtime.BaseAPI {
        return await response.value();
        {{/returnType}}
        {{^returnType}}
-        await this.{{nickname}}Raw({{#allParams.0}}{ {{#allParams}}{{paramName}}: {{paramName}}{{^-last}}, {{/-last}}{{/allParams}} }{{/allParams.0}}, initOverrides);
+        await this.{{nickname}}Raw({{#allParams.0}}{ {{#allParams}}{{paramName}}: {{paramName}}{{^-last}}, {{/-last}}{{/allParams}} }, {{/allParams.0}}initOverrides);


this is to fix a bug caused by someone else but I found the fix. The coma was present even if the first parameter was not. My sample pet store exposed this issue and the generated typescript code did not compile correctly.

bflamand · 2021-09-21T16:06:24Z

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

                param.dataTypeAlternate = param.dataType;
                if (param.isArray) {
+                    param.isUniqueId = param.isUniqueId || param.itemsAreUniqueId();


mostly a code refactoring to improve and unify similar code for codegen property and operation logic. Includes some edge cases fixes and improvements to typings.

Do you think you can add some unit tests that demonstrate / verify the above mentioned edge cases fixes?

I dont have time to add tests right now. I did add fake endpoints to the saga&record samples to validate the output of the new edge cases but no unit test yet. Hopefully, that is enough for now. Remember that the saga&record mode is basically only used by our project for now. I may have time in the future to add meaningful tests for this mode. Or even spawn a completely separate generator not attached to typescript fetch eventually but I am not ready for that right now.

bflamand · 2021-09-21T16:07:46Z

modules/openapi-generator/src/main/resources/typescript-fetch/recordGeneric.mustache

@@ -96,7 +96,17 @@ class {{classname}}RecordUtils extends ApiRecordUtils<{{classname}}, {{classname
        (apiObject as any).recType = {{#isEntity}}asEntity ? {{classname}}RecordEntityProps.recType : {{/isEntity}}{{classname}}RecordProps.recType;
 {{#vars}}
        {{#isUniqueId}}
-        {{^required}}if (apiObject.{{name}}) { {{/required}}(apiObject as any).{{name}} = apiObject.{{name}}.{{#isArray}}map(item => item.{{/isArray}}toString(){{#isArray}}){{/isArray}};{{^required}} } {{/required}}
+        {{#isArray}}


fix issues for items arrays in some circumstances

bflamand · 2021-09-21T17:10:41Z

This pull request contains the latests improvements/tweaks to the typescript saga & immutablejs generator that were added in the last few weeks/months. We continue to use this in production code, it works very well.

@wing328 Can you please merge this at your convenience!
Thanks!

Surefire fix

…utablejs-enhancements

wing328 · 2021-09-28T02:32:42Z

modules/openapi-generator/src/main/resources/typescript-fetch/sagaApiManager.mustache

-            }
-        };
-
+    public static init(apiBaseConfig: ConfigurationParameters) {


Is this a breaking change as the init method signature has changed?

yes, technically this is a breaking change. It was done to provide more flexibility to the application. It only affect those using the saga&record mode, which almost certainly means only us for now. No project using the default typescriptfetch mode is affected by this.

So in other words, I did not touch at all the code for the base typescriptfetch. Only the sage&record mode is tweaked with this pull request.
Just to be perfectly clear: The original typescriptfetch remains now totally unaffected with zero modifications. This was the case when we did the previous huge pull request that was merged and it is still the case now. :-)

merge master

…utablejs-enhancements

bflamand · 2021-10-14T17:19:33Z

@wing328 Still awaiting for this pull request to be merged...

macjohnny · 2021-10-14T18:54:46Z

@amakhrov or @akehir would you have time for a review?

amakhrov · 2021-10-14T19:06:07Z

@bflamand thanks for your contribution!
Do you think you could list the issues you're fixing here - probably in the PR description?
It would help with reviewing, and probably also later when putting together changelog for the new version.
Thanks!

bflamand · 2021-10-15T13:45:49Z

Hello @amakhrov @wing328 , as requested I added a "Changes" section to the initial PR description. You can also check the various comments I placed in the review for more info. This is just a small improvement&tweaks release to the typescript fetch saga&record mode.
Hopefully, you can get this merged soon, thank you!

amakhrov · 2021-10-14T19:33:42Z

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

+                return ((ExtendedCodegenProperty)this.items).getItemsDataType();
+            };
+            return this.items.dataType;
+        }


looks like these helpers are duplicated in the two classes. I'm wondering if it can be extracted/reused somehow, instead of duplicating?

the code appear identical in both classes, I will look into this.

I pushed a fix that reuse the core code similar in both classes for these helpers.

amakhrov · 2021-10-17T20:58:39Z

samples/client/petstore/typescript-fetch/builds/sagas-and-records/src/apis/PetApiSagas.ts

@@ -180,7 +185,7 @@ export function *findPetsByIdsSagaImp(_action_: Action<PayloadFindPetsByIds>) {
        yield put(findPetsByIdsRequest(requestPayload));

        const response: Required<Array<Pet>> = yield apiCall(Api.petApi, Api.petApi.findPetsByIds,
-            ids.map(p => parseFloat(p)).toArray(),
+            ids.map(p => (p ? parseFloat(p) : null) as number ).toArray(),


Type assertion looses type safety here. Actually the code is now lying about the actual type of the values
Could you clarify why it's needed?

This is hard to explain, I will try. We built the saga&record mode on top of the existing typescript fetch. This was faster than creating a fully standalone generator. In the saga&record mode, there is another layer of generated code on top of the original typescript fetch output. And in this mode we fully support the very strict typechecking of the latest typescript versions. When using the saga&record mode, the application will NOT call Api.petApi.findPetsByIds directly, it will instead essentially use the "findPetsByIdsSagaImp" function. This fuction exposes the correct and very strict typings to the application, this is what is important.

Now, the underlying typescript fetch generated api function (which is called here by the SagaImp() function) does not fully support complex typechecking like that. We did not want to modify in any way the original typescript fetch. So here we mask some typings when going through the typescript fetch layer. For example here, the type is only generated as "Array<number>" in the original typescript fetch instead of Array<number | null>. The important thing for us is that the types used by the application (i.e. the types in the immutable Records and Payload interfaces) are strict and correct.

I'm not sure why it can ever be null though. but anyway, it probably doesn't affect the end application.

just slightly concerned about added complexity in the template, which would need to be maintained going forward - so the less gotchas it has the better (otherwise the next contributor is likely to change that to something that seems simpler)

@amakhrov Like @bflamand said previously, the real goal is to create a new generator for this alone (saga&record) but because of time constraint on our current project, this approach was quicker and easier. But our team is working a lot with that and I'm pretty sure next year we will be able to extract this and create its own generator.

amakhrov · 2021-10-17T21:01:54Z

samples/client/petstore/typescript-fetch/builds/sagas-and-records/src/models/CategoryRecord.ts

@@ -61,6 +61,7 @@ class CategoryRecordUtils extends ApiRecordUtils<Category, CategoryRecord> {

    public *toInlined(entityId?: string | null) {
        if (!entityId) {return undefined; }
+        // @ts-ignore
        const entity = yield select(apiEntityCategorySelector, {id: entityId});


something fishy is going on here. it looks like the generated code is no compiling, so you're suppressing some typechecks.
is it possible to generate stricter ts code in the first place?

These ts-ignore are covering a typescript error that is essentially just a warning that the type will be assumed to be any. This is again a very complex issue to explain if you are not familiar with the behavior of typescript with generator function typings. In a latest typescript version, they improved the strictness of the typechecking with regards to the "yield" keywords used in generator functions and depending which version of typescript you use in your project this might be an issue,. Also note that generator functions are still not fully supported yet in typescript (and may never be). Since this generated code is essentially an implementation detail and is not exposed directly to the application, it is more versatile to prefix some yield statements with ts-ignore directives to allow the application more choice in its typescript version.
Also, if an application whishes to use the very latest typescript version in strict mode(like we do), some of the yields statements are very hard/impossible to type correctly so it is simpler to use ts-ignore.
Again, from the point of vue of the host project, this is just an implementation detail and it does not affect the typings or compilation of the host application.

thanks, I didn't realize TS still has issues with generators

amakhrov · 2021-10-17T21:08:00Z

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

                param.dataTypeAlternate = param.dataType;
                if (param.isArray) {
+                    param.isUniqueId = param.isUniqueId || param.itemsAreUniqueId();


Do you think you can add some unit tests that demonstrate / verify the above mentioned edge cases fixes?

…endedCodegenParameter.

macjohnny · 2021-10-19T19:46:23Z

@amakhrov apart from missing unit tests (which could be added later), do you think the pr is ready to be merged?

bflamand · 2021-10-27T14:19:48Z

@macjohnny @amakhrov Still awaiting for this to get merged..., can someone have a look at this please? I think all issues/questions were addressed and we should be good to go.

* OpenAPITools-master: (457 commits) [java][jersey2] remove warning using JsonMapper.builder (OpenAPITools#10734) update scribejava to 8.x (OpenAPITools#10733) [powershell] add file upload support (OpenAPITools#10735) Add openapi-generator kotlin article (OpenAPITools#10731) [ts-angular]: add ts-ignore directives to avoid compilation errors (OpenAPITools#10713) rebalance circleci tests (OpenAPITools#10727) [java][okhttp-gson] update dependencies in pom.xml (OpenAPITools#10709) [java][jersey2] update plugins in pom.xml (OpenAPITools#10710) Fix library generation compatibility with Gradle 7.2 (OpenAPITools#10716) [kotlin-spring] change the suffix from ".kt" to "Controller.kt" when generating a controller class (OpenAPITools#10671) [cpprestsdk] CMake build system improvements (OpenAPITools#10660) adds get/setHasMultipleTypes to Java schema classes (OpenAPITools#10715) update microprofile to newer version (OpenAPITools#10714) Typescript saga immutablejs enhancements and small fixes (OpenAPITools#10444) add an option for configKey (OpenAPITools#10707) Allow specification of configkey for microprofile clients (OpenAPITools#10693) Update crystal client gitignore.mustache with shards related files (OpenAPITools#10698) Adds ComposedSchema to store schema composed schemas (OpenAPITools#10653) Adds setPrettyPrint and the reslver MethodValueResolver.INSTANCE (OpenAPITools#10683) [dart] Fix pub server URL (OpenAPITools#10695) ...

bflamand-work and others added 30 commits September 29, 2020 15:13

first commit: add cli option for saga and records. Added dummy sagas.…

ac55b6c

…mustache test file.

More progress with default values. First prototype for isEntity and i…

61a3033

…sUniqueId.

record generation complete

2e3a04e

record generation complete

9411143

progress with saga generation

6e867ee

progress with saga generation

3da2805

Merge pull request #1 from OpenAPITools/master

7b42777

Sync with latest openapi master

first fully working saga generation

9a3160c

merge with latest master

5554245

removed unneeded "items" properties.

f8030fb

moved global CodegenModel modifications into subclass ExtendedCodegen…

8f33935

…Model used exclusively by TypescriptFetchClient. Adding missing samples files.

moved global CodegenOperation modifications into subclass ExtendedCod…

f387d06

…egenOperation used exclusively by TypescriptFetchClient.

moved global CodegenProperty modifications into subclass ExtendedCode…

c586da2

…genProperty used exclusively by TypescriptFetchClient.

moved global CodegenParameter modifications into subclass ExtendedCod…

b273409

…egenParameter used exclusively by TypescriptFetchClient.

added the missing "allSagas" export.

9b48cf2

renamed & reworked "meta data response" flags to a more useful genera…

1796282

…l concept of "operation return passthrough"

added vendor flag keepAsJSObject as escape hatch to support circular …

a82ff31

…dependencies in models and other special cases. Also fixed issues with default values for some records properties.

added autodetection for passthrough to simplify standardised specs.

549797d

fix small issue with passthrough void

1706006

fix small issues with passthrough void and missing passthrough import…

32c97a4

…s in some cases. Fix issues with enum default values.

fix small issues with passthrough void and missing passthrough import…

e6c5ef3

…s in some cases. Fix issues with enum default values.

Merge pull request #3 from OpenAPITools/master

ea5a5eb

merge latest openapi

Merge remote-tracking branch 'origin/master' into typescript-saga-imm…

e86c81f

…utablejs

Added "reservedRecordField" feature to support remapping fields names…

9160502

… that cannot be used in Records. Added missing export to record: toApi().

added uniqueId inference. Fix small generation when uniqueId property…

a77f17e

… is an array.

removed feature "reservedRecordField" and replaced it with existing b…

8925de1

…uilt-in "reserved words" feature. Fix minor issues with typings in generated files.

Changed api recType names to make them less likely to cause name conf…

090150b

…licts. Added generated ApiEntities (record, reducer & selector) files.

Moved location of ApiEntities related files and fix issues with exports.

76eef30

Merge pull request #4 from OpenAPITools/master

47e06dd

Merge openapi master

bflamand commented Sep 21, 2021

View reviewed changes

- fix generate samples analysis.

98c8b41

JFCote requested a review from wing328 September 22, 2021 14:47

update surefire to newer version

e5c43ec

wing328 added Client: TypeScript Enhancement: General Issue: Bug labels Sep 25, 2021

wing328 added this to the 5.3.0 milestone Sep 25, 2021

bflamand and others added 3 commits September 27, 2021 09:30

Merge pull request #26 from OpenAPITools/surefire-fix

3db8f8a

Surefire fix

Merge remote-tracking branch 'origin/master' into typescript-saga-imm…

5ec1a19

…utablejs-enhancements

- un-commenting test files with issues.

ccfbe2f

wing328 reviewed Sep 28, 2021

View reviewed changes

bflamand and others added 2 commits October 5, 2021 09:35

Merge pull request #28 from OpenAPITools/master

fbf9368

merge master

Merge remote-tracking branch 'origin/master' into typescript-saga-imm…

24df61a

…utablejs-enhancements

amakhrov reviewed Oct 17, 2021

View reviewed changes

factored similar utility functions in ExtendedCodegenProperty and Ext…

d654a78

…endedCodegenParameter.

wing328 modified the milestones: 5.3.0, 5.3.1 Oct 25, 2021

macjohnny merged commit 27c82e8 into OpenAPITools:master Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescript saga immutablejs enhancements and small fixes #10444

Typescript saga immutablejs enhancements and small fixes #10444

bflamand commented Sep 21, 2021 •

edited

Loading

bflamand Sep 21, 2021

bflamand Sep 21, 2021

bflamand Sep 21, 2021

amakhrov Oct 17, 2021

bflamand Oct 18, 2021

bflamand Sep 21, 2021

bflamand commented Sep 21, 2021

wing328 Sep 28, 2021

bflamand Sep 28, 2021

bflamand Sep 28, 2021 •

edited

Loading

bflamand commented Oct 14, 2021

macjohnny commented Oct 14, 2021

amakhrov commented Oct 14, 2021

bflamand commented Oct 15, 2021 •

edited

Loading

amakhrov Oct 14, 2021

bflamand Oct 18, 2021

bflamand Oct 18, 2021

amakhrov Oct 17, 2021

bflamand Oct 18, 2021 •

edited

Loading

amakhrov Oct 19, 2021

JFCote Oct 20, 2021

amakhrov Oct 17, 2021

bflamand Oct 18, 2021 •

edited

Loading

amakhrov Oct 19, 2021

amakhrov Oct 17, 2021

macjohnny commented Oct 19, 2021

bflamand commented Oct 27, 2021

Typescript saga immutablejs enhancements and small fixes #10444

Typescript saga immutablejs enhancements and small fixes #10444

Conversation

bflamand commented Sep 21, 2021 • edited Loading

PR checklist

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

bflamand commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflamand Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

bflamand commented Oct 14, 2021

macjohnny commented Oct 14, 2021

amakhrov commented Oct 14, 2021

bflamand commented Oct 15, 2021 • edited Loading

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

bflamand Oct 18, 2021 • edited Loading

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

bflamand Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macjohnny commented Oct 19, 2021

bflamand commented Oct 27, 2021

bflamand commented Sep 21, 2021 •

edited

Loading

bflamand Sep 28, 2021 •

edited

Loading

bflamand commented Oct 15, 2021 •

edited

Loading

bflamand Oct 18, 2021 •

edited

Loading

bflamand Oct 18, 2021 •

edited

Loading