fix(ngcc): do not collect private declarations from external packages (#34811)

Previously, while trying to build an `NgccReflectionHost`'s
`privateDtsDeclarationMap`, `computePrivateDtsDeclarationMap()` would
try to collect exported declarations from all source files of the
program (i.e. without checking whether they were within the target
package, as happens for declarations in `.d.ts` files).

Most of the time, that would not be a problem, because external packages
would be represented as `.d.ts` files in the program. But when an
external package had no typings, the JS files would be used instead. As
a result, the `ReflectionHost` would try to (unnecessarilly) parse the
file in order to extract exported declarations, which in turn would be
harmless in most cases.

There are certain cases, though, where the `ReflectionHost` would throw
an error, because it cannot parse the external package's JS file. This
could happen, for example, in `UmdReflectionHost`, which expects the
file to contain exactly one statement. See #34544 for more details on a
real-world failure.

This commit fixes the issue by ensuring that
`computePrivateDtsDeclarationMap()` will only collect exported
declarations from files within the target package.

Jira issue: [FW-1794](https://angular-team.atlassian.net/browse/FW-1794)

Fixes #34544

PR Close #34811

integration/ngcc/debug-test.sh

@@ -15,7 +15,7 @@ node $(pwd)/../../scripts/build-packages-dist.js
 
 # Workaround https://github.com/yarnpkg/yarn/issues/2165
 # Yarn will cache file://dist URIs and not update Angular code
-readonly cache=../.yarn_local_cache
+export readonly cache=../.yarn_local_cache
 function rm_cache {
   rm -rf $cache
 }

integration/ngcc/package.json

@@ -13,6 +13,7 @@
     "@angular/material": "9.0.0-rc.4",
     "@angular/platform-browser": "file:../../dist/packages-dist/platform-browser",
     "@angular/platform-browser-dynamic": "file:../../dist/packages-dist/platform-browser-dynamic",
+    "@angular/platform-server": "file:../../dist/packages-dist/platform-server",
     "@angular/router": "file:../../dist/packages-dist/router",
     "@types/node": "file:../../node_modules/@types/node",
     "rxjs": "file:../../node_modules/rxjs",

integration/ngcc/test.sh

@@ -140,6 +140,17 @@ assertSucceeded "Expected 'ngcc' to log 'Compiling'."
   assertSucceeded "Expected 'ngcc' to not add trailing commas to factory function parameters in UMD."
 
 
+# Can it compile `@angular/platform-server` in UMD + typings without errors?
+# (The CLI prefers the `main` property (which maps to UMD) over `module` when compiling `@angular/platform-server`.
+# See https://github.com/angular/angular-cli/blob/e36853338/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/server.ts#L34)
+  rm -rf node_modules/@angular/platform-server && \
+    yarn install --cache-folder $cache --check-files && \
+    test -d node_modules/@angular/platform-server
+  assertSucceeded "Expected to re-install '@angular/platform-server'."
+
+  ngcc --properties main --target @angular/platform-server
+  assertSucceeded "Expected 'ngcc' to successfully compile '@angular/platform-server' (main)."
+
 # Can it be safely run again (as a noop)?
 # And check that it logged skipping compilation as expected
 ngcc -l debug | grep 'Skipping'

integration/ngcc/yarn.lock

@@ -49,6 +49,12 @@
 "@angular/platform-browser@file:../../dist/packages-dist/platform-browser":
   version "9.0.0-rc.1"
 
+"@angular/platform-server@file:../../dist/packages-dist/platform-server":
+  version "9.0.0-rc.1"
+  dependencies:
+    domino "^2.1.2"
+    xhr2 "^0.1.4"
+
 "@angular/router@file:../../dist/packages-dist/router":
   version "9.0.0-rc.1"
 
@@ -853,6 +859,11 @@ dev-ip@^1.0.1:
   resolved "https://registry.yarnpkg.com/dev-ip/-/dev-ip-1.0.1.tgz#a76a3ed1855be7a012bb8ac16cb80f3c00dc28f0"
   integrity sha1-p2o+0YVb56ASu4rBbLgPPADcKPA=
 
+domino@^2.1.2:
+  version "2.1.4"
+  resolved "https://registry.yarnpkg.com/domino/-/domino-2.1.4.tgz#78922e7fab7c610f35792b6c745b7962d342e9c4"
+  integrity sha512-l70mlQ7IjPKC8kT7GljQXJZmt5OqFL+RE91ik5y5WWQtsd9wP8R7gpFnNu96fK5MqAAZRXfLLsnzKtkty5fWGQ==
+
 easy-extender@^2.3.4:
   version "2.3.4"
   resolved "https://registry.yarnpkg.com/easy-extender/-/easy-extender-2.3.4.tgz#298789b64f9aaba62169c77a2b3b64b4c9589b8f"
@@ -3479,6 +3490,11 @@ ws@~3.3.1:
     safe-buffer "~5.1.0"
     ultron "~1.1.0"
 
+xhr2@^0.1.4:
+  version "0.1.4"
+  resolved "https://registry.yarnpkg.com/xhr2/-/xhr2-0.1.4.tgz#7f87658847716db5026323812f818cadab387a5f"
+  integrity sha1-f4dliEdxbbUCYyOBL4GMras4el8=
+
 xml2js@^0.4.17:
   version "0.4.19"
   resolved "https://registry.yarnpkg.com/xml2js/-/xml2js-0.4.19.tgz#686c20f213209e94abf0d1bcf1efaa291c7827a7"

integration/run_tests.sh

@@ -38,7 +38,7 @@ fi
 
 # Workaround https://github.com/yarnpkg/yarn/issues/2165
 # Yarn will cache file://dist URIs and not update Angular code
-readonly cache=.yarn_local_cache
+export readonly cache=.yarn_local_cache
 function rm_cache {
   rm -rf $cache
 }

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

@@ -50,7 +50,7 @@ export const CONSTRUCTOR_PARAMS = 'ctorParameters' as ts.__String;
  */
 export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements NgccReflectionHost {
   /**
-   * A mapping from source declarations typings declarations, which are both publicly exported.
+   * A mapping from source declarations to typings declarations, which are both publicly exported.
    *
    * There should be one entry for every public export visible from the root file of the source
    * tree. Note that by definition the key and value declarations will not be in the same TS
@@ -1573,14 +1573,14 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
       Map<ts.Declaration, ts.Declaration> {
     const declarationMap = new Map<ts.Declaration, ts.Declaration>();
     const dtsDeclarationMap = new Map<string, ts.Declaration>();
-    const dtsFiles = getNonRootFiles(dts);
     const typeChecker = dts.program.getTypeChecker();
+
+    const dtsFiles = getNonRootPackageFiles(dts);
     for (const dtsFile of dtsFiles) {
-      if (isWithinPackage(dts.package, dtsFile)) {
       this.collectDtsExportedDeclarations(dtsDeclarationMap, dtsFile, typeChecker);
     }
-    }
+
-    const srcFiles = getNonRootFiles(src);
+    const srcFiles = getNonRootPackageFiles(src);
     for (const srcFile of srcFiles) {
       this.collectSrcExportedDeclarations(declarationMap, dtsDeclarationMap, srcFile);
     }
@@ -2070,7 +2070,8 @@ function getRootFileOrFail(bundle: BundleProgram): ts.SourceFile {
   return rootFile;
 }
 
-function getNonRootFiles(bundle: BundleProgram): ts.SourceFile[] {
+function getNonRootPackageFiles(bundle: BundleProgram): ts.SourceFile[] {
   const rootFile = bundle.program.getSourceFile(bundle.path);
-  return bundle.program.getSourceFiles().filter(f => f !== rootFile);
+  return bundle.program.getSourceFiles().filter(
+      f => (f !== rootFile) && isWithinPackage(bundle.package, f));
 }

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

@@ -2184,7 +2184,7 @@ exports.ExternalModule = ExternalModule;
         });
       });
 
-      describe('getDtsDeclarationsOfClass()', () => {
+      describe('getDtsDeclaration()', () => {
         it('should find the dts declaration that has the same relative path to the source file',
            () => {
              loadTestFiles(TYPINGS_SRC_FILES);

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

@@ -547,6 +547,7 @@ runInEachFileSystem(() => {
           name: _('/ep/src/index.js'),
           contents: `
         import 'an_external_lib';
+        import 'an_external_lib_without_typings';
         import {InternalClass} from './internal';
         import * as func1 from './func1';
         import * as missing from './missing-class';
@@ -579,6 +580,10 @@ runInEachFileSystem(() => {
           name: _('/ep/src/shadow-class.js'),
           contents: 'export class ShadowClass {}',
         },
+        {
+          name: _('/an_external_lib_without_typings/index.js'),
+          contents: '// Some content.',
+        },
       ];
 
       TYPINGS_DTS_FILES = [
@@ -1958,7 +1963,7 @@ runInEachFileSystem(() => {
       });
     });
 
-    describe('getDtsDeclarationsOfClass()', () => {
+    describe('getDtsDeclaration()', () => {
       it('should find the dts declaration that has the same relative path to the source file',
          () => {
            loadTestFiles(TYPINGS_SRC_FILES);
@@ -2019,15 +2024,41 @@ runInEachFileSystem(() => {
             getRootFiles(TYPINGS_SRC_FILES)[0], false, [_('/ep/src/shadow-class.js')]);
         const dts = makeTestBundleProgram(
             getRootFiles(TYPINGS_DTS_FILES)[0], false, [_('/ep/typings/shadow-class.d.ts')]);
-        const missingClass = getDeclaration(
+        const shadowClass = getDeclaration(
             bundle.program, _('/ep/src/shadow-class.js'), 'ShadowClass', isNamedClassDeclaration);
         const host = new Esm2015ReflectionHost(new MockLogger(), false, bundle, dts);
 
-        const dtsDecl = host.getDtsDeclaration(missingClass) !;
+        const dtsDecl = host.getDtsDeclaration(shadowClass) !;
         expect(dtsDecl).not.toBeNull();
         expect(dtsDecl.getSourceFile().fileName).toEqual(_('/ep/typings/shadow-class.d.ts'));
       });
 
+      it('should ignore source files outside of the entrypoint', () => {
+        const externalLibWithoutTypingsIndex = _('/an_external_lib_without_typings/index.js');
+
+        class TestEsm2015ReflectionHost extends Esm2015ReflectionHost {
+          getExportsOfModule(node: ts.Node) {
+            if (ts.isSourceFile(node) && (node.fileName === externalLibWithoutTypingsIndex)) {
+              throw new Error(
+                  `'getExportsOfModule()' called on '${externalLibWithoutTypingsIndex}'.`);
+            }
+            return super.getExportsOfModule(node);
+          }
+        }
+
+        loadTestFiles(TYPINGS_SRC_FILES);
+        loadTestFiles(TYPINGS_DTS_FILES);
+        const bundle = makeTestBundleProgram(
+            getRootFiles(TYPINGS_SRC_FILES)[0], false, [externalLibWithoutTypingsIndex]);
+        const dts = makeTestBundleProgram(getRootFiles(TYPINGS_DTS_FILES)[0]);
+        const missingClass = getDeclaration(
+            bundle.program, _('/ep/src/missing-class.js'), 'MissingClass2',
+            isNamedClassDeclaration);
+        const host = new TestEsm2015ReflectionHost(new MockLogger(), false, bundle, dts);
+
+        expect(host.getDtsDeclaration(missingClass)).toBeNull();
+      });
+
       it('should find the dts file that contains a matching class declaration, even if the source files do not match',
          () => {
            loadTestFiles(TYPINGS_SRC_FILES);

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

@@ -2450,7 +2450,7 @@ runInEachFileSystem(() => {
       });
     });
 
-    describe('getDtsDeclarationsOfClass()', () => {
+    describe('getDtsDeclaration()', () => {
       it('should find the dts declaration that has the same relative path to the source file',
          () => {
            loadTestFiles(TYPINGS_SRC_FILES);

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

@@ -2360,7 +2360,7 @@ runInEachFileSystem(() => {
       });
     });
 
-    describe('getDtsDeclarationsOfClass()', () => {
+    describe('getDtsDeclaration()', () => {
       it('should find the dts declaration that has the same relative path to the source file',
          () => {
            loadTestFiles(TYPINGS_SRC_FILES);