From 0ad80f876d1577ec2fa1869c2018b8a9fec70205 Mon Sep 17 00:00:00 2001 From: tomas-gold_swi Date: Mon, 10 Jun 2024 13:42:13 +0200 Subject: [PATCH 1/2] introduce flat strict export --- .gitignore | 1 + generator/js_generator.cc | 29 +++++++++------- generator/js_generator.h | 1 + package.json | 6 ++-- test-jest/README.md | 9 +++++ test-jest/commonjs-strict/flat-strict.test.js | 34 +++++++++++++++++++ test-jest/commonjs-strict/protos/proto.proto | 20 +++++++++++ test-jest/commonjs-strict/protos/proto2.proto | 8 +++++ 8 files changed, 94 insertions(+), 14 deletions(-) create mode 100644 test-jest/README.md create mode 100644 test-jest/commonjs-strict/flat-strict.test.js create mode 100644 test-jest/commonjs-strict/protos/proto.proto create mode 100644 test-jest/commonjs-strict/protos/proto2.proto diff --git a/.gitignore b/.gitignore index a1690e7..9330eee 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ bazel-* /testproto_libs1.js /testproto_libs2.js /google-protobuf.js +test-jest/*_pb.js diff --git a/generator/js_generator.cc b/generator/js_generator.cc index d6fad0a..79dd53a 100644 --- a/generator/js_generator.cc +++ b/generator/js_generator.cc @@ -228,7 +228,8 @@ std::string MaybeCrossFileRef(const GeneratorOptions& options, const FileDescriptor* from_file, const Descriptor* to_message) { if ((options.import_style == GeneratorOptions::kImportCommonJs || - options.import_style == GeneratorOptions::kImportCommonJsStrict) && + options.import_style == GeneratorOptions::kImportCommonJsStrict || + options.import_style == GeneratorOptions::kImportCommonJsFlatStrict) && from_file != to_message->file()) { // Cross-file ref in CommonJS needs to use the module alias instead of // the global name. @@ -1737,7 +1738,7 @@ void Generator::GenerateProvides(const GeneratorOptions& options, // foo.bar.Baz = function() { /* ... */ } // Do not use global scope in strict mode - if (options.import_style == GeneratorOptions::kImportCommonJsStrict) { + if (options.import_style == GeneratorOptions::kImportCommonJsStrict || options.import_style == GeneratorOptions::kImportCommonJsFlatStrict) { std::string namespaceObject = *it; // Remove "proto." from the namespace object ABSL_CHECK_EQ(0, namespaceObject.compare(0, 6, "proto.")); @@ -2315,7 +2316,7 @@ void Generator::GenerateClassFieldToObject(const GeneratorOptions& options, std::string value_to_object; if (value_field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) { value_to_object = - GetMessagePath(options, value_field->message_type()) + ".toObject"; + SubmessageTypeRef(options, value_field) + ".toObject"; } else { value_to_object = "undefined"; } @@ -2450,7 +2451,7 @@ void Generator::GenerateClassFieldFromObject( "$fieldclass$.fromObject));\n", "name", JSObjectFieldName(options, field), "index", JSFieldIndex(field), "fieldclass", - GetMessagePath(options, value_field->message_type())); + SubmessageTypeRef(options, value_field)); } else { // `msg` is a newly-constructed message object that has not yet built any // map containers wrapping underlying arrays, so we can simply directly @@ -2584,7 +2585,7 @@ void Generator::GenerateClassField(const GeneratorOptions& options, printer->Print( ",\n" " $messageType$", - "messageType", GetMessagePath(options, value_field->message_type())); + "messageType", SubmessageTypeRef(options, value_field)); } else { printer->Print( ",\n" @@ -2961,7 +2962,7 @@ void Generator::GenerateRepeatedMessageHelperMethods( "\n", "index", JSFieldIndex(field), "oneofgroup", (InRealOneof(field) ? (", " + JSOneofArray(options, field)) : ""), "ctor", - GetMessagePath(options, field->message_type())); + SubmessageTypeRef(options, field)); } void Generator::GenerateClassExtensionFieldInfo(const GeneratorOptions& options, @@ -3102,14 +3103,14 @@ void Generator::GenerateClassDeserializeBinaryField( if (value_field->type() == FieldDescriptor::TYPE_MESSAGE) { printer->Print(", $messageType$.deserializeBinaryFromReader", "messageType", - GetMessagePath(options, value_field->message_type())); + SubmessageTypeRef(options, value_field)); } else { printer->Print(", null"); } printer->Print(", $defaultKey$", "defaultKey", JSFieldDefault(key_field)); if (value_field->type() == FieldDescriptor::TYPE_MESSAGE) { printer->Print(", new $messageType$()", "messageType", - GetMessagePath(options, value_field->message_type())); + SubmessageTypeRef(options, value_field)); } else { printer->Print(", $defaultValue$", "defaultValue", JSFieldDefault(value_field)); @@ -3302,7 +3303,7 @@ void Generator::GenerateClassSerializeBinaryField( if (value_field->type() == FieldDescriptor::TYPE_MESSAGE) { printer->Print(", $messageType$.serializeBinaryToWriter", "messageType", - GetMessagePath(options, value_field->message_type())); + SubmessageTypeRef(options, value_field)); } printer->Print(");\n"); @@ -3482,6 +3483,8 @@ bool GeneratorOptions::ParseFromOptions( import_style = kImportCommonJs; } else if (option.second == "commonjs_strict") { import_style = kImportCommonJsStrict; + } else if (option.second == "commonjs_flat_strict") { + import_style = kImportCommonJsFlatStrict; } else if (option.second == "browser") { import_style = kImportBrowser; } else if (option.second == "es6") { @@ -3618,12 +3621,14 @@ void Generator::GenerateFile(const GeneratorOptions& options, // Generate "require" statements. if ((options.import_style == GeneratorOptions::kImportCommonJs || - options.import_style == GeneratorOptions::kImportCommonJsStrict)) { + options.import_style == GeneratorOptions::kImportCommonJsStrict || + options.import_style == GeneratorOptions::kImportCommonJsFlatStrict) + ) { printer->Print("var jspb = require('google-protobuf');\n"); printer->Print("var goog = jspb;\n"); // Do not use global scope in strict mode - if (options.import_style == GeneratorOptions::kImportCommonJsStrict) { + if (options.import_style == GeneratorOptions::kImportCommonJsStrict || options.import_style == GeneratorOptions::kImportCommonJsFlatStrict) { printer->Print("var proto = {};\n\n"); } else { // To get the global object we call a function with .call(null), this will @@ -3691,7 +3696,7 @@ void Generator::GenerateFile(const GeneratorOptions& options, } // if provided is empty, do not export anything - if (options.import_style == GeneratorOptions::kImportCommonJs && + if ((options.import_style == GeneratorOptions::kImportCommonJs || options.import_style == GeneratorOptions::kImportCommonJsFlatStrict) && !provided.empty()) { printer->Print("goog.object.extend(exports, $package$);\n", "package", GetNamespace(options, file)); diff --git a/generator/js_generator.h b/generator/js_generator.h index 8ce3882..13daae4 100644 --- a/generator/js_generator.h +++ b/generator/js_generator.h @@ -70,6 +70,7 @@ struct GeneratorOptions { kImportClosure, // goog.require() kImportCommonJs, // require() kImportCommonJsStrict, // require() with no global export + kImportCommonJsFlatStrict, // require() with no global export + flat export of the proto objects kImportBrowser, // no import statements kImportEs6, // import { member } from '' } import_style; diff --git a/package.json b/package.json index 438bbe5..53d664b 100644 --- a/package.json +++ b/package.json @@ -12,18 +12,20 @@ "package.json", "README.md" ], - "dependencies": {}, "devDependencies": { "glob": "~7.1.4", "google-closure-compiler": "~20190819.0.0", "google-closure-deps": "^20210406.0.0", "google-closure-library": "~20200315.0.0", + "google-protobuf": "^3.21.2", "gulp": "~4.0.2", - "jasmine": "~3.5.0" + "jasmine": "~3.5.0", + "jest": "29.7.0" }, "scripts": { "build": "node ./node_modules/gulp/bin/gulp.js dist", "test": "node ./node_modules/gulp/bin/gulp.js test", + "test:jest": "jest ./test-jest --runInBand", "clean": "node ./node_modules/gulp/bin/gulp.js clean" }, "repository": { diff --git a/test-jest/README.md b/test-jest/README.md new file mode 100644 index 0000000..387d73c --- /dev/null +++ b/test-jest/README.md @@ -0,0 +1,9 @@ +## Prerequisities + +- node version 20.0.0+ + +## How to run + +You need to build the "protoc-gen-js" binary before running any test. +1. Run `bazel build` in the root directory. +2. Run `npm run test:jest` \ No newline at end of file diff --git a/test-jest/commonjs-strict/flat-strict.test.js b/test-jest/commonjs-strict/flat-strict.test.js new file mode 100644 index 0000000..d074868 --- /dev/null +++ b/test-jest/commonjs-strict/flat-strict.test.js @@ -0,0 +1,34 @@ +const path = require("path"); +const util = require("util"); +const exec = util.promisify(require("node:child_process").exec); + +const protocJsPlugin = path.resolve(__dirname, "..", "..", "bazel-bin", "generator", "protoc-gen-js"); +const command = `protoc --plugin=protoc-gen-js=${protocJsPlugin} --js_out=import_style=commonjs_flat_strict,binary:.`; + +describe("When import_style is 'commonjs_flat_strict'", () => { + it("'proto' is exported directly without package nesting", async () => { + await exec(command + " ./protos/proto.proto ./protos/proto2.proto", { cwd: __dirname }); + const r = require(path.resolve(__dirname, "./protos/proto_pb")); + expect(r.CommandRequest).toBeDefined(); + }); + + it("global 'proto' is not polluted", async () => { + await exec(command + " ./protos/proto.proto ./protos/proto2.proto", { cwd: __dirname }); + const r = require(path.resolve(__dirname, "./protos/proto_pb")); + expect(global.proto).toBe(undefined); + }); + + it("addCommands resolves correctly without an error", async () => { + await exec(command + " ./protos/proto.proto ./protos/proto2.proto", { cwd: __dirname }); + const r = require(path.resolve(__dirname, "./protos/proto_pb")); + const c = new r.CommandRequest(); + c.addCommands(); + }); + + it("getCommandsList resolves correctly without an error", async () => { + await exec(command + " ./protos/proto.proto ./protos/proto2.proto", { cwd: __dirname }); + const r = require(path.resolve(__dirname, "./protos/proto_pb")); + const c = new r.CommandRequest(); + c.getCommandsList(); + }); +}); diff --git a/test-jest/commonjs-strict/protos/proto.proto b/test-jest/commonjs-strict/protos/proto.proto new file mode 100644 index 0000000..09b2b9d --- /dev/null +++ b/test-jest/commonjs-strict/protos/proto.proto @@ -0,0 +1,20 @@ +syntax = "proto3"; + +package foo.bar.baz.v2; + +import "protos/proto2.proto"; + +service FooService { + rpc Commands(CommandRequest) returns (CommandResponse) {} +} + +message CommandRequest { + string session_id = 1; + repeated command.pkg.nesting.Command commands = 2; + command.pkg.nesting.Command command = 3; + map more_commands = 4; +} + +message CommandResponse { + string result = 1; +} diff --git a/test-jest/commonjs-strict/protos/proto2.proto b/test-jest/commonjs-strict/protos/proto2.proto new file mode 100644 index 0000000..099ffeb --- /dev/null +++ b/test-jest/commonjs-strict/protos/proto2.proto @@ -0,0 +1,8 @@ +syntax = "proto3"; + +package command.pkg.nesting; + +message Command { + string id = 1; + string command = 2; +} \ No newline at end of file From e600ce371a433ee86ee13e0fcc1e32553c0499fe Mon Sep 17 00:00:00 2001 From: tomas-gold_swi Date: Mon, 10 Jun 2024 14:35:40 +0200 Subject: [PATCH 2/2] trigger