-
Notifications
You must be signed in to change notification settings - Fork 172
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
Add Server interface #247
Add Server interface #247
Conversation
This approach provides between type compatibility between grpc and grpc-js packages
@badsyntax what's the status of this PR? 🤔 |
@IchordeDionysos unfortunately i'm still waiting on these changes to be released: grpc/grpc-node#1587 i need those changes to build a generic service interface that will work with both |
grpc/grpc-node#1587 is published so this PR is good for a review! Any thoughts about this @MarcusLongmuir? |
@MarcusLongmuir Any thoughts about this one? It's not a huge change but would be helpful for people i reckon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Minor note about the dependency, but otherwise LGTM 👍
package.json
Outdated
@@ -33,6 +33,7 @@ | |||
"google-protobuf": "^3.6.1" | |||
}, | |||
"devDependencies": { | |||
"@grpc/grpc-js": "^1.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably best expressed in the README rather than this package specifying it as a dev dependency as that might not get noticed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding it as a dev dependency was intended to make it easier to test the generated types, but it's not required by the package so I have remove it.
I've also added a compatibility note to the README.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MarcusLongmuir The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks for the merge @MarcusLongmuir. Is there an ETA for a release? |
Apologies for the delay. We've just been overhauling a lot of permissions and repo configurations that made this rather laborious. This has gone out in |
Awesome thank you! |
Unless i'm completely missing something, it's not possible to use the types generated by this package for a server implementation. This change fixes that.
I've had to make various upstream changes to allow the types to work across the
grpc
and@grpc/grpc-js
packages. I've been following & contributing to theproto-loader
typescript generator changes in grpc/grpc-node#1474, and changes in this PR align with those changes.If using
@grpc/grpc-js
, a minimum version of1.2.0
is required. I'm not sure how best to handle this, whether we would mention this in the readme, or add it as apeerDependency
?This PR is a draft because there's a mismatch of types between thegrpc
and@grpc/grpc-js
packages. The code in this PR will generate types that only work with@grpc/grpc-js
. I hope to get come clarity on the types mismatch soon.UPDATE: Waiting on the latest type changes to @grpc/grpc-js to be published, to support this change.UPDATE 2: We still need the types to match ingrpc
for this to work.Changes
Example usage
Verification
I've used the generated files in an example app and they seem ok. I briefly explored writing tests but all the existing tests are testing the javascript implementation and not the generated types, so not sure how best to create tests for typescript interfaces.
I believe we can ignore the the failing build (linting) as this new code is consistent with existing code.
Related
#221