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-rxjs] add rxjs 7 support #9958

Merged
merged 9 commits into from
Feb 3, 2022

Conversation

denyo
Copy link
Contributor

@denyo denyo commented Jul 16, 2021

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
Member

@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

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

@macjohnny
Copy link
Member

@denyo ping

@denyo
Copy link
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
Member

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

# 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
@TiFu
Copy link
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
Member

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

@jopelle1
Copy link

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

@trumbitta
Copy link
Contributor

@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

Polm1 commented Jan 19, 2022

Following 👀

@macjohnny
Copy link
Member

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
Member

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

@jopelle1
Copy link

I opened a new PR fixing this one #11364 :

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

@denyo
Copy link
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
Member

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

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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants