From 1062d985b96fbe3d1f20a9ebe692b572eecdb6aa Mon Sep 17 00:00:00 2001 From: Peter Marton Date: Thu, 8 Feb 2018 14:07:15 -0800 Subject: [PATCH 1/4] Feat: add import-style=commonjs_strict option to the compiler --- js/README.md | 1 + .../protobuf/compiler/js/js_generator.cc | 33 +++++++++++++++---- .../protobuf/compiler/js/js_generator.h | 9 ++--- 3 files changed, 33 insertions(+), 10 deletions(-) diff --git a/js/README.md b/js/README.md index ef0d4b198a..d8edca3745 100644 --- a/js/README.md +++ b/js/README.md @@ -144,6 +144,7 @@ Some examples: The `import_style` option is left to the default, which is `closure`. - `--js_out=import_style=commonjs,binary:protos`: this contains the options `import_style=commonjs` and `binary` and outputs to the directory `protos`. + `import_style=commonjs_strict` doesn't expose the output on the global scope. API === diff --git a/src/google/protobuf/compiler/js/js_generator.cc b/src/google/protobuf/compiler/js/js_generator.cc index d8282e4094..dd76a509b9 100755 --- a/src/google/protobuf/compiler/js/js_generator.cc +++ b/src/google/protobuf/compiler/js/js_generator.cc @@ -276,7 +276,8 @@ string GetEnumPath(const GeneratorOptions& options, string MaybeCrossFileRef(const GeneratorOptions& options, const FileDescriptor* from_file, const Descriptor* to_message) { - if (options.import_style == GeneratorOptions::kImportCommonJs && + if ((options.import_style == GeneratorOptions::kImportCommonJs || + options.import_style == GeneratorOptions::kImportCommonJsStrict) && from_file != to_message->file()) { // Cross-file ref in CommonJS needs to use the module alias instead of // the global name. @@ -1675,8 +1676,18 @@ void Generator::GenerateProvides(const GeneratorOptions& options, // // // Later generated code expects foo.bar = {} to exist: // foo.bar.Baz = function() { /* ... */ } - printer->Print("goog.exportSymbol('$name$', null, global);\n", "name", - *it); + + // Do not use global scope in strict mode + if (options.import_style == GeneratorOptions::kImportCommonJsStrict) { + string namespaceObject = *it; + // Remove "proto." from the namespace object + namespaceObject.erase(0, 6); + printer->Print("goog.exportSymbol('$name$', null, proto);\n", "name", + namespaceObject); + } else { + printer->Print("goog.exportSymbol('$name$', null, global);\n", "name", + *it); + } } } } @@ -3321,6 +3332,8 @@ bool GeneratorOptions::ParseFromOptions( import_style = kImportClosure; } else if (options[i].second == "commonjs") { import_style = kImportCommonJs; + } else if (options[i].second == "commonjs_strict") { + import_style = kImportCommonJsStrict; } else if (options[i].second == "browser") { import_style = kImportBrowser; } else if (options[i].second == "es6") { @@ -3430,10 +3443,17 @@ void Generator::GenerateFile(const GeneratorOptions& options, GenerateHeader(options, printer); // Generate "require" statements. - if (options.import_style == GeneratorOptions::kImportCommonJs) { + if ((options.import_style == GeneratorOptions::kImportCommonJs || + options.import_style == GeneratorOptions::kImportCommonJsStrict)) { printer->Print("var jspb = require('google-protobuf');\n"); printer->Print("var goog = jspb;\n"); - printer->Print("var global = Function('return this')();\n\n"); + + // Do not use global scope in strict mode + if (options.import_style == GeneratorOptions::kImportCommonJsStrict) { + printer->Print("var proto = {};\n\n"); + } else { + printer->Print("var global = Function('return this')();\n\n"); + } for (int i = 0; i < file->dependency_count(); i++) { const string& name = file->dependency(i)->name(); @@ -3477,7 +3497,8 @@ void Generator::GenerateFile(const GeneratorOptions& options, GenerateExtension(options, printer, *it); } - if (options.import_style == GeneratorOptions::kImportCommonJs) { + if ((options.import_style == GeneratorOptions::kImportCommonJs || + options.import_style == GeneratorOptions::kImportCommonJsStrict)) { printer->Print("goog.object.extend(exports, $package$);\n", "package", GetFilePath(options, file)); } diff --git a/src/google/protobuf/compiler/js/js_generator.h b/src/google/protobuf/compiler/js/js_generator.h index 3cc60e2283..b50ef7fd83 100755 --- a/src/google/protobuf/compiler/js/js_generator.h +++ b/src/google/protobuf/compiler/js/js_generator.h @@ -63,10 +63,11 @@ struct GeneratorOptions { bool binary; // What style of imports should be used. enum ImportStyle { - kImportClosure, // goog.require() - kImportCommonJs, // require() - kImportBrowser, // no import statements - kImportEs6, // import { member } from '' + kImportClosure, // goog.require() + kImportCommonJs, // require() + kImportCommonJsStrict, // require() with no global export + kImportBrowser, // no import statements + kImportEs6, // import { member } from '' } import_style; GeneratorOptions() From 3c4e36847330df7c6b3e136b5abf590466dac164 Mon Sep 17 00:00:00 2001 From: Peter Marton Date: Thu, 1 Mar 2018 16:05:19 -0800 Subject: [PATCH 2/4] Test: cover import_style=commonjs_strict --- Makefile.am | 2 ++ js/commonjs/strict_test.js | 54 ++++++++++++++++++++++++++++++++++++++ js/gulpfile.js | 17 +++++++++++- js/test9.proto | 40 ++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 js/commonjs/strict_test.js create mode 100644 js/test9.proto diff --git a/Makefile.am b/Makefile.am index 48f2c03465..b9ebdba337 100644 --- a/Makefile.am +++ b/Makefile.am @@ -930,6 +930,7 @@ js_EXTRA_DIST= \ js/commonjs/import_test.js \ js/commonjs/jasmine.json \ js/commonjs/rewrite_tests_for_commonjs.js \ + js/commonjs/strict_test.js \ js/commonjs/test6/test6.proto \ js/commonjs/test7/test7.proto \ js/compatibility_tests/v3.0.0/binary/arith_test.js \ @@ -1001,6 +1002,7 @@ js_EXTRA_DIST= \ js/test4.proto \ js/test5.proto \ js/test8.proto \ + js/test9.proto \ js/test_bootstrap.js \ js/testbinary.proto \ js/testempty.proto diff --git a/js/commonjs/strict_test.js b/js/commonjs/strict_test.js new file mode 100644 index 0000000000..12df8f8518 --- /dev/null +++ b/js/commonjs/strict_test.js @@ -0,0 +1,54 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2016 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Test suite is written using Jasmine -- see http://jasmine.github.io/ + + + +var googleProtobuf = require('google-protobuf'); +var asserts = require('closure_asserts_commonjs'); +var global = Function('return this')(); + +// Bring asserts into the global namespace. +googleProtobuf.object.extend(global, asserts); + +var test9_pb = require('./test9_pb'); + +describe('Strict test suite', function() { + it('testImportedMessage', function() { + var simple1 = new test9_pb.Simple9() + var simple2 = new test9_pb.Simple9() + assertObjectEquals(simple1.toObject(), simple2.toObject()); + }); + + it('testGlobalScopePollution', function() { + assertObjectEquals(global.proto.jspb.test.Simple9, undefined); + }); +}); diff --git a/js/gulpfile.js b/js/gulpfile.js index fc9559f934..4e6dd14ba4 100644 --- a/js/gulpfile.js +++ b/js/gulpfile.js @@ -41,6 +41,11 @@ var group2Protos = [ 'commonjs/test7/test7.proto', ]; +var group3Protos = [ + 'test9.proto' +]; + + gulp.task('genproto_well_known_types_closure', function (cb) { exec(protoc + ' --js_out=one_output_file_per_input_file,binary:. -I ../src -I . ' + wellKnownTypes.join(' '), function (err, stdout, stderr) { @@ -112,6 +117,15 @@ gulp.task('genproto_wellknowntypes', function (cb) { cb(err); }); }); +gulp.task('genproto_group3_commonjs_strict', function (cb) { + exec('mkdir -p commonjs_out && ' + protoc + ' --js_out=import_style=commonjs_strict,binary:commonjs_out -I ../src -I commonjs -I . ' + group3Protos.join(' '), + function (err, stdout, stderr) { + console.log(stdout); + console.log(stderr); + cb(err); + }); +}); + function getClosureBuilderCommand(exportsFile, outputFile) { return './node_modules/google-closure-library/closure/bin/build/closurebuilder.py ' + @@ -159,7 +173,7 @@ gulp.task('commonjs_testdeps', function (cb) { }); }); -gulp.task('make_commonjs_out', ['dist', 'genproto_well_known_types_commonjs', 'genproto_group1_commonjs', 'genproto_group2_commonjs', 'genproto_commonjs_wellknowntypes', 'commonjs_asserts', 'commonjs_testdeps'], function (cb) { +gulp.task('make_commonjs_out', ['dist', 'genproto_well_known_types_commonjs', 'genproto_group1_commonjs', 'genproto_group2_commonjs', 'genproto_commonjs_wellknowntypes', 'commonjs_asserts', 'commonjs_testdeps', 'genproto_group3_commonjs_strict'], function (cb) { // TODO(haberman): minify this more aggressively. // Will require proper externs/exports. var cmd = "mkdir -p commonjs_out/binary && mkdir -p commonjs_out/test_node_modules && "; @@ -174,6 +188,7 @@ gulp.task('make_commonjs_out', ['dist', 'genproto_well_known_types_commonjs', 'g exec(cmd + 'cp commonjs/jasmine.json commonjs_out/jasmine.json && ' + 'cp google-protobuf.js commonjs_out/test_node_modules && ' + + 'cp commonjs/strict_test.js commonjs_out/strict_test.js &&' + 'cp commonjs/import_test.js commonjs_out/import_test.js', function (err, stdout, stderr) { console.log(stdout); diff --git a/js/test9.proto b/js/test9.proto new file mode 100644 index 0000000000..4eeac02a85 --- /dev/null +++ b/js/test9.proto @@ -0,0 +1,40 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2008 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +syntax = "proto2"; + +option java_package = "com.google.apps.jspb.proto"; +option java_multiple_files = true; + +message Simple9 { + required string a_string = 1; + repeated string a_repeated_string = 2; + optional bool a_boolean = 3; +} From 13f94b4092cff9bc36a62adb4bf2e362b4f85979 Mon Sep 17 00:00:00 2001 From: Peter Marton Date: Mon, 26 Mar 2018 16:07:45 -0700 Subject: [PATCH 3/4] Fix strict JS generator with import in a protofile --- Makefile.am | 1 + js/commonjs/strict_test.js | 19 +++++++-- js/gulpfile.js | 3 +- js/test10.proto | 39 +++++++++++++++++++ js/test9.proto | 3 +- .../protobuf/compiler/js/js_generator.cc | 9 +++-- 6 files changed, 65 insertions(+), 9 deletions(-) create mode 100644 js/test10.proto diff --git a/Makefile.am b/Makefile.am index b9ebdba337..efcce65f59 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1003,6 +1003,7 @@ js_EXTRA_DIST= \ js/test5.proto \ js/test8.proto \ js/test9.proto \ + js/test10.proto \ js/test_bootstrap.js \ js/testbinary.proto \ js/testempty.proto diff --git a/js/commonjs/strict_test.js b/js/commonjs/strict_test.js index 12df8f8518..46458c10b3 100644 --- a/js/commonjs/strict_test.js +++ b/js/commonjs/strict_test.js @@ -40,15 +40,28 @@ var global = Function('return this')(); googleProtobuf.object.extend(global, asserts); var test9_pb = require('./test9_pb'); +var test10_pb = require('./test10_pb'); describe('Strict test suite', function() { it('testImportedMessage', function() { - var simple1 = new test9_pb.Simple9() - var simple2 = new test9_pb.Simple9() + var simple1 = new test9_pb.jspb.exttest.strict.nine.Simple9() + var simple2 = new test9_pb.jspb.exttest.strict.nine.Simple9() assertObjectEquals(simple1.toObject(), simple2.toObject()); }); it('testGlobalScopePollution', function() { - assertObjectEquals(global.proto.jspb.test.Simple9, undefined); + assertObjectEquals(global.jspb.exttest, undefined); + }); + + describe('with imports', function() { + it('testImportedMessage', function() { + var simple1 = new test10_pb.jspb.exttest.strict.ten.Simple10() + var simple2 = new test10_pb.jspb.exttest.strict.ten.Simple10() + assertObjectEquals(simple1.toObject(), simple2.toObject()); + }); + + it('testGlobalScopePollution', function() { + assertObjectEquals(global.jspb.exttest, undefined); + }); }); }); diff --git a/js/gulpfile.js b/js/gulpfile.js index 4e6dd14ba4..709c5cf9eb 100644 --- a/js/gulpfile.js +++ b/js/gulpfile.js @@ -42,7 +42,8 @@ var group2Protos = [ ]; var group3Protos = [ - 'test9.proto' + 'test9.proto', + 'test10.proto' ]; diff --git a/js/test10.proto b/js/test10.proto new file mode 100644 index 0000000000..9fa5256c54 --- /dev/null +++ b/js/test10.proto @@ -0,0 +1,39 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2008 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +syntax = "proto3"; + +package jspb.exttest.strict.ten; + +import "test9.proto"; + +message Simple10 { + jspb.exttest.strict.nine.Simple9 a = 1; +} diff --git a/js/test9.proto b/js/test9.proto index 4eeac02a85..9f68085282 100644 --- a/js/test9.proto +++ b/js/test9.proto @@ -30,8 +30,7 @@ syntax = "proto2"; -option java_package = "com.google.apps.jspb.proto"; -option java_multiple_files = true; +package jspb.exttest.strict.nine; message Simple9 { required string a_string = 1; diff --git a/src/google/protobuf/compiler/js/js_generator.cc b/src/google/protobuf/compiler/js/js_generator.cc index dd76a509b9..dfa22ac692 100755 --- a/src/google/protobuf/compiler/js/js_generator.cc +++ b/src/google/protobuf/compiler/js/js_generator.cc @@ -3458,7 +3458,8 @@ void Generator::GenerateFile(const GeneratorOptions& options, for (int i = 0; i < file->dependency_count(); i++) { const string& name = file->dependency(i)->name(); printer->Print( - "var $alias$ = require('$file$');\n", + "var $alias$ = require('$file$');\n" + "goog.object.extend(proto, $alias$);\n", "alias", ModuleAlias(name), "file", GetRootPath(file->name(), name) + GetJSFilename(options, name)); @@ -3497,10 +3498,12 @@ void Generator::GenerateFile(const GeneratorOptions& options, GenerateExtension(options, printer, *it); } - if ((options.import_style == GeneratorOptions::kImportCommonJs || - options.import_style == GeneratorOptions::kImportCommonJsStrict)) { + if (options.import_style == GeneratorOptions::kImportCommonJs) { printer->Print("goog.object.extend(exports, $package$);\n", "package", GetFilePath(options, file)); + } else if(options.import_style == GeneratorOptions::kImportCommonJsStrict) { + printer->Print("goog.object.extend(exports, proto);\n", + "package", GetFilePath(options, file)); } // Emit well-known type methods. From e479adf394dae62b6ddd82675d840d24c3431b83 Mon Sep 17 00:00:00 2001 From: Peter Marton Date: Mon, 21 May 2018 16:00:54 -0700 Subject: [PATCH 4/4] fix(js_generator): check for proto --- src/google/protobuf/compiler/js/js_generator.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/google/protobuf/compiler/js/js_generator.cc b/src/google/protobuf/compiler/js/js_generator.cc index dfa22ac692..0d9eeb9564 100755 --- a/src/google/protobuf/compiler/js/js_generator.cc +++ b/src/google/protobuf/compiler/js/js_generator.cc @@ -1681,6 +1681,7 @@ void Generator::GenerateProvides(const GeneratorOptions& options, if (options.import_style == GeneratorOptions::kImportCommonJsStrict) { string namespaceObject = *it; // Remove "proto." from the namespace object + GOOGLE_CHECK(namespaceObject.compare(0, 6, "proto.")); namespaceObject.erase(0, 6); printer->Print("goog.exportSymbol('$name$', null, proto);\n", "name", namespaceObject);