Add support for generating grpc-node service types #193

esilkensen · 2019-08-05T06:54:23Z

Changes

This PR adds support for generating grpc-node service types (see #75, #42, #27):

The existing service=true option has been replaced with service=grpc-web, following the suggestion in Ability to generate typings for grpc-node service files #75 (comment).
The new service=grpc-node option can be used to generate grpc-node service types, writing a _grpc_pb.d.ts file for each _grpc_pb.js file generated by the protoc-gen-grpc plugin.

Verification

To facilitate testing of the service options and the new service=grpc-node option in particular, the generate.sh script has been modified following #75 (comment):

examples/generated contains the output when the service option is not used
examples/generated-grpc-web contains the output for the service=grpc-web option
examples/generated-grpc-node contains the output for the service=grpc-node option

The generate.sh script uses grpc-tools to generate _grpc_pb.js implementation files to help test that the generated _grpc_pb.d.ts files are consistent with them.

esilkensen · 2019-08-09T07:28:56Z

examples/generated-grpc-node/proto/examplecom/simple_service_grpc_pb.d.ts

+
+export const SimpleServiceService: ISimpleServiceService;
+
+export class SimpleServiceClient extends grpc.Client {


Here's an example of one of the generated grpc-node service types

esilkensen · 2019-08-09T07:32:01Z

package.json

    "lodash": "^4.17.5",
    "lodash.isequal": "^4.5.0",
    "minimist": "^1.2.0",
    "mocha": "^5.2.0",
    "mocha-spec-json-output-reporter": "^1.1.6",
    "source-map-support": "^0.4.18",
-    "ts-node": "^5.0.1",
+    "ts-node": "^8.3.0",


As I was working on this, it seemed that ts-node 5.0.1 wasn't catching type errors in test/integration/service/grpcnode.ts - I tried updating to the latest 8.3.0 and then it did catch them and would cause npm run test to fail, as expected.

esilkensen · 2019-08-09T07:35:06Z

package.json

@@ -46,13 +46,15 @@
    "babel": "^6.5.2",
    "browser-headers": "^0.4.1",
    "chai": "^3.5.0",
+    "grpc": "^1.22.2",


Is this package ok here in "devDependencies" even though it's imported in the generated _grpc_pb.d.ts files?

Yep should be fine, I see you are only consuming the grpc package in your tests (hence a dev dependency) so shouldn't cause an issue for consumers.

esilkensen · 2019-08-09T07:39:22Z

src/service/common.ts

+  }
+}
+
+export class GrpcServiceDescriptor {


This file is unchanged code extracted from grpcweb.ts - except for renaming this class from GrpcWebServiceDescriptor to GrpcServiceDescriptor since it wasn't necessarily specific to grpc-web.

esilkensen · 2019-08-09T07:40:27Z

test/mocha-run-suite.sh

@@ -19,7 +19,7 @@ fi
 mocha \
    --reporter mocha-spec-json-output-reporter \
    --reporter-options "fileName=./test/mocha-report.json" \
-    --require ts-node/register/type-check \
+    --require ts-node/register \


type-check is the default behavior in the newer ts-node

esilkensen · 2019-08-09T08:05:02Z

Hi @jonny-improbable 👋

I wondered if this is still a feature you think is worth pursuing?

If so, I think this PR is potentially ready for a review. Whenever you have a chance to take a look, if there's anything I can do to help make it easier to review, I will be happy to do that.

(I wondered about breaking off an initial PR that did nothing but adding that examples/grpc-web-services directory, in the interest of shrinking the diff...)

Anyway, sorry to @ ping you - I'll look forward to any next steps to take with this PR, and of course will understand if this just isn't a path for this project to go down.

Cheers!

jonny-improbable

Nice, I like your approach; and from experience I can imagine this was a lot of work!

Some thoughts in no particular order.

Migration Path; I feel that it's important we provide a migration path for users; if we land this change as is, ts-protoc-gen with the service=true flag will no longer emit gprc-web client stubs which may be confusing for consumers. Can I suggest you add some logic to detect the usage of service=true, and emit a prominent message that this functionality will be deprecated and the consumer must change their usage of protoc.
Ownership; are you able to commit to triaging issues that may arise from people using this code? I am not a consumer of 'node grpc' myself so will need help here.
Placement; see RFC: The future of this package #145, I would actually prefer if this package went away however as you can see I haven't made much progress on this issue (and it got automatically closed, sorry about that!). Note that https://github.com/agreatfool/grpc_tools_node_protoc_ts appears to already provide gprc-node service creation; perhaps that package provides all the functionality required in which case encouraging users to migrate to https://github.com/agreatfool/grpc_tools_node_protoc_ts may be the better option once protoc-gen-improbable-eng-grpc-web Hello World grpc-web#368 is merged...

Please let me know your thoughts on the above, and sorry for the slow turn around.

jonny-improbable · 2019-08-14T16:16:49Z

package.json

@@ -46,13 +46,15 @@
    "babel": "^6.5.2",
    "browser-headers": "^0.4.1",
    "chai": "^3.5.0",
+    "grpc": "^1.22.2",


Yep should be fine, I see you are only consuming the grpc package in your tests (hence a dev dependency) so shouldn't cause an issue for consumers.

jonny-improbable · 2019-08-14T16:17:49Z

src/index.ts

        generateGrpcWebService(outputFileName, fileNameToDescriptor[fileName], exportMap)
          .forEach(file => codeGenResponse.addFile(file));
      }
+      if (generateGrpcNodeServices) {


This should be else if to reflect the fact that it's not possible to emit both grpc-web and grpc-node services at the same time.

jonny-improbable · 2019-08-14T16:21:06Z

test/integration/service/grpcweb.ts

@@ -44,31 +44,40 @@ describe("service/grpc-web", () => {
      assert.strictEqual(SimpleService.DoClientStream.responseType, Empty);
    });

-    it("should contain the expected  DoClientStream method", () => {
+    it("should contain the expected DoBidiStream method", () => {


esilkensen · 2019-08-15T07:20:28Z

Thanks Jonny!

Migration Path; I pushed a change that preserves the behavior for service=true, but logs a warning that it has been deprecated and service=grpc-web should be used instead.
Ownership; I can definitely commit to this. I would be happy to jump on any issues, and also would be using this code personally at work.
Placement; Ah yes, I was looking at RFC: The future of this package #145 and grpc_tools_node_protoc_ts (and this fork of it too) as I was getting started on this PR. I think what you've laid out makes a lot of sense.

I ended up going down this path because:

I really like the test/CI coverage in ts-protoc-gen. grpc_tools_node_protoc_ts doesn't have any; I think code was copied from ts-protoc-gen, but certain things like Handling of enums #156 haven't made it in.
We're already using ts-protoc-gen, and recently had reason to start using grpc-node services. It'd be super easy to just bump a version number and pass a new service parameter.

Do you think there's a case for adding this functionality to ts-protoc-gen, for however much current users of the package may get out of it, with the understanding that you would still be pursuing #145 and these grpc-node services wouldn't be part of the plans going forward?

If so, I'd reiterate my commitment to owning any issues related to this code. But if not, I certainly understand, and could look at contributing and migrating to that other project instead.

(Thanks for the pointer to that ts-morph library, looks cool!)

jonny-improbable · 2019-08-16T07:41:03Z

Morning @esilkensen,

Thanks for answers my questions above; it looks like we are all set! Sorry to be a pain, but I've just landed #195 which has caused a number of conflicts, in particular around the way the example protos are generated. Please could you resolve these and then we will be good to merge.

esilkensen · 2019-08-16T08:35:21Z

Sounds good, thanks so much @jonny-improbable,

I've just resolved the conflicts and will watch to make sure CI passes.

(Then head to bed, it is early morning for me 😴)

esilkensen · 2019-08-23T13:30:15Z

Hi @jonny-improbable, I just wanted to check back in on this PR, to see if you were still thinking of merging it -- or if there are any additional changes I could make? Thanks again!

jonny-improbable

Let's do this.

esilkensen · 2019-08-30T07:24:58Z

Nice!

What do you think about versioning? Will this eventually make it into a 0.10.1 release?

(I'll pull from ts-protoc-gen@next in the meantime)

jonny-improbable · 2019-08-30T08:47:26Z

0.x versioning is a little hectic, I'll just bump this out at 0.11 as it contains a major new feature.

…

On Fri, 30 Aug 2019 at 08:24, Erik Silkensen ***@***.***> wrote: Nice! What do you think about versioning? Will this eventually make it into a 0.10.1 release? (I'll pull from ***@***.*** in the meantime) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#193?email_source=notifications&email_token=ADMBV4L5VALSDHADYKUUQJ3QHDDMXA5CNFSM4IJIA3VKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5QZZ6I#issuecomment-526490873>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADMBV4IRWU7LZBKSRFDNDRDQHDDMXANCNFSM4IJIA3VA> .

esilkensen added 4 commits August 5, 2019 09:02

Run protoc twice, with and without service=true

1d0e43d

Use service=grpc-web instead of service=true

ca3d035

Extract common code from service/grpcweb.ts

db86200

Add support for service=grpc-node option

74589a7

esilkensen force-pushed the grpc-node-services branch 3 times, most recently from 1f95c2c to c66fcde Compare August 5, 2019 21:59

Update yarn.lock

ee6a3da

esilkensen force-pushed the grpc-node-services branch from c66fcde to ee6a3da Compare August 5, 2019 22:27

Add additional grpcnode.ts tests

a6fdf5d

esilkensen force-pushed the grpc-node-services branch from 09fc784 to a6fdf5d Compare August 6, 2019 09:14

Tests ready for review

857d3ce

esilkensen marked this pull request as ready for review August 9, 2019 06:59

esilkensen commented Aug 9, 2019

View reviewed changes

jonny-improbable reviewed Aug 14, 2019

View reviewed changes

esilkensen added 2 commits August 14, 2019 16:22

Warn when using deprecated service=true parameter

77c1218

Merge remote-tracking branch 'upstream/master' into grpc-node-services

6de0a87

Merge remote-tracking branch 'upstream/master' into grpc-node-services

9ebc5ac

jonny-improbable approved these changes Aug 29, 2019

View reviewed changes

jonny-improbable merged commit 005b479 into improbable-eng:master Aug 29, 2019

esilkensen deleted the grpc-node-services branch August 30, 2019 06:40

esilkensen mentioned this pull request Aug 31, 2019

Generate .d.ts files for node gRPC services namely/docker-protoc#141

Merged

jonny-improbable mentioned this pull request Oct 22, 2019

Developing ts-protoc-gen on windows #79

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for generating grpc-node service types #193

Add support for generating grpc-node service types #193

esilkensen commented Aug 5, 2019 •

edited

Loading

esilkensen Aug 9, 2019

esilkensen Aug 9, 2019

esilkensen Aug 9, 2019 •

edited

Loading

jonny-improbable Aug 14, 2019

esilkensen Aug 9, 2019

esilkensen Aug 9, 2019

esilkensen commented Aug 9, 2019

jonny-improbable left a comment

jonny-improbable Aug 14, 2019

jonny-improbable Aug 14, 2019

jonny-improbable Aug 14, 2019

esilkensen commented Aug 15, 2019 •

edited

Loading

jonny-improbable commented Aug 16, 2019

esilkensen commented Aug 16, 2019

esilkensen commented Aug 23, 2019

jonny-improbable left a comment

esilkensen commented Aug 30, 2019

jonny-improbable commented Aug 30, 2019 via email


		export const SimpleServiceService: ISimpleServiceService;

		export class SimpleServiceClient extends grpc.Client {

Add support for generating grpc-node service types #193

Add support for generating grpc-node service types #193

Conversation

esilkensen commented Aug 5, 2019 • edited Loading

Changes

Verification

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esilkensen Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esilkensen commented Aug 9, 2019

jonny-improbable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esilkensen commented Aug 15, 2019 • edited Loading

jonny-improbable commented Aug 16, 2019

esilkensen commented Aug 16, 2019

esilkensen commented Aug 23, 2019

jonny-improbable left a comment

Choose a reason for hiding this comment

esilkensen commented Aug 30, 2019

jonny-improbable commented Aug 30, 2019 via email

esilkensen commented Aug 5, 2019 •

edited

Loading

esilkensen Aug 9, 2019 •

edited

Loading

esilkensen commented Aug 15, 2019 •

edited

Loading