fix: Fix generating wrong web-rpc implementation for wrapper-type method arg

[programmador]

Fixes #862
Though I didn't see any difference in generated wrapper related rpc definition code when switching between different useOptionals values.

[stephenh]

Hi @programmador ; this looks great! I'm kinda surprised, the build passed without any changes to our integration tests.

Usually for PRs, we try to find an example/test project in our integration/ test directory, like:

https://github.com/stephenh/ts-proto/tree/main/integration/grpc-web

And then update the *.proto files to trigger the condition that was broken, and then check the new generated code into the repo / as part of the PR. (Usually checking in generated code is kind of odd, but give that is all ts-proto does, including the generated code in PRs is a nice way of visualizing what has actually changed in the output and that we don't have any regressions).

Basically, can you look through the grpc-web integration tests for a proto file that is close to what you're fixing, and update it / add a method that includes a wrapper type as a request message?

We probably have some tests that are already returning wrapper types as response messages, but not request messages...but not sure.

This will just ensure your fix doesn't accidentally regress if/when things are updated in the future.

Thanks!

[programmador]

While running yarn test in provided docker-compose environment lots are failing with the same error:

    integration/simple/simple-oneofs-test.ts:3:32 - error TS2307: Cannot find module './pbjs' or its corresponding type declarations.

Should I undertake anything or just ignore them?
It should be protobufjs as I see. Strange that some of the tests pass and the others don't

[programmador]

While running yarn test in provided docker-compose environment lots are failing with the same error:

    integration/simple/simple-oneofs-test.ts:3:32 - error TS2307: Cannot find module './pbjs' or its corresponding type declarations.

Should I undertake anything or just ignore them? It should be protobufjs as I see. Strange that some of the tests pass and the others don't

O that's ok now, I just had to run few protoc commands as mentioned in package.json

[programmador]

As I see grpc-web tests do not generate any TypeScript rpc definitions. I've added an rpc method which both receives and returns a StringValue wrapper. Then I've re-run commands specified by build:test alias. And then re-run tests. Nothing changed except example.bin. Moreover I've tried to re-run tests on main branch and nothing has failed. Either there are no rpc checks (only messages themselves are covered) or I don't understand how these tests work.

[programmador]

Anyway I've pushed my changes - please look whether it's what was required or not.
a6f9274

[stephenh]

@programmador this looks great! Can you go ahead and check in the generated code changes? Having those in the PR is a lot what our "tests" are.

[programmador]

@programmador this looks great! Can you go ahead and check in the generated code changes? Having those in the PR is a lot what our "tests" are.

Sorry, it seems like the more we conversate the more I realize I do not understand. There is no generated code produced by grpc-web tests. The only generated thing is example.bin. So at changes of what do I have to look?

Maybe the code is generated inside of tests and is not being saved anywhere so I have to dump something to stdout in test code?

[programmador]

If You're asking about my project's code - so here's a contract:

enum BlockType {
  BLOCK_TYPE_NONE = 0;
  BLOCK_TYPE_LITE = 1;
  BLOCK_TYPE_FULL = 2;
}

message BlockPrediction {
  BlockType type = 1;
}

...

service RiskService {
  ...
  rpc PredictBlock(google.protobuf.StringValue) returns (models.BlockPrediction) {};
}

Here's a generated code before fix (1.151.1):

  PredictBlock(request: DeepPartial<StringValue>, metadata?: grpc.Metadata): Observable<BlockPrediction> {
    return this.rpc.unary(RiskServicePredictBlockDesc, string.fromPartial(request), metadata);
  }

And here's one after the fix:

  PredictBlock(request: DeepPartial<StringValue>, metadata?: grpc.Metadata): Observable<BlockPrediction> {
    return this.rpc.unary(RiskServicePredictBlockDesc, StringValue.fromPartial(request), metadata);
  }

[programmador]

Older version of ts-proto (^1.93.2 though I don't know which exactly without ^ sign) generated even more strange code:

  PredictBlock(request: DeepPartial<StringValue>, metadata?: grpc.Metadata): Observable<BlockPrediction> {
    return this.rpc.unary(RiskServicePredictBlockDesc, string | undefined.fromPartial(request), metadata);
  }

So we have 3 variants of substitution before .fromPartial():

• string | undefined
• string
• StringValue

where the latest one is correct.

[stephenh]

Hi @programmador , sorry, I just meant the changes to the example.ts file that came from your addition of the Uppercase method.

I went and pushed that commit to this PR, and it shows the changes to the output are:

  Uppercase(request: DeepPartial<StringValue>, metadata?: grpc.Metadata): Promise<StringValue> {
    return this.rpc.unary(DashAPICredsUppercaseDesc, StringValue.fromPartial(request), metadata);
  }

Is that what you expect for the Uppercase method? If so, we can go ahead and merge this PR. Thanks!

[stephenh]

Fwiw I think ideally this code would end up being:

  Uppercase(request: string, metadata?: grpc.Metadata): Promise<string> {
    return this.rpc.unary(DashAPICredsUppercaseDesc, StringValue.wrap(request), metadata).then(response => StringValue.unwrap(response));
  }

Or something like that, I forget what the StringValue.wrap/unwrap methods are/don't see them in the output at the moment, but then the user wouldn't even know about the StringValue wrapper types.

Happy to leave that for another PR though, because it'd be a more complicated change.

[programmador]

Fwiw I think ideally this code would end up being:

  Uppercase(request: string, metadata?: grpc.Metadata): Promise<string> {
    return this.rpc.unary(DashAPICredsUppercaseDesc, StringValue.wrap(request), metadata).then(response => StringValue.unwrap(response));
  }

Or something like that, I forget what the StringValue.wrap/unwrap methods are/don't see them in the output at the moment, but then the user wouldn't even know about the StringValue wrapper types.

Happy to leave that for another PR though, because it'd be a more complicated change.

Yes, I thought about whether rpc signature should work just with scalar values. And realized that wrappers are currently supported in ts-proto at protobuf(lower) level but not at rpc(upper) level. So i decided not to go so far, at least not in this PR. And also when I'm not sure how it's meant to be. Though now I understand what we see it from the same angle.

UPD: that other PR should also respect optionals setting

[programmador]

Hi @programmador , sorry, I just meant the changes to the example.ts file that came from your addition of the Uppercase method.

I went and pushed that commit to this PR, and it shows the changes to the output are:

  Uppercase(request: DeepPartial<StringValue>, metadata?: grpc.Metadata): Promise<StringValue> {
    return this.rpc.unary(DashAPICredsUppercaseDesc, StringValue.fromPartial(request), metadata);
  }

Is that what you expect for the Uppercase method? If so, we can go ahead and merge this PR. Thanks!

Exactly! But how did You make that change? By hand? I thought it should be generated automatically either when I run tests (I don't know how they should work here) or by running protoc scripts from yarn 'build:test' job. Tried both - and nothing generated the code.

[stephenh]

But how did You make that change? By hand?

Ah yeah, sorry, I ran yarn bin2ts

[stephenh]

🎉 This PR is included in version 1.165.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release
• v1.165.2

Your semantic-release bot 📦🚀