Skip to content
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 Fetch] New optional mode with redux saga & immutablejs (saga & records) #8578

Merged
merged 47 commits into from
Jun 23, 2021

Conversation

bflamand
Copy link
Contributor

Added new generation mode for typescript fetch to support saga & records.

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.1.x, 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)

bflamand-work and others added 30 commits September 29, 2020 15:13
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.
…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.
… 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.
@wing328
Copy link
Member

wing328 commented Feb 1, 2021

$ /bin/bash ./bin/utils/detect_tab_in_java_class.sh
modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptFetchClientCodegen.java:	
Java files contain tab '\t'. Please remove it and try again.

Looks like some changes are using tab instead of spaces. Please have a look.

@wing328
Copy link
Member

wing328 commented Feb 1, 2021

index a7930b9853d..12c49c4b7cb 100644
--- a/docs/generators/typescript-fetch.md
+++ b/docs/generators/typescript-fetch.md
@@ -20,12 +20,12 @@ These options may be applied as additional-properties (cli) or configOptions (pl
 |nullSafeAdditionalProps|Set to make additional properties types declare that their indexer may return undefined| |false|
 |prefixParameterInterfaces|Setting this property to true will generate parameter interface declarations prefixed with API class name to avoid name conflicts.| |false|
 |prependFormOrBodyParameters|Add form or body parameters to the beginning of the parameter list.| |false|
+|sagasAndRecords|Setting this property to true will generate additional files for use with redux-saga and immutablejs.| |false|
 |snapshot|When setting this property to true, the version will be suffixed with -SNAPSHOT.yyyyMMddHHmm| |false|
 |sortModelPropertiesByRequiredFlag|Sort model properties to place required parameters before optional parameters.| |true|
 |sortParamsByRequiredFlag|Sort method arguments to place required parameters before optional parameters.| |true|
 |supportsES6|Generate code that conforms to ES6.| |false|
 |typescriptThreePlus|Setting this property to true will generate TypeScript 3.6+ compatible code.| |false|
-|sagasAndRecords|Setting this property to true will generate additional files for working with redux-saga and immmutablejs.| |false|
 |useSingleRequestParameter|Setting this property to true will generate functions with a single argument containing all API endpoint parameters instead of one argument per parameter.| |true|
 |withInterfaces|Setting this property to true will generate interfaces next to the default class implementations.| |false|
 |withoutRuntimeChecks|Setting this property to true will remove any runtime checks on the request and response payloads. Payloads will be casted to their expected types.| |false|
diff --git a/samples/client/petstore/typescript-fetch/builds/default-v3.0/models/Capitalization.ts b/samples/client/petstore/typescript-fetch/builds/default-v3.0/models/Capitalization.ts
index dbb703aad5e..72152fe0462 100644
--- a/samples/client/petstore/typescript-fetch/builds/default-v3.0/models/Capitalization.ts
+++ b/samples/client/petstore/typescript-fetch/builds/default-v3.0/models/Capitalization.ts
@@ -51,7 +51,6 @@ export interface Capitalization {
     sCAETHFlowPoints?: string;
     /**
      * Name of the pet
-
      * @type {string}
      * @memberof Capitalization
      */
Perform git status
On branch pull/8578
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   docs/generators/typescript-fetch.md
	modified:   samples/client/petstore/typescript-fetch/builds/default-v3.0/models/Capitalization.ts

no changes added to commit (use "git add" and/or "git commit -a")

This script runs in pull requests against the anticipated merge commit (as if the PR was merged now).
When you see unexpected files here, it likely means that there are newer commits in master that you need to 
rebase or merge into your branch.

Please run 'bin/utils/ensure-up-to-date' locally and commit changes (UNCOMMITTED CHANGES ERROR)

Please run bin/utils/ensure-up-to-date locally and commit the changes.

bflamand and others added 14 commits February 8, 2021 09:56
…lysis time for consuming project. Removed tab characters in mustache files. Reformat code for TypeScriptFetchClientCodegen to try to remove false positive for tabs vs spaces.
…pt version to address some typing errors on library build.
… ensure proper typechecking info is generated.
…t to convert an entity to an inlined model recursively.
…utablejs

# Conflicts:
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptFetchClientCodegen.java
@bflamand
Copy link
Contributor Author

bflamand commented Apr 20, 2021

@wing328 Can you please merge this!? I finally managed to pass all the checks!! (the travis-ci check has an error only because of the docker rate limitting problem, but I know that it also passes successfully)

@wing328
Copy link
Member

wing328 commented Apr 21, 2021

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) for another review.

@@ -0,0 +1,16 @@
generatorName: typescript-fetch
outputDir: samples/client/petstore/typescript-fetch/builds/sagas-and-records
inputSpec: modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing-saga-and-records.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use OpenAPI 3.0 spec instead as we're doing the same for other samples from openapi 2.0 to 3.0 spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this can wait, I would prefer to take a look at this at a later time. I am not sure of the implications of changing this. It is probably very easy, but I am not sure. Would prefer to proceeed with a first merge and upgrade in a next phase. See my additional comments below regarding the futur plan for this upgrade.

@wing328
Copy link
Member

wing328 commented Apr 21, 2021

I noticed lots of new functions/codes are added to modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptFetchClientCodegen.java. I wonder if you can add more comments to explain those code blocks.

@bflamand
Copy link
Contributor Author

bflamand commented Apr 21, 2021

Here is some additional information about this upgrade:

This upgrade is almost a new generatror. It will generate files for supporting immutablejs data structure and redux-saga patterns. This is built on-top of typescript files generated. It is currently packaged together with typescript fetch because this was the easiest way obtain a fully working generator by leveraging the existing generated code and adding another layer on top of it. It is also usefull for any code base, like ours, that wants to transition to the new layer. Some of our code still uses the original typescript fetch layer (which remains unchanged) and we can progressively transition to the saga&recopr layer since both are outputted when using the sagaAndRecord flag.

In a subsequent phase, I plan to create a fully stand alone generator that will only generate files needed for the saga&record layer directly (with a little bit of low level code using fetch, similar to what typesdcript fetch uses). But this might be a huge task, so we are proceeding with this intermediate step for now.

This upgrade is completely backward compatible and 100% optional: I did not modified the behavior of typescript fetch in any way whatsoever. Great care was taken to wrap all potential changes with a "if (sageAndRecord)", please tell me if you see a problem somewhere! I do not want to affect the existing typescript fetch. If the saga&recor switch is "false" typescript fetch should behave exactly as before with zero changes.
@wing328

@bflamand
Copy link
Contributor Author

bflamand commented Apr 21, 2021

BTW, we have been testing the saga&record output for the last several months in our production code base now and it works really well. We made a few tweaks here and there along the way. The code and its output is now pretty stable. I am hoping we can merge this as-is if no issues are found and then we can proceed in a more incremental way for future changes. My intention is definitively to add more comments throught the code, and potentially eventually split this in a new generator. Rest assured, I intend to support this for the foreseeable future.
@wing328

@macjohnny
Copy link
Member

I restarted the build, lets see if it succeeds. Then we could merge it (although I didnt have a detailed look at the code)

@bflamand
Copy link
Contributor Author

Alright, standing by to fix any issues if something goes wrong...

@bflamand
Copy link
Contributor Author

bflamand commented Apr 21, 2021

@macjohnny I think we are good to go... ?

@bflamand
Copy link
Contributor Author

bflamand commented Jun 16, 2021

Hello @macjohnny or @wing328 , I have been waiting for a while for this to be merged... Could you please try to merge this when you have time. Thank you!

@wing328 wing328 added this to the 5.2.0 milestone Jun 23, 2021
@wing328 wing328 merged commit 48e05ce into OpenAPITools:master Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants