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] Default TypeScript fetch configuration to 3.6+ true #9974

Conversation

szTheory
Copy link
Contributor

@szTheory szTheory commented Jul 19, 2021

Related to issue #9973

Here is a PR that changes the default to true. There is one issue however. I'm not sure why it's not updating the value to true in the generated documentation at docs/generators/typescript-fetch.md as well. Any ideas?

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.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.

Tagging the technical committee for review:
@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)

@macjohnny
Copy link
Member

thanks for your contribution.
did you run ./bin/utils/export_docs_generators.sh?
since this is a breaking-change with fallback, we should aim for the next minor release.

Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

LGTM

@macjohnny macjohnny added this to the 5.3.0 milestone Aug 6, 2021
@macjohnny macjohnny changed the base branch from master to 5.3.x August 6, 2021 07:06
Copy link
Contributor

@TiFu TiFu left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Code looks good to me as well.

As mentioned by macjohnny please regenerate the samples or apply the below patch to your code (git apply copy_below_patch_into_this_file.patch).

Patch based on your repository & mvn clean package; ./bin/utils/export_docs_generators.sh

diff --git a/docs/generators/typescript-fetch.md b/docs/generators/typescript-fetch.md
index f8e9cd2328a..8a080239a01 100644
--- a/docs/generators/typescript-fetch.md
+++ b/docs/generators/typescript-fetch.md
@@ -26,7 +26,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
 |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|
+|typescriptThreePlus|Setting this property to true will generate TypeScript 3.6+ compatible code.| |true|
 |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|

@szTheory
Copy link
Contributor Author

Thanks for reviewing this!

@macjohnny yes I did run ./bin/utils/export_docs_generators.sh

@TiFu I followed your instructions, but after running mvn clean package; ./bin/utils/export_docs_generators.sh it clobbers the diff and I'm left with no changes in the git repository when I run git diff. Not sure what is going on. It's like the generated docs are not picking up the change from the source material somehow.

@TiFu
Copy link
Contributor

TiFu commented Aug 15, 2021

@szTheory I would suggest that you just manually edit the file/apply the patch, then commit & push (i.e. skip the mvn clean package stuff). As long as the edits are committed, the tests will pass.

(If you feel like debugging this: what OS do you use? Are you using any IDE that might interfere? Are you running both clean package and export docs from a shell?)

@szTheory
Copy link
Contributor Author

szTheory commented Aug 16, 2021

@TiFu Thanks I applied the patch and pushed the change but a few of the CI tests are still failing.

Edit: I ran the commands (both clean package and export docs) directly from the terminal on Mac.

@TiFu
Copy link
Contributor

TiFu commented Aug 16, 2021

All of the failures seem to be unrelated - I will close & reopen this PR to rerun the tests as it looks like there were some download issues on sonatype's side.

  • The maven plugin tests failed because oai-generator-maven-plugin couldn't be resolved in sonatype.
  • Gradle seems to be related - it's complaining about a missing package
  • CircleCI also could not resolve some random package - caused by pkmst-microservice
  • Appveyor failed due to a missing gradle dependency (same fault as above)

@TiFu TiFu closed this Aug 16, 2021
@TiFu TiFu reopened this Aug 16, 2021
@TiFu
Copy link
Contributor

TiFu commented Aug 17, 2021

@wing328 Can you maybe chime in here? I am a bit unsure where the errors come from, e.g. the below (from maven plugin tests)

Error:  Plugin org.openapitools:openapi-generator-maven-plugin:5.2.0-SNAPSHOT or one of its dependencies could not be resolved: Could not find artifact org.openapitools:openapi-generator-maven-plugin:jar:5.2.0-SNAPSHOT in sonatype-snapshots (https://oss.sonatype.org/content/repositories/snapshots/) -> [Help 1]
Error:  
Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.
Error:  Re-run Maven using the -X switch to enable full debug logging.
Error:  
Error:  For more information about the errors and possible solutions, please read the following articles:
Error:  [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginResolutionException
Error: Process completed with exit code 1.

@macjohnny macjohnny changed the base branch from 5.3.x to master August 25, 2021 06:19
@macjohnny
Copy link
Member

@szTheory can you please merge the most recent master into your branch?

@szTheory
Copy link
Contributor Author

@macjohnny I've merged master and ran the commands to build the project and update samples:

./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh

@macjohnny
Copy link
Member

sorry for the inconvenience, but can you rebase your branch onto the most recent master? I guess you started off from the 5.3 branch

@macjohnny
Copy link
Member

@szTheory ping

@wing328 wing328 modified the milestones: 5.3.0, 5.3.1 Oct 25, 2021
@wing328 wing328 modified the milestones: 5.3.1, 5.4.0 Dec 29, 2021
wing328 added a commit that referenced this pull request Jan 16, 2022
…#9974 (#11331)

* Default TypeScript fetch configuration to 3.6+ true, as it's been out for a while now

* TypeScript fetch update the three plus CLI option default to true

* Apply doc generator patch

* Build the project and update samples

* remove VERSION

* test ts fetch clients first

* Revert "test ts fetch clients first"

This reverts commit 590a7f2.

Co-authored-by: szTheory <szTheory@users.noreply.github.com>
@wing328
Copy link
Member

wing328 commented Jan 16, 2022

Closed via #11331

@wing328 wing328 closed this Jan 16, 2022
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

5 participants