Apply WKT / _virtual_imports patch

[pfgallagher]

Based on #11

Sorry for the delay, @Dig-Doug !

[pfgallagher]

We'll presumably need to add in the updates for the tests as well, but I was hoping to see if it passes the CI/CD pipeline first, since I've been having issues getting a dev env for this set up correctly.

[Dig-Doug]

LGTM pending tests

[pfgallagher]

I spent a bit of time this morning looking into this. It looks like we're running into the same 404 issue as in #11. Additionally, it looks like there are some new namespacing problems between @com_github_protocolbuffers_protobuf and @com_google_protobuf, stemming from upstream changes in those libraries. Could you take a look and let me know what you think needs to be done? Thanks!

[Dig-Doug]

I'll take a look today

[Dig-Doug]

OK, did a bit of debugging. Here's what I did:

I ran the tests locally:

bazel test //...:all
# Picked the first failing test in the output and ran in debug mode:
npx @bazel/ibazel run //test:pizza_service_proto_test_suite_chromium-local

Running the test in debug mode should open a browser. You can see that it's requesting the timestamp proto js code and getting a 404:

01 10 2021 11:38:12.808:WARN [web-server]: 404: /google-protobuf/google/protobuf/timestamp_pb.js

That's a problem. The path should have the workspace name at the beginning. If you try going to http://localhost:9876/base/com_github_protocolbuffers_protobuf/google/protobuf/timestamp_pb.js in the local browser, you'll see the code it there.

So, why is it requesting the wrong code? We're adding the proto to the delivery person proto, so look at the generate code for it:

cat bazel-bin/test/proto/common/delivery_person_pb.js | head -n 50

And you'll see:

var google_protobuf_timestamp_pb = require('google-protobuf/google/protobuf/timestamp_pb'); 
goog.object.extend(proto, google_protobuf_timestamp_pb);
var test_proto_common_pizza_pb = require('rules_typescript_proto/test/proto/common/pizza_pb');

The timestamp require doesn't have the workspace (unlike the next import, which has rules_typescript_proto).

I believe it's coming from this code.

Should we use the code in the npm library or update the code to reference the local generated version?

[pfgallagher]

Wow, this is a tough one!

I did some more digging and it appears the test failures are being caused by the require calls that test delivery_person_ts_proto's transitive inclusion. Is it plausible that this is a module resolution issue?

[Dig-Doug]

Yes, I think the problem is that we can't resolve the timestamp proto JS code. There are two place we can potentially fetch the code from:

1. Since this is a WKT, it is included in the google-protobuf npm module. You'll need to figure out why it can't find the code with the current require statement.
2. We also generate a local version of the code just like all other protos. It's module name is com_github_protocolbuffers_protobuf/google/protobuf/timestamp_pb. If you edit the generated code to change google-protobuf to the correct workspace name, it will hopefully all work.

I don't have a strong preference on which approach we use -- whichever requires less configuration for library users.

[pfgallagher]

I played around with idea one today. I was able to use the requireJS path mapping config to fix the 404 issue, at least. If we go that route, though, it looks like additional module transformation might need to be done (CommonJS -> UMD, iirc) due to the google-protobuf package not including a UMD dist.

Idea two seems like it would work, but would presumably necessitate a bit of hardcoding. If you're all right with that, I think it could be done fairly easily by inserting it into the change_import_style pipeline.

[Dig-Doug]

Yeah, I'm fine with some hard coding if you want to do option 2. Just leave a comment with a reference to the js_generator code where the package name is defined.