Missing exports: unary

[ztl8702]

Description

When building rollup bundles with @improbable-eng/grpc-web, a warning about "Missing exports" is thrown.

Errors seen

...
(!) Missing exports
https://rollupjs.org/guide/en/#error-name-is-not-exported-by-module
node_modules/rules_typescript_proto/test/proto/pizza_service_pb_service.mjs
unary is not exported by node_modules/@improbable-eng/grpc-web/dist/grpc-web-client.js
31:     callback = arguments[1];
32:   }
33:   var client = grpc.unary(PizzaService.OrderPizza, {
                        ^
34:     request: requestMessage,
35:     host: this.serviceHost,
node_modules/rules_typescript_proto/test/proto/pizza_service_pb_service.mjs
Code is not exported by node_modules/@improbable-eng/grpc-web/dist/grpc-web-client.js
39:     onEnd: function (response) {
40:       if (callback) {
41:         if (response.status !== grpc.Code.OK) {
                                         ^
42:           var err = new Error(response.statusMessage);
43:           err.code = response.status;
created bazel-out/darwin-fastbuild/bin/test/test_es6_bundling in 1s
Target //test:rollup_test up-to-date:
  bazel-bin/test/rollup_test.sh
  bazel-bin/test/rollup_test_loader.js
  bazel-bin/test/rollup_test_require_patch.js

Minimal Reproduction

 git clone https://github.com/Dig-Doug/rules_typescript_proto
 git checkout e3a6de3
 bazel build //test:rollup_test 

Your Environment

What operating system are you using?

macOS 10.15.2

What version of bazel are you using?

Build label: 2.0.0
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Thu Dec 19 12:33:30 2019 (1576758810)
Build timestamp: 1576758810
Build timestamp as int: 1576758810

What version of the library are you using?

git commit e3a6de3

Any other comments?

I noticed that this warning is also present in the most recent CI run:

https://github.com/Dig-Doug/rules_typescript_proto/runs/396864580#step:5:50

I am also aware of improbable-eng/grpc-web#369, which has a slightly different warning about grpc not being exported. Whereas the current issue
concerns grpc.unary and grpc.Code. So I am not sure if this is a regression or a new bug.

[Dig-Doug]

Thanks for reporting this issue and creating a test case for it.

I did some debugging and I've been able to get rid of the rollup errors by changing namedExports in the config:

// test/rollup.config.js 
const commonjs = require('rollup-plugin-commonjs');
const nodeRequire = require('rollup-plugin-node-resolve');

module.exports = {
  plugins: [
    nodeRequire(),
    commonjs({
      namedExports: {
        '@improbable-eng/grpc-web': [
          'unary',
          'Code',
        ],
      }
    }),
  ],
};

This doesn't make your test pass, but I think it's a step in the right direction. In the test, I get an error:

1) Rollup PizzaServiceClient.orderPizza should return a UnaryResponse
  Message:
    TypeError: grpcWebClient_1 is not a function
  Stack:
        at <Jasmine>
        at PizzaServiceClient.grpc.unary (node_modules/rules_typescript_proto/test/proto/pizza_service_pb_service.mjs:33:16)
        at UserContext.<anonymous> (test/rollup_test.spec.js:27:29)
        at <Jasmine>
        at processImmediate (internal/timers.js:439:21)

I've run out of time to debug today, maybe you can figure out what's going wrong in the test? If not, I can take another look tomorrow.

[ztl8702]

Thanks I will take another look today.

[ztl8702]

Made some progress: with the change you suggested in #64 (comment), the generated bazel-bin/test/test_es6_bundling/index.js contains:

var grpcWebClient = createCommonjsModule(function (module, exports) {
  // what looks like the content of 
  // @improbable-eng/grpc-web/dist/grpc-web-client.js
}

// ...

unwrapExports(grpcWebClient);
var grpcWebClient_1 = grpcWebClient.unary;
var grpcWebClient_2 = grpcWebClient.Code;

// ...
PizzaServiceClient.prototype.orderPizza = function orderPizza(requestMessage, metadata, callback) {
  if (arguments.length === 2) {
    callback = arguments[1];
  }
  var client = grpcWebClient_1(PizzaService.OrderPizza, {
    request: requestMessage,
    host: this.serviceHost,
// ...

And this is what grpcWebClient actually contains when the bundle is loaded into the browser:

So the generated bundle should actually contain:

unwrapExports(grpcWebClient);
var grpcWebClient_1 = grpcWebClient.grpc.unary;
var grpcWebClient_2 = grpcWebClient.grpc.Code;

(also unwrapExports seems to be doing nothing here)

Looking at the the commit history and CI logs, it seems the warning about unresolved imports originated in PR #13

[ztl8702]

Hmm, my JS-fu is not quite there, but I think this might be the cause:

Rollup.js is complaining about a named import/export not resolving.

But pizza_service_pb_serivce.mjs is using namespace import:

// pizza_service_pb_serivce.mjs
import * as test_proto_pizza_service_pb from "rules_typescript_proto/test/proto/pizza_service_pb";
import * as grpc from "@improbable-eng/grpc-web";

whereas:

// pizza_service_pb_serivce.d.ts
import * as test_proto_pizza_service_pb from "../../test/proto/pizza_service_pb";
import {grpc} from "@improbable-eng/grpc-web";

// pizza_service_pb_serivce.js
var test_proto_pizza_service_pb = require("rules_typescript_proto/test/proto/pizza_service_pb");
var grpc = require("@improbable-eng/grpc-web").grpc;

both seem to be correctly "reaching into" the grpc object.

So I think rollup.js is correct about not being able to find unary because it is not a named exported. The import statement in pizza_service_pb_serivce.mjs is incorrect.

[Dig-Doug]

Good find! You're correct,

var grpc = require("@improbable-eng/grpc-web").grpc; is not equivalent to import * as grpc from "@improbable-eng/grpc-web";

Overall PR looks good, once we get the tests fixed it should be good to go.

[ztl8702]

Noted, thanks! I will probably need to get back to this next week.

[tsawada]

Hi, @Dig-Doug, thanks for working on and releasing this great work.
Are there any blockers on merging #69? Currently .mjs files are practically unusable as far as I understand and tested. Would really appreciate if the PR gets merged, even without integration tests.