-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Typescript saga immutablejs enhancements and small fixes #10444
Typescript saga immutablejs enhancements and small fixes #10444
Conversation
…mustache test file.
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.
…s in some cases. Fix issues with enum default values.
merge latest openapi
… that cannot be used in Records. Added missing export to record: toApi().
…uilt-in "reserved words" feature. Fix minor issues with typings in generated files.
…licts. Added generated ApiEntities (record, reducer & selector) files.
Merge openapi master
@@ -14,6 +14,7 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
/* | |||
package org.openapitools.codegen.protobuf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
param.dataTypeAlternate = param.dataType; | ||
if (param.isArray) { | ||
param.isUniqueId = param.isUniqueId || param.itemsAreUniqueId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think you can add some unit tests that demonstrate / verify the above mentioned edge cases fixes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix issues for items arrays in some circumstances
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! |
} | ||
}; | ||
|
||
public static init(apiBaseConfig: ConfigurationParameters) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change as the init method signature has changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@wing328 Still awaiting for this pull request to be merged... |
@bflamand thanks for your contribution! |
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. |
return ((ExtendedCodegenProperty)this.items).getItemsDataType(); | ||
}; | ||
return this.items.dataType; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like these helpers are duplicated in the two classes. I'm wondering if it can be extracted/reused somehow, instead of duplicating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code appear identical in both classes, I will look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a fix that reuse the core code similar in both classes for these helpers.
@@ -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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@@ -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}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I didn't realize TS still has issues with generators
param.dataTypeAlternate = param.dataType; | ||
if (param.isArray) { | ||
param.isUniqueId = param.isUniqueId || param.itemsAreUniqueId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think you can add some unit tests that demonstrate / verify the above mentioned edge cases fixes?
…endedCodegenParameter.
@amakhrov apart from missing unit tests (which could be added later), do you think the pr is ready to be merged? |
@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) ...
PR checklist
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.
master
(5.3.0),6.0.x
@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:
Changes affecting only typescript fetch saga&record mode: