cargdev/angular
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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
This commit is contained in:
George Kalpakas
2020-01-15 21:54:36 +02:00
committed by Andrew Kushnir
parent 92d26e26ea
commit c80392bcd2
10 changed files with 76 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
packages/compiler-cli/ngcc/src/host/esm2015_host.ts
View File

@ -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);
}
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));
}

2
packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts
View File

@ -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);

37
packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts
View File

@ -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);

2
packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts
View File

@ -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);

2
packages/compiler-cli/ngcc/test/host/umd_host_spec.ts
View File

@ -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);