From 321da5cc83b3b8d5019de9dc9acf5da3dba125eb Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Sun, 28 Apr 2019 20:47:56 +0100 Subject: [PATCH] refactor(compiler-cli): ngcc - track non-Angular entry-points (#29643) Previously we completely ignored entry-points that had not been compiled with Angular, since we do not need to compile them with ngcc. But this makes it difficult to reason about dependencies between entry-points that were compiled with Angular and those that were not. Now we do track these non-Angular compiled entry-points but they are marked as `compiledByAngular: false`. PR Close #29643 --- .../ngcc/src/packages/dependency_resolver.ts | 16 +++-- .../ngcc/src/packages/entry_point.ts | 6 +- .../test/packages/dependency_resolver_spec.ts | 58 +++++++++++++------ .../ngcc/test/packages/entry_point_spec.ts | 27 +++++---- 4 files changed, 69 insertions(+), 38 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/packages/dependency_resolver.ts b/packages/compiler-cli/ngcc/src/packages/dependency_resolver.ts index ef37a548a3..15e1c3d3f8 100644 --- a/packages/compiler-cli/ngcc/src/packages/dependency_resolver.ts +++ b/packages/compiler-cli/ngcc/src/packages/dependency_resolver.ts @@ -83,8 +83,12 @@ export class DependencyResolver { let sortedEntryPointNodes: string[]; if (target) { - sortedEntryPointNodes = graph.dependenciesOf(target.path); - sortedEntryPointNodes.push(target.path); + if (target.compiledByAngular) { + sortedEntryPointNodes = graph.dependenciesOf(target.path); + sortedEntryPointNodes.push(target.path); + } else { + sortedEntryPointNodes = []; + } } else { sortedEntryPointNodes = graph.overallOrder(); } @@ -107,11 +111,13 @@ export class DependencyResolver { const ignoredDependencies: IgnoredDependency[] = []; const graph = new DepGraph(); - // Add the entry points to the graph as nodes - entryPoints.forEach(entryPoint => graph.addNode(entryPoint.path, entryPoint)); + const angularEntryPoints = entryPoints.filter(entryPoint => entryPoint.compiledByAngular); + + // Add the Angular compiled entry points to the graph as nodes + angularEntryPoints.forEach(entryPoint => graph.addNode(entryPoint.path, entryPoint)); // Now add the dependencies between them - entryPoints.forEach(entryPoint => { + angularEntryPoints.forEach(entryPoint => { const entryPointPath = getEntryPointPath(entryPoint); const {dependencies, missing, deepImports} = this.host.computeDependencies(entryPointPath); diff --git a/packages/compiler-cli/ngcc/src/packages/entry_point.ts b/packages/compiler-cli/ngcc/src/packages/entry_point.ts index a51ce1f25f..5f1d92fe26 100644 --- a/packages/compiler-cli/ngcc/src/packages/entry_point.ts +++ b/packages/compiler-cli/ngcc/src/packages/entry_point.ts @@ -33,6 +33,8 @@ export interface EntryPoint { path: AbsoluteFsPath; /** The path to a typings (.d.ts) file for this entry-point. */ typings: AbsoluteFsPath; + /** Is this EntryPoint compiled with the Angular View Engine compiler? */ + compiledByAngular: boolean; } interface PackageJsonFormatProperties { @@ -89,9 +91,6 @@ export function getEntryPointInfo( // Also there must exist a `metadata.json` file next to the typings entry-point. const metadataPath = path.resolve(entryPointPath, typings.replace(/\.d\.ts$/, '') + '.metadata.json'); - if (!fs.existsSync(metadataPath)) { - return null; - } const entryPointInfo: EntryPoint = { name: entryPointPackageJson.name, @@ -99,6 +98,7 @@ export function getEntryPointInfo( package: packagePath, path: entryPointPath, typings: AbsoluteFsPath.from(path.resolve(entryPointPath, typings)), + compiledByAngular: fs.existsSync(metadataPath), }; return entryPointInfo; diff --git a/packages/compiler-cli/ngcc/test/packages/dependency_resolver_spec.ts b/packages/compiler-cli/ngcc/test/packages/dependency_resolver_spec.ts index 60c8f16d30..1618ab8f0e 100644 --- a/packages/compiler-cli/ngcc/test/packages/dependency_resolver_spec.ts +++ b/packages/compiler-cli/ngcc/test/packages/dependency_resolver_spec.ts @@ -21,18 +21,38 @@ describe('DependencyResolver', () => { resolver = new DependencyResolver(new MockLogger(), host); }); describe('sortEntryPointsByDependency()', () => { - const first = { path: _('/first'), packageJson: {esm5: 'index.ts'} } as EntryPoint; - const second = { path: _('/second'), packageJson: {esm2015: 'sub/index.ts'} } as EntryPoint; - const third = { path: _('/third'), packageJson: {esm5: 'index.ts'} } as EntryPoint; - const fourth = { path: _('/fourth'), packageJson: {esm2015: 'sub2/index.ts'} } as EntryPoint; - const fifth = { path: _('/fifth'), packageJson: {esm5: 'index.ts'} } as EntryPoint; + const first = { + path: _('/first'), + packageJson: {esm5: './index.js'}, + compiledByAngular: true + } as EntryPoint; + const second = { + path: _('/second'), + packageJson: {esm2015: './sub/index.js'}, + compiledByAngular: true + } as EntryPoint; + const third = { + path: _('/third'), + packageJson: {fesm5: './index.js'}, + compiledByAngular: true + } as EntryPoint; + const fourth = { + path: _('/fourth'), + packageJson: {fesm2015: './sub2/index.js'}, + compiledByAngular: true + } as EntryPoint; + const fifth = { + path: _('/fifth'), + packageJson: {module: './index.js'}, + compiledByAngular: true + } as EntryPoint; const dependencies = { - [_('/first/index.ts')]: {resolved: [second.path, third.path, '/ignored-1'], missing: []}, - [_('/second/sub/index.ts')]: {resolved: [third.path, fifth.path], missing: []}, - [_('/third/index.ts')]: {resolved: [fourth.path, '/ignored-2'], missing: []}, - [_('/fourth/sub2/index.ts')]: {resolved: [fifth.path], missing: []}, - [_('/fifth/index.ts')]: {resolved: [], missing: []}, + [_('/first/index.js')]: {resolved: [second.path, third.path, '/ignored-1'], missing: []}, + [_('/second/sub/index.js')]: {resolved: [third.path, fifth.path], missing: []}, + [_('/third/index.js')]: {resolved: [fourth.path, '/ignored-2'], missing: []}, + [_('/fourth/sub2/index.js')]: {resolved: [fifth.path], missing: []}, + [_('/fifth/index.js')]: {resolved: [], missing: []}, }; it('should order the entry points by their dependency on each other', () => { @@ -43,8 +63,8 @@ describe('DependencyResolver', () => { it('should remove entry-points that have missing direct dependencies', () => { spyOn(host, 'computeDependencies').and.callFake(createFakeComputeDependencies({ - [_('/first/index.ts')]: {resolved: [], missing: ['/missing']}, - [_('/second/sub/index.ts')]: {resolved: [], missing: []}, + [_('/first/index.js')]: {resolved: [], missing: ['/missing']}, + [_('/second/sub/index.js')]: {resolved: [], missing: []}, })); const result = resolver.sortEntryPointsByDependency([first, second]); expect(result.entryPoints).toEqual([second]); @@ -55,9 +75,9 @@ describe('DependencyResolver', () => { it('should remove entry points that depended upon an invalid entry-point', () => { spyOn(host, 'computeDependencies').and.callFake(createFakeComputeDependencies({ - [_('/first/index.ts')]: {resolved: [second.path], missing: []}, - [_('/second/sub/index.ts')]: {resolved: [], missing: ['/missing']}, - [_('/third/index.ts')]: {resolved: [], missing: []}, + [_('/first/index.js')]: {resolved: [second.path], missing: []}, + [_('/second/sub/index.js')]: {resolved: [], missing: ['/missing']}, + [_('/third/index.js')]: {resolved: [], missing: []}, })); // Note that we will process `first` before `second`, which has the missing dependency. const result = resolver.sortEntryPointsByDependency([first, second, third]); @@ -70,9 +90,9 @@ describe('DependencyResolver', () => { it('should remove entry points that will depend upon an invalid entry-point', () => { spyOn(host, 'computeDependencies').and.callFake(createFakeComputeDependencies({ - [_('/first/index.ts')]: {resolved: [second.path], missing: []}, - [_('/second/sub/index.ts')]: {resolved: [], missing: ['/missing']}, - [_('/third/index.ts')]: {resolved: [], missing: []}, + [_('/first/index.js')]: {resolved: [second.path], missing: []}, + [_('/second/sub/index.js')]: {resolved: [], missing: ['/missing']}, + [_('/third/index.js')]: {resolved: [], missing: []}, })); // Note that we will process `first` after `second`, which has the missing dependency. const result = resolver.sortEntryPointsByDependency([second, first, third]); @@ -85,7 +105,7 @@ describe('DependencyResolver', () => { it('should error if the entry point does not have either the esm5 nor esm2015 formats', () => { expect(() => resolver.sortEntryPointsByDependency([ - { path: '/first', packageJson: {} } as EntryPoint + { path: '/first', packageJson: {}, compiledByAngular: true } as EntryPoint ])).toThrowError(`There is no format with import statements in '/first' entry-point.`); }); diff --git a/packages/compiler-cli/ngcc/test/packages/entry_point_spec.ts b/packages/compiler-cli/ngcc/test/packages/entry_point_spec.ts index c360d80dfd..a4c028d328 100644 --- a/packages/compiler-cli/ngcc/test/packages/entry_point_spec.ts +++ b/packages/compiler-cli/ngcc/test/packages/entry_point_spec.ts @@ -30,6 +30,7 @@ describe('getEntryPointInfo()', () => { path: _('/some_package/valid_entry_point'), typings: _(`/some_package/valid_entry_point/valid_entry_point.d.ts`), packageJson: loadPackageJson('/some_package/valid_entry_point'), + compiledByAngular: true, }); }); @@ -45,17 +46,19 @@ describe('getEntryPointInfo()', () => { expect(entryPoint).toBe(null); }); - it('should return null if there is no esm2015 nor fesm2015 field in the package.json', () => { - const entryPoint = - getEntryPointInfo(new MockLogger(), SOME_PACKAGE, _('/some_package/missing_esm2015')); - expect(entryPoint).toBe(null); - }); - - it('should return null if there is no metadata.json file next to the typing file', () => { - const entryPoint = - getEntryPointInfo(new MockLogger(), SOME_PACKAGE, _('/some_package/missing_metadata.json')); - expect(entryPoint).toBe(null); - }); + it('should return an object with `compiledByAngular` set to false if there is no metadata.json file next to the typing file', + () => { + const entryPoint = + getEntryPointInfo(new MockLogger(), SOME_PACKAGE, _('/some_package/missing_metadata')); + expect(entryPoint).toEqual({ + name: 'some-package/missing_metadata', + package: SOME_PACKAGE, + path: _('/some_package/missing_metadata'), + typings: _(`/some_package/missing_metadata/missing_metadata.d.ts`), + packageJson: loadPackageJson('/some_package/missing_metadata'), + compiledByAngular: false, + }); + }); it('should work if the typings field is named `types', () => { const entryPoint = getEntryPointInfo( @@ -66,6 +69,7 @@ describe('getEntryPointInfo()', () => { path: _('/some_package/types_rather_than_typings'), typings: _(`/some_package/types_rather_than_typings/types_rather_than_typings.d.ts`), packageJson: loadPackageJson('/some_package/types_rather_than_typings'), + compiledByAngular: true, }); }); @@ -78,6 +82,7 @@ describe('getEntryPointInfo()', () => { path: _('/some_package/material_style'), typings: _(`/some_package/material_style/material_style.d.ts`), packageJson: JSON.parse(readFileSync('/some_package/material_style/package.json', 'utf8')), + compiledByAngular: true, }); });