Skip to content

Upgrate gRPC to 1.13 and typescript to 2.9 #35

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

Closed
wants to merge 11 commits into from
Closed

Upgrate gRPC to 1.13 and typescript to 2.9 #35

wants to merge 11 commits into from

Conversation

FedeBev
Copy link

@FedeBev FedeBev commented Jul 15, 2018

In this pull request I updated:

  • gRPC to version 1.13: this version has 2 deprecation (grpc.load and addProtoService) that have been fixed
  • typescript to the version 2.9: for gRPC 1.13 compatibility
  • rxjs has been updated to version 6 by @kondi (rxjs6 branch - commit e70eaa6)

@ksaldana1
Copy link
Contributor

Thanks for the PR @FedeBev ! I can't give you too detailed of a code review--I'll defer to @kondi there. We are using rxjs-grpc pretty heavily in our Node layer to serve as a bit of a JSON proxy/aggregator for our gRPC service layer. I have been maintaining my own fork internally (mostly for Metadata support).

I pulled your changes into my fork and made the various upgrades to rxjs 6 in our Node libraries. Everything worked without much issue. I will let you know if I run into any issues going forward, but for now I can serve as an additional confirmation that the upgrades are working as expected.

@kondi
Copy link
Owner

kondi commented Jul 18, 2018

Hi!
Thanks for the contribution.

  • You can already use grpc 1.13 version, the current dependency is only a peerDependency, and only set as a minimum version (min 1.1.2). Did I miss something? I know there is a deprecation warning, but it is working.
  • Does it help something (from the user point of view) if the project is compiled with TypeScript 2.9?
  • If you need the rxjs6 support, you can use the rxjs6 prerelease version (you have actually merged that). Its API is the same as the current 0.1.7 version except for the rxjs peerDependency upgrade.

If I would bump the grpc version requirement, it would be a breaking change, so I have to bump the major version. For the next major version I already plan to change a bit the API and I do not want to introduce multiple breaking changes separately.

@FedeBev
Copy link
Author

FedeBev commented Jul 18, 2018

  • Yes, it works with warnings but I think it's bad when you use a library that introduces warnings not under your control and the 1.1 version if very very old (something like 2 years? Many improvements have been made throughout this time)

  • You need a TS upgrade for some new gRPC generic functions compilation in version 1.13, maybe the 2.5 is enough but I have not tried.

  • Yes, I can

What is the problem of increasing a major?
In my opinion you could do a major with rxjs6 and gRPC 1.13 and in the future a second one with the new changes you want to make. But this is only my opinion

@MOZGIII
Copy link

MOZGIII commented Sep 6, 2018

rxjs-grpc/package.json

Lines 29 to 37 in 0e0f8d1

"dependencies": {
"bluebird": "^3.4.7",
"grpc": "^1.1.2",
"jscodeshift": "^0.3.30",
"minimist": "^1.2.0",
"mz": "^2.6.0",
"protobufjs": "~6.6.5",
"tmp": "^0.0.31"
},

grpc is actially not a peer, but a full dependency. Should probably be made peer though...

Regarding major version bumps, according to semver, pre 1.0.0 releases can be backward incompatible however they want, so just a minor bump is enough here. However, a major version bump (to 1.x.x), according to semver, would mean that this project became stable and will not break the API compatibility within the following minor/patch releases.
In either way, it's normally not up to any third-party to bump project versions, or trying to forcing it. It's simply rude, and complicates the merging of the actual useful parts of the PRs.

Overall, even though this PR made some useful changes (like bumping grpc version and switching to @grpc/proto-loader - this is a reasonable change, since relying on a deprecated API is not a good idea), it contains too much, and I'd recommend splitting it into multiple individual PRs to ease the review and merge process. That way at least some changes from this PR can be merged easily, without debating over the need to introduce them.

@kondi
Copy link
Owner

kondi commented Mar 5, 2019

Released in v0.2.0.

@kondi kondi closed this Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants