Merge pull request #4299 from hekike/feat/add-commonjs-strict-import-style

Feat: add import-style=commonjs_strict option to the compiler

8 files changed, 202 insertions, 11 deletions

diff --git a/Makefile.am b/Makefile.am
index 76bb2ba2..24f520c6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -935,6 +935,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                   \
@@ -1006,6 +1007,8 @@ js_EXTRA_DIST=                                                         \
   js/test4.proto                                                       \
   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/README.md b/js/README.md
index ef0d4b19..d8edca37 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/js/commonjs/strict_test.js b/js/commonjs/strict_test.js
new file mode 100644
index 00000000..46458c10
--- /dev/null
+++ b/js/commonjs/strict_test.js
@@ -0,0 +1,67 @@
+// 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');
+var test10_pb = require('./test10_pb');
+
+describe('Strict test suite', function() {
+  it('testImportedMessage', function() {
+    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.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 fc9559f9..709c5cf9 100644
--- a/js/gulpfile.js
+++ b/js/gulpfile.js
@@ -41,6 +41,12 @@ var group2Protos = [
   'commonjs/test7/test7.proto',
 ];
 
+var group3Protos = [
+  'test9.proto',
+  'test10.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 +118,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 +174,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 +189,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/test10.proto b/js/test10.proto
new file mode 100644
index 00000000..9fa5256c
--- /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
new file mode 100644
index 00000000..9f680852
--- /dev/null
+++ b/js/test9.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 = "proto2";
+
+package jspb.exttest.strict.nine;
+
+message Simple9 {
+  required string a_string = 1;
+  repeated string a_repeated_string = 2;
+  optional bool a_boolean = 3;
+}
diff --git a/src/google/protobuf/compiler/js/js_generator.cc b/src/google/protobuf/compiler/js/js_generator.cc
index dfa60068..d25a3668 100644
--- 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,19 @@ 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
+        GOOGLE_CHECK(namespaceObject.compare(0, 6, "proto."));
+        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);
+      }
     }
   }
 }
@@ -3325,6 +3337,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") {
@@ -3434,15 +3448,23 @@ 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();
       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));
@@ -3485,6 +3507,9 @@ void Generator::GenerateFile(const GeneratorOptions& options,
   if (options.import_style == GeneratorOptions::kImportCommonJs && !provided.empty()) {
     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.
diff --git a/src/google/protobuf/compiler/js/js_generator.h b/src/google/protobuf/compiler/js/js_generator.h
index 3cc60e22..b50ef7fd 100644
--- 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()