fix(ngcc): recognize re-exports with imported TS helpers in CommonJS and UMD (#34527)

Previously, the `CommonJsReflectionHost` and `UmdReflectionHost` would
only recognize re-exports of the form `__export(...)`. This is what
re-exports look like, when the TypeScript helpers are emitted inline
(i.e. when compiling with the default [TypeScript compiler options][1]
that include `noEmitHelpers: false` and `importHelpers: false`).

However, when compiling with `importHelpers: true` and [tslib][2] (which
is the recommended way for optimized bundles), the re-exports will look
like: `tslib_1.__exportStar(..., exports)`
These types of re-exports were previously not recognized by the
CommonJS/UMD `ReflectionHost`s and thus ignored.

This commit fixes this by ensuring both re-export formats are
recognized.

[1]: https://www.typescriptlang.org/docs/handbook/compiler-options.html
[2]: https://www.npmjs.com/package/tslib

PR Close #34527

packages/compiler-cli/ngcc/src/host/commonjs_umd_utils.ts

@@ -19,6 +19,21 @@ export interface ExportStatement extends ts.ExpressionStatement {
   expression: ts.BinaryExpression&{left: ts.PropertyAccessExpression & {expression: ts.Identifier}};
 }
 
+/**
+ * A CommonJS or UMD re-export statement.
+ *
+ * In CommonJS/UMD, re-export statements can have several forms (depending, for example, on whether
+ * the TypeScript helpers are imported or emitted inline). The expression can have one of the
+ * following forms:
+ * - `__export(firstArg)`
+ * - `__exportStar(firstArg)`
+ * - `tslib.__export(firstArg, exports)`
+ * - `tslib.__exportStar(firstArg, exports)`
+ *
+ * In all cases, we only care about `firstApp`, which is the first argument of the re-export call
+ * expression and can be either a `require('...')` call or an identifier (initialized via a
+ * `require('...')` call).
+ */
 export interface ReexportStatement extends ts.ExpressionStatement { expression: ts.CallExpression; }
 
 export interface RequireCall extends ts.CallExpression {
@@ -66,12 +81,47 @@ export function isExportStatement(stmt: ts.Statement): stmt is ExportStatement {
 
 /**
  * Check whether the specified `ts.Statement` is a re-export statement, i.e. an expression statement
- * of the form: `__export(<foo>)`
+ * of one of the following forms:
+ * - `__export(<foo>)`
+ * - `__exportStar(<foo>)`
+ * - `tslib.__export(<foo>, exports)`
+ * - `tslib.__exportStar(<foo>, exports)`
  */
 export function isReexportStatement(stmt: ts.Statement): stmt is ReexportStatement {
-  return ts.isExpressionStatement(stmt) && ts.isCallExpression(stmt.expression) &&
+  // Ensure it is a call expression statement.
-      ts.isIdentifier(stmt.expression.expression) &&
+  if (!ts.isExpressionStatement(stmt) || !ts.isCallExpression(stmt.expression)) {
-      stmt.expression.expression.text === '__export' && stmt.expression.arguments.length === 1;
+    return false;
+  }
+
+  // Get the called function identifier.
+  // NOTE: Currently, it seems that `__export()` is used when emitting helpers inline and
+  //       `__exportStar()` when importing them
+  //       ([source](https://github.com/microsoft/TypeScript/blob/d7c83f023/src/compiler/transformers/module/module.ts#L1796-L1797)).
+  //       So, theoretically, we only care about the formats `__export(<foo>)` and
+  //       `tslib.__exportStar(<foo>, exports)`.
+  //       The current implementation accepts the other two formats (`__exportStar(...)` and
+  //       `tslib.__export(...)`) as well to be more future-proof (given that it is unlikely that
+  //       they will introduce false positives).
+  let fnName: string|null = null;
+  if (ts.isIdentifier(stmt.expression.expression)) {
+    // Statement of the form `someFn(...)`.
+    fnName = stmt.expression.expression.text;
+  } else if (
+      ts.isPropertyAccessExpression(stmt.expression.expression) &&
+      ts.isIdentifier(stmt.expression.expression.name)) {
+    // Statement of the form `tslib.someFn(...)`.
+    fnName = stmt.expression.expression.name.text;
+  }
+
+  // Ensure the called function is either `__export()` or `__exportStar()`.
+  if ((fnName !== '__export') && (fnName !== '__exportStar')) {
+    return false;
+  }
+
+  // Ensure there is at least one argument.
+  // (The first argument is the exported thing and there will be a second `exports` argument in the
+  // case of imported helpers).
+  return stmt.expression.arguments.length > 0;
 }
 
 /**

packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts

@@ -510,7 +510,8 @@ var c = file_a.a;
           var a_module = require('./a_module');
           var b_module = require('./b_module');
           var xtra_module = require('./xtra_module');
-          var wildcard_reexports = require('./wildcard_reexports');
+          var wildcard_reexports_emitted_helpers = require('./wildcard_reexports_emitted_helpers');
+          var wildcard_reexports_imported_helpers = require('./wildcard_reexports_imported_helpers');
           `
         },
         {
@@ -552,15 +553,24 @@ exports.xtra2 = xtra2;
 `,
         },
         {
-          name: _('/wildcard_reexports.js'),
+          name: _('/wildcard_reexports_emitted_helpers.js'),
           contents: `
-    function __export(m) {
+function __export(m) {
   for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
-    }
+}
-    var b_module = require("./b_module");
+var b_module = require("./b_module");
-    __export(b_module);
+__export(b_module);
-    __export(require("./xtra_module"));
+__export(require("./xtra_module"));
-    `
+`,
+        },
+        {
+          name: _('/wildcard_reexports_imported_helpers.js'),
+          contents: `
+var tslib_1 = require("tslib");
+var b_module = require("./b_module");
+tslib_1.__exportStar(b_module, exports);
+tslib_1.__exportStar(require("./xtra_module"), exports);
+`,
         },
       ];
 
@@ -1763,12 +1773,42 @@ exports.ExternalModule = ExternalModule;
               ]);
         });
 
-        it('should handle wildcard re-exports of other modules', () => {
+        it('should handle wildcard re-exports of other modules (with emitted helpers)', () => {
           loadFakeCore(getFileSystem());
           loadTestFiles(EXPORTS_FILES);
           const bundle = makeTestBundleProgram(_('/index.js'));
           const host = new CommonJsReflectionHost(new MockLogger(), false, bundle);
-          const file = getSourceFileOrError(bundle.program, _('/wildcard_reexports.js'));
+          const file =
+              getSourceFileOrError(bundle.program, _('/wildcard_reexports_emitted_helpers.js'));
+          const exportDeclarations = host.getExportsOfModule(file);
+          expect(exportDeclarations).not.toBe(null);
+          expect(Array.from(exportDeclarations !.entries())
+                     .map(entry => [entry[0], entry[1].node !.getText(), entry[1].viaModule]))
+              .toEqual([
+                ['Directive', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')],
+                ['a', `a = 'a'`, _('/b_module')],
+                ['b', `b = a_module.a`, _('/b_module')],
+                ['c', `a = 'a'`, _('/b_module')],
+                ['d', `b = a_module.a`, _('/b_module')],
+                ['e', `e = 'e'`, _('/b_module')],
+                ['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')],
+                [
+                  'SomeClass',
+                  `SomeClass = (function() {\n  function SomeClass() {}\n  return SomeClass;\n}())`,
+                  _('/b_module')
+                ],
+                ['xtra1', `xtra1 = 'xtra1'`, _('/xtra_module')],
+                ['xtra2', `xtra2 = 'xtra2'`, _('/xtra_module')],
+              ]);
+        });
+
+        it('should handle wildcard re-exports of other modules (with imported helpers)', () => {
+          loadFakeCore(getFileSystem());
+          loadTestFiles(EXPORTS_FILES);
+          const bundle = makeTestBundleProgram(_('/index.js'));
+          const host = new CommonJsReflectionHost(new MockLogger(), false, bundle);
+          const file =
+              getSourceFileOrError(bundle.program, _('/wildcard_reexports_imported_helpers.js'));
           const exportDeclarations = host.getExportsOfModule(file);
           expect(exportDeclarations).not.toBe(null);
           expect(Array.from(exportDeclarations !.entries())

packages/compiler-cli/ngcc/test/host/umd_host_spec.ts

@@ -564,10 +564,10 @@ runInEachFileSystem(() => {
           name: _('/index.js'),
           contents: `
           (function (global, factory) {
-            typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('./a_module'), require('./b_module'), require('./wildcard_reexports'), require('./wildcard_reexports_with_require')) :
+            typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('./a_module'), require('./b_module'), require('./wildcard_reexports'), require('./wildcard_reexports_imported_helpers'), require('./wildcard_reexports_with_require')) :
-            typeof define === 'function' && define.amd ? define('index', ['exports', './a_module', './b_module', './wildcard_reexports', './wildcard_reexports_with_require'], factory) :
+            typeof define === 'function' && define.amd ? define('index', ['exports', './a_module', './b_module', './wildcard_reexports', './wildcard_reexports_imported_helpers', './wildcard_reexports_with_require'], factory) :
-            (factory(global.index, global.a_module, global.b_module, global.wildcard_reexports, global.wildcard_reexports_with_require));
+            (factory(global.index, global.a_module, global.b_module, global.wildcard_reexports, global.wildcard_reexports_imported_helpers, global.wildcard_reexports_with_require));
-          }(this, (function (exports, a_module, b_module, wildcard_reexports, wildcard_reexports_with_require) { 'use strict';
+          }(this, (function (exports, a_module, b_module, wildcard_reexports, wildcard_reexports_imported_helpers, wildcard_reexports_with_require) { 'use strict';
           })));
           `
         },
@@ -635,6 +635,18 @@ runInEachFileSystem(() => {
   }
   __export(b_module);
   __export(xtra_module);
+})));`,
+        },
+        {
+          name: _('/wildcard_reexports_imported_helpers.js'),
+          contents: `
+(function (global, factory) {
+  typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('tslib'), require('./b_module'), require('./xtra_module')) :
+  typeof define === 'function' && define.amd ? define('wildcard_reexports', ['exports', 'tslib', './b_module', './xtra_module'], factory) :
+  (factory(global.wildcard_reexports_imported_helpers, tslib, b_module, xtra_module));
+}(this, (function (exports, tslib, b_module, xtra_module) { 'use strict';
+  tslib.__exportStar(b_module, exports);
+  tslib.__exportStar(xtra_module, exports);
 })));`,
         },
         {
@@ -1947,7 +1959,7 @@ runInEachFileSystem(() => {
         expect(classSymbol !.implementation.valueDeclaration).toBe(node);
       });
 
-      it('should handle wildcard re-exports of other modules', () => {
+      it('should handle wildcard re-exports of other modules (with emitted helpers)', () => {
         loadFakeCore(getFileSystem());
         loadTestFiles(EXPORTS_FILES);
         const bundle = makeTestBundleProgram(_('/index.js'));
@@ -1975,6 +1987,35 @@ runInEachFileSystem(() => {
             ]);
       });
 
+      it('should handle wildcard re-exports of other modules (with imported helpers)', () => {
+        loadFakeCore(getFileSystem());
+        loadTestFiles(EXPORTS_FILES);
+        const bundle = makeTestBundleProgram(_('/index.js'));
+        const host = new UmdReflectionHost(new MockLogger(), false, bundle);
+        const file =
+            getSourceFileOrError(bundle.program, _('/wildcard_reexports_imported_helpers.js'));
+        const exportDeclarations = host.getExportsOfModule(file);
+        expect(exportDeclarations).not.toBe(null);
+        expect(Array.from(exportDeclarations !.entries())
+                   .map(entry => [entry[0], entry[1].node !.getText(), entry[1].viaModule]))
+            .toEqual([
+              ['Directive', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')],
+              ['a', `a = 'a'`, _('/b_module')],
+              ['b', `b = a_module.a`, _('/b_module')],
+              ['c', `a = 'a'`, _('/b_module')],
+              ['d', `b = a_module.a`, _('/b_module')],
+              ['e', `e = 'e'`, _('/b_module')],
+              ['DirectiveX', `Directive: FnWithArg<(clazz: any) => any>`, _('/b_module')],
+              [
+                'SomeClass',
+                `SomeClass = (function() {\n    function SomeClass() {}\n    return SomeClass;\n  }())`,
+                _('/b_module')
+              ],
+              ['xtra1', `xtra1 = 'xtra1'`, _('/xtra_module')],
+              ['xtra2', `xtra2 = 'xtra2'`, _('/xtra_module')],
+            ]);
+      });
+
       it('should handle wildcard re-exports of other modules using `require()` calls', () => {
         loadFakeCore(getFileSystem());
         loadTestFiles(EXPORTS_FILES);