MethodDescriptor Introduced in #567 is broken

[clehene]

/cc @stanley-cheung
I'm running with --grpc-web_out=import_style=commonjs+dts,mode=grpcweb

Here's a generated MethodDescriptor: note optin_pb.OptInRequest instead of proto.x.protobuf.v1alpha2.OptInRequest

/**
 * @const
 * @type {!grpc.web.MethodDescriptor<
 *   !proto.x.protobuf.v1alpha2.OptInRequest,
 *   !proto.google.protobuf.Empty>}
 */
const methodDescriptor_OptInService_OptIn = new grpc.web.MethodDescriptor(
  '/x.protobuf.v1alpha2.OptInService/OptIn',
  grpc.web.MethodType.UNARY,
  optin_pb.OptInRequest,
  google_protobuf_empty_pb.Empty,
  /** @param {!proto.x.protobuf.v1alpha2.OptInRequest} request */
  function(request) {
    return request.serializeBinary();
  },
  google_protobuf_empty_pb.Empty.deserializeBinary
);

I'm unable to compile grpc-web anymore (no build instructions either..), however the problem seems around these lines

https://github.com/grpc/grpc-web/pull/567/files#diff-2e729f9088e537bfeb6fefc318963a96R1078
https://github.com/grpc/grpc-web/pull/567/files#diff-2e729f9088e537bfeb6fefc318963a96R1389

This line could use a comment to explain what's the goal of method->output_type()->file() != file

if (import_style == ImportStyle::COMMONJS &&
            method->output_type()->file() != file) {

[buehler]

Yep, I'm also experiencing this error. It seemed to work in 1.0.4

[travikk]

Same here, used to work with 1.0.4

[stanley-cheung]

Sorry for being late to look into this.

I just looked into this - but it appears that the issue may run deeper than I thought. At first I thought it's some DOA issue that borking everything but it's not.

I run the 1.0.5 plugin on the helloworld example and it works. Then I realized the helloworld example uses --grpc-web_out=import_style=commonjs.

So I tried the Echo ts-example next, since that uses --grpc-web_out=import_style=commonjs+dts. But it works too. And I verified that the generated code (from the 1.0.5 plugin) echo_grpc_web_pb.js has the new MethodDescriptor stuff, like

const methodDescriptor_EchoService_Echo = new grpc.web.MethodDescriptor(                                      
  '/grpc.gateway.testing.EchoService/Echo',                                                                   
  grpc.web.MethodType.UNARY,                                                                                  
  proto.grpc.gateway.testing.EchoRequest,                                                                     
  proto.grpc.gateway.testing.EchoResponse,                                                                    
  /** @param {!proto.grpc.gateway.testing.EchoRequest} request */                                             
  function(request) {                                                                                         
    return request.serializeBinary();                                                                         
  },                                                                                                          
  proto.grpc.gateway.testing.EchoResponse.deserializeBinary                                                   
); 

So I wonder what's the difference between your .proto and my .proto.

Can you post your relevant section of the .proto, perhaps the package name, and the relevant RPC method? I am now thinking perhaps this has to do with a specific case where the request is in a different proto package than the service itself?

[buehler]

Hey stanley, I'm going to have a look at the scenario as soon as possible.
I believe it has something to do with imports.

I got a models.proto without any service, and a svc.proto that imports that models proto. It happens with those models.

When the service uses the models (with package name ofcourse)

models:

package models;

message Test{}

svc.proto

package svc;

import "models.proto";

message SvcTest{
  models.Test test = 1;
}

service TestSvc {
  rpc Foobar(SvcTest) returns (models.Test);
}

edit: at least this is the special part about my proto setup

[stanley-cheung]

@buehler Thanks. With these imports, I am able to verify the problem

• All these had been tested with --grpc-web_out=import_style=commonjs+dts
• I added some imports just like the comment above.
  • Added a models.proto with a Test empty message
  • Added import "models.proto"; into echo.proto
  • Changed the response of an RPC to returns (models.Test)
• With version 1.0.4, protos with imports work just fine end-to-end
• With version 1.0.5, I get a Uncaught ReferenceError: echo_pb is not defined when the compiled JS is being loaded at the browser
• With the fix fix wrong package name of input type #599 compiled in, the error is gone.

It is still a bit unclear to me, why with 1.0.5, with the bug, running tsc and npx webpack on my ts project yields no error though. The Uncaught ReferenceError: echo_pb is not defined error only shows up when the browser tries to load the compiled JS file ./dist/main.js.

[travikk]

@stanley-cheung fantastic news, thanks for the fix! When are you looking to cut a new release?

[stanley-cheung]

Hopefully soon. I am trying to see if I can add a test to prevent this from happening again in the future

[buehler]

Hey @stanley-cheung
I constructed an example and found the following..
The thing is: the descriptor uses the wrong variable to access the model. So instead of the proto.namespace.message it uses the namespace_pb accessor and this is (under circumstances) not defined.

Since this is an error in javascript and the typescript definitions are fine, neither tsc nor webpack can find this error.
This would be like the following example:

// foobar.d.ts
export class Foobar {}

// foobar.js
module.exports = {};

If you use this file in typescript and compile it, you could "instantiate" a Foobar class. The compile would not throw an error, but when you actually run the code, Foobar will not be defined.

This is a runtime error that happens when the JIT of the browser does parse and compile the javascript code.

// Definition... look how `service_pb` is loaded.
var models_pb = require('./models_pb.js')
const proto = {};
proto.service = require('./service_pb.js');

const methodDescriptor_Service_Demo = new grpc.web.MethodDescriptor(
  '/service.Service/Demo',
  grpc.web.MethodType.UNARY,
  service_pb.ServiceModel,  // <<<<< `service_pb` is not defined.
  models_pb.DemoModel,
  /** @param {!proto.service.ServiceModel} request */
  function(request) {
    return request.serializeBinary();
  },
  models_pb.DemoModel.deserializeBinary
);

A purposed test that could help to prevent those errors (beside actually compile it with tsc or webpack) could be to load the actual code with a node vm. Maybe with something like https://www.npmjs.com/package/vm2. With this, you could be sure that the code actually runs through the JIT compiler of a js engine.

I attached my findings and demo files for further analysis if needed.

Cheers
prototest.tar.gz

[buehler]

oh. just saw that you actually added the eval test 😁

[travikk]

@stanley-cheung would it be possible to get a released version anytime soon?

[stanley-cheung]

@travikk Working on it

[stanley-cheung]

Fixed in version 1.0.6 now.

[travikk]

@stanley-cheung amazing works, thanks. I can confirm it works for me now!