Skip to content

[typescript-rxjs] add rxjs 7 support#9958

Merged
macjohnny merged 9 commits intoOpenAPITools:masterfrom
denyo:feature/rxjs-7
Feb 3, 2022
Merged

[typescript-rxjs] add rxjs 7 support#9958
macjohnny merged 9 commits intoOpenAPITools:masterfrom
denyo:feature/rxjs-7

Conversation

@denyo
Copy link
Copy Markdown
Contributor

@denyo denyo commented Jul 16, 2021 •
edited
Loading

This PR adds support for rxjs 7 (https://github.com/ReactiveX/rxjs/blob/master/CHANGELOG.md) which introduces some breaking interface changes but brings both better minification and improved performance.

Also type imports are refactored to type-only imports.

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.

@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
Copy Markdown
Member

macjohnny commented Aug 6, 2021

@denyo thanks for your contribution. would there be a possibility to support RxJS 7 in a backwards compatible way? maybe introduce an option or version flag, similar to

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/TypeScriptAngularClientCodegen.java

Line 121 in c4b9804

this.cliOptions.add(new CliOption(NG_VERSION, "The version of Angular. (At least 6.0.0)").defaultValue(this.ngVersion));
?

@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Sep 29, 2021

@denyo ping

@denyo
Copy link
Copy Markdown
Contributor Author

denyo commented Sep 30, 2021

@macjohnny I won't get the chance to make it backwards compatible.
Therefore I would suggest to change the target branch to the next major version.

@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Sep 30, 2021

@wing328 what is your opinion here? should we break the existing rxjs generator, or should we create a new rxjs7 generator?

@denyo
Merge branch 'master' into feature/rxjs-7
b1997ea 
# Conflicts:
#	modules/openapi-generator/src/main/resources/typescript-rxjs/apis.mustache
#	modules/openapi-generator/src/main/resources/typescript-rxjs/runtime.mustache
#	samples/client/petstore/typescript-rxjs/builds/default/runtime.ts
#	samples/client/petstore/typescript-rxjs/builds/es6-target/runtime.ts
#	samples/client/petstore/typescript-rxjs/builds/with-npm-version/runtime.ts
#	samples/client/petstore/typescript-rxjs/builds/with-progress-subscriber/runtime.ts
@denyo denyo changed the base branch from master to 6.0.x October 1, 2021 08:49
@denyo denyo requested a review from jimschubert as a code owner October 26, 2021 12:50
@TiFu
Copy link
Copy Markdown
Contributor

TiFu commented Nov 13, 2021

Quick question: Why does this PR affect 4,359 files? Looks like some unrelated commits have been included.

@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Nov 13, 2021

@TiFu I guess this is because of the changed target branch

@jopelle1
Copy link
Copy Markdown

jopelle1 commented Jan 19, 2022

Any update on this PR ? Rxjs 7 support would be very useful

@trumbitta
Copy link
Copy Markdown
Contributor

trumbitta commented Jan 19, 2022

@macjohnny would it possible to release this as is and let people stuck with rxjs 6 (rxjs 8 is coming) just point to an old version of the generator by themselves?

@Polm1
Copy link
Copy Markdown

Polm1 commented Jan 19, 2022

Following 👀

@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Jan 19, 2022

According to https://github.com/OpenAPITools/openapi-generator#11---compatibility, we can have breaking changes only for a major release.
If an option is introduced, we can also habe it for the current minor or patch release

@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Jan 19, 2022

@denyo can you rebase onto the 6.0 branch to avoid the unrelated changes?

@jopelle1 jopelle1 mentioned this pull request Jan 20, 2022
Closed
5 tasks
@jopelle1
Copy link
Copy Markdown

jopelle1 commented Jan 20, 2022

I opened a new PR fixing this one #11364 :

  • Rebased on 6.0x
  • Implemented option to switch to rxjs 7

@denyo
Copy link
Copy Markdown
Contributor Author

denyo commented Jan 21, 2022

@macjohnny done.

For everyone else waiting, this is the docker image built from this branch: denyo/openapi-generator-cli:6.0.0-test

@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Jan 24, 2022

I would suggest to close this PR in favor of #11364, which we could have in 5.4, and thus avoid merge conflicts in the future.
Any objections?

@denyo
Copy link
Copy Markdown
Contributor Author

denyo commented Jan 24, 2022

I would suggest to close this PR in favor of #11364, which we could have in 5.4, and thus avoid merge conflicts in the future. Any objections?

The only concern I have is that complexity will increase in the generator with every new version of rxjs, especially within templates, e.g. by adding a toggle or extending the flag for every version and at the same time users need to update their flags/toggles, too.

From a user's perspective I would expect to get the latest version by default and have an opt-in for older versions if I really wanna be backwards compatible. At least this is what I am used to.

However the philosophy of this project might be different.

@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Jan 24, 2022

@denyo we can remove the rxjs<7 support for the 6.0 release, which is an easy thing to do an removes the complexity in the template

@macjohnny macjohnny added this to the 6.0.0 milestone Feb 3, 2022
@macjohnny macjohnny changed the base branch from 6.0.x to master February 3, 2022 08:51
@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Feb 3, 2022

since the next release will be 6.0.0, lets merge this PR

@macjohnny macjohnny merged commit dc1df25 into OpenAPITools:master Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimschubert jimschubert Awaiting requested review from jimschubert

Assignees

No one assigned

Labels

Breaking change (without fallback) Client: TypeScript

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@denyo @macjohnny @TiFu @jopelle1 @trumbitta @Polm1