From 5b42084912f6bc8bdc419b0893fe3e25351719dc Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Wed, 15 Jan 2020 21:54:36 +0200 Subject: [PATCH] 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 | 2 +- integration/ngcc/package.json | 1 + integration/ngcc/test.sh | 11 ++++++ integration/ngcc/yarn.lock | 16 ++++++++ integration/run_tests.sh | 2 +- .../ngcc/src/host/esm2015_host.ts | 17 +++++---- .../ngcc/test/host/commonjs_host_spec.ts | 2 +- .../ngcc/test/host/esm2015_host_spec.ts | 37 +++++++++++++++++-- .../ngcc/test/host/esm5_host_spec.ts | 2 +- .../ngcc/test/host/umd_host_spec.ts | 2 +- 10 files changed, 76 insertions(+), 16 deletions(-) diff --git a/integration/ngcc/debug-test.sh b/integration/ngcc/debug-test.sh index fc9aca7945..d7ed06be59 100755 --- a/integration/ngcc/debug-test.sh +++ b/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 } diff --git a/integration/ngcc/package.json b/integration/ngcc/package.json index f3cb039f52..9df0ea3e3f 100644 --- a/integration/ngcc/package.json +++ b/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", diff --git a/integration/ngcc/test.sh b/integration/ngcc/test.sh index e17972a122..32adc70f10 100755 --- a/integration/ngcc/test.sh +++ b/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' diff --git a/integration/ngcc/yarn.lock b/integration/ngcc/yarn.lock index d701229ecb..197e362d42 100644 --- a/integration/ngcc/yarn.lock +++ b/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" diff --git a/integration/run_tests.sh b/integration/run_tests.sh index dac52b3bb4..86688fefa7 100755 --- a/integration/run_tests.sh +++ b/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 } diff --git a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts index fe3f03d9d5..fe498123ec 100644 --- a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts +++ b/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 { const declarationMap = new Map(); const dtsDeclarationMap = new Map(); - 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); - } + 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)); } diff --git a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts index cba513de17..25457342f7 100644 --- a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts +++ b/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); diff --git a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts index 35b3c20cf3..6f086011db 100644 --- a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts +++ b/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); diff --git a/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts b/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts index a17de8642b..845e5358f5 100644 --- a/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts +++ b/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); diff --git a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts index 9ecf3b4042..cb3ffc8d7e 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts +++ b/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);