Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

clutz + google/protobuf + commonjs #334

Closed
tamird opened this issue Aug 26, 2016 · 10 comments
Closed

clutz + google/protobuf + commonjs #334

tamird opened this issue Aug 26, 2016 · 10 comments

Comments

@tamird
Copy link

tamird commented Aug 26, 2016

Howdy!

I'm trying to get clutz integrated into our proto build over in CockroachDB, but I'm running into some trouble. Currently, I'm, compiling our protos to commonjs and trying to run clutz over the output to produce typings. The error I get is below:

~/src/clutz/build/install/clutz/bin/clutz --externs ~/src/clutz/src/resources/es6_min.js -o cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.d.ts cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.js
cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.js:8: ERROR - variable require is undeclared
   6| // GENERATED CODE -- DO NOT EDIT!
   7|
   8| var jspb = require('google-protobuf');
   9| var goog = jspb;
  10| var global = Function('return this')();

cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.js:10: ERROR - variable Function is undeclared
   8| var jspb = require('google-protobuf');
   9| var goog = jspb;
  10| var global = Function('return this')();
  11|
  12| var cockroach_config_config_pb = require('../../../cockroach/config/config_pb.js');

cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.js:61: ERROR - variable proto is undeclared
  59|  * @constructor
  60|  */
  61| proto.cockroach.server.serverpb.DatabasesRequest = function(opt_data) {
  62|   jspb.Message.initialize(this, opt_data, 0, -1, null, null);
  63| };

cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.js:65: ERROR - variable COMPILED is undeclared
  63| };
  64| goog.inherits(proto.cockroach.server.serverpb.DatabasesRequest, jspb.Message);
  65| if (goog.DEBUG && !COMPILED) {
  66|   proto.cockroach.server.serverpb.DatabasesRequest.displayName = 'proto.cockroach.server.serverpb.DatabasesRequest';
  67| }

cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.js:170: ERROR - variable undefined is undeclared
  168|  */
  169| proto.cockroach.server.serverpb.DatabasesRequest.prototype.serializeBinaryToWriter = function (writer) {
  170|   var f = undefined;
  171| };
  172|

cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.js:6632: ERROR - variable exports is undeclared
  6630| };
  6631|
  6632| goog.object.extend(exports, proto.cockroach.server.serverpb);

6 error(s), 0 warning(s), 57.4% typed
make[1]: *** [cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.d.ts] Error 2
make: *** [protobuf] Error 2

This is my first attempt at using any tool in the closure compiler ecosystem, so I'm sure I've done something wrong. Any help would be greatly appreciated!

@rkirov
Copy link
Contributor

rkirov commented Aug 27, 2016

Welcome :)

Clutz is basically a wrapper around the Closure Compiler and what you are seeing is the Closure Compiler rejecting your code, because of some unknown symbols.

It a bit of a wild chase to find where each of those comes from, and passing all those deps to the clutz invocation, but here is a start:

COMPILED - https://github.com/google/closure-library/blob/master/closure/goog/base.js#L32
Function - https://github.com/google/closure-compiler/blob/master/externs/es3.js#L491
require - https://github.com/google/closure-compiler/blob/master/contrib/nodejs/globals.js#L33

Also I don't recommend using ~/src/clutz/src/resources/es6_min.js as externs, since it is just a test shim. Instead use *.js from here - https://github.com/google/closure-compiler/tree/master/externs

@tamird
Copy link
Author

tamird commented Aug 27, 2016

Thanks for the help!

OK, I did a bit of hacking and reading a got a little further. First, I had to patch clutz so it invokes closure with commonjs support:

diff --git a/src/main/java/com/google/javascript/clutz/Options.java b/src/main/java/com/google/javascript/clutz/Options.java
index 2cceaca..887a2dc 100644
--- a/src/main/java/com/google/javascript/clutz/Options.java
+++ b/src/main/java/com/google/javascript/clutz/Options.java
@@ -86,6 +86,7 @@ public class Options {
     options.setLanguageOut(CompilerOptions.LanguageMode.ECMASCRIPT5);
     options.setCheckTypes(true);
     options.setInferTypes(true);
+    options.setProcessCommonJSModules(true);
     options.setIdeMode(true); // So that we can query types after compilation.
     return options;
   }

I also tweaked my list of externs as you suggested, but now I have a new problem!

~/src/clutz/build/install/clutz/bin/clutz --externs ~/src/closure-compiler/externs/es3.js ~/src/closure-compiler/externs/es6.js ~/src/closure-compiler/contrib/nodejs/globals.js ~/src/closure-library/closure/goog/base.js -o cockroach/ui/app/js/protos/cockroach/roachpb/metadata_pb.d.ts cockroach/ui/app/js/protos/cockroach/roachpb/metadata_pb.js
cockroach/ui/app/js/protos/cockroach/roachpb/metadata_pb.js:8: ERROR - Failed to load module "google-protobuf"
   6| // GENERATED CODE -- DO NOT EDIT!
   7|
   8| var jspb = require('google-protobuf');
   9| var goog = jspb;
  10| var global = Function('return this')();

cockroach/ui/app/js/protos/cockroach/roachpb/metadata_pb.js:12: ERROR - Failed to load module "../../cockroach/util/unresolved_addr_pb.js"
  10| var global = Function('return this')();
  11|
  12| var cockroach_util_unresolved_addr_pb = require('../../cockroach/util/unresolved_addr_pb.js');
  13| goog.exportSymbol('proto.cockroach.roachpb.Attributes', null, global);
  14| goog.exportSymbol('proto.cockroach.roachpb.NodeDescriptor', null, global);

cockroach/ui/app/js/protos/cockroach/roachpb/metadata_pb.js:32: ERROR - variable proto is undeclared
  30|  * @constructor
  31|  */
  32| proto.cockroach.roachpb.Attributes = function(opt_data) {
  33|   jspb.Message.initialize(this, opt_data, 0, -1, proto.cockroach.roachpb.Attributes.repeatedFields_, null);
  34| };

3 error(s), 0 warning(s), 55.4% typed

Any ideas on where to go from here? The lines:

goog.exportSymbol('proto.cockroach.roachpb.Attributes', null, global);
goog.exportSymbol('proto.cockroach.roachpb.NodeDescriptor', null, global);
goog.exportSymbol('proto.cockroach.roachpb.RangeDescriptor', null, global);
goog.exportSymbol('proto.cockroach.roachpb.ReplicaDescriptor', null, global);
goog.exportSymbol('proto.cockroach.roachpb.ReplicaIdent', null, global);
goog.exportSymbol('proto.cockroach.roachpb.StoreCapacity', null, global);
goog.exportSymbol('proto.cockroach.roachpb.StoreDeadReplicas', null, global);
goog.exportSymbol('proto.cockroach.roachpb.StoreDescriptor', null, global);

are present in cockroach/ui/app/js/protos/cockroach/roachpb/metadata_pb.js, so I'm not sure what to make of the error I'm seeing.

@tamird
Copy link
Author

tamird commented Aug 27, 2016

Hm, looks like I am at least partially running into google/closure-compiler#954.

@tamird
Copy link
Author

tamird commented Aug 27, 2016

Following the advice on that issue (aliasing require to externalRequire) I'm down to just the one error:

~/src/clutz/build/install/clutz/bin/clutz --externs ~/src/closure-compiler/externs/es3.js ~/src/closure-compiler/externs/es6.js ~/src/closure-compiler/contrib/nodejs/globals.js ~/src/closure-library/closure/goog/base.js -o cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.d.ts cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.js
cockroach/ui/app/js/protos/cockroach/server/serverpb/admin_pb.js:61: ERROR - variable proto is undeclared
  59|  * @constructor
  60|  */
  61| proto.cockroach.server.serverpb.DatabasesRequest = function(opt_data) {
  62|   jspb.Message.initialize(this, opt_data, 0, -1, null, null);
  63| };

1 error(s), 0 warning(s), 58.1% typed

@tamird tamird changed the title variable {require,Function,COMPILED) is undeclared clutz + google/protobuf + commonjs Aug 27, 2016
@rkirov
Copy link
Contributor

rkirov commented Aug 29, 2016

This is really odd, because most likely the first line of admin_pb.js says goog.provide('cockroach.server.serverpb.DatabasesRequest') which defines the proto variable proto.

That's said, most likely after this change you will discover a slew of other ones, because you need to pass all your dependencies to the clutz execution - that includes jspb's message.js, closure's array.js, etc. In our usage of clutz it is plugged into a build system that keep track of all dependencies.

@tamird
Copy link
Author

tamird commented Aug 29, 2016

This is really odd, because most likely the first line of admin_pb.js says goog.provide('cockroach.server.serverpb.DatabasesRequest') which defines the proto variable proto.

In fact, it does not say this! It only says goog.exportSymbol('proto.cockroach.server.serverpb.DatabasesRequest', null, global);.

That's said, most likely after this change you will discover a slew of other ones, because you need to pass all your dependencies to the clutz execution - that includes jspb's message.js, closure's array.js, etc. In our usage of clutz it is plugged into a build system that keep track of all dependencies.

Are you sure? That's certainly the case when producing closure-style code from protoc, but in this case I'm using commonjs.

@rkirov
Copy link
Contributor

rkirov commented Aug 29, 2016

aha, that's the problem then, you need to produce code using closures' dependency mechanism if you are to feed to through clutz. Again clutz is just a wrapper around closure compiler with a .d.ts emit instead of regular .js emit.

@tamird
Copy link
Author

tamird commented Aug 29, 2016

Understood, but I thought the closure compiler could also handle commonjs. Perhaps I should take it up with them?

I had other problems when trying to go the closure route (namely that I would then have to closure-ify my entire project to get things working).

@rkirov
Copy link
Contributor

rkirov commented Aug 29, 2016

I think it depends on your definition of "handle" :)

For all the static type analysis closure needs to have static understanding of where each module lives and what symbols are defined on it. I am guessing closure can handle CJS for basic minification tasks, but not for stricter type checking, which must be turned on by one of the flags inside clutz. It is all a guess on my part since all the use cases I have at hand are closure annotated code as input to clutz.

My recommendation is trying to feed your project into closure independent of clutz, as you will have more documentation and support for that task alone. Once you have done that we can try to replicate with clutz.

@rkirov
Copy link
Contributor

rkirov commented Oct 4, 2016

Closing this issue, as there is nothing directly actionable in clutz codebase. My take away is that clutz is not a great fit for projects that dont' already go through Closure compiler.

@rkirov rkirov closed this as completed Oct 4, 2016
@dgp1130 dgp1130 mentioned this issue May 4, 2020
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants