Skip to content

Commit 9b1bb37

Browse files
petebacondarwinbenlesh
authored andcommittedSep 18, 2018
fix(ivy): ngcc should compile entry-points in the correct order (angular#25862)
The compiler should process all an entry-points dependencies before processing that entry-point. PR Close angular#25862
1 parent 9763898 commit 9b1bb37

21 files changed

+1448
-396
lines changed
 

‎integration/ngcc/.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
yarn.lock

‎integration/ngcc/test.sh

+8-1
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,17 @@ set -e -x
44

55
PATH=$PATH:$(npm bin)
66

7-
ivy-ngcc fesm2015,esm2015
7+
ivy-ngcc --help
8+
9+
# node --inspect-brk $(npm bin)/ivy-ngcc -f esm2015
10+
ivy-ngcc
11+
812
# Did it add the appropriate build markers?
13+
14+
# - fesm2015
915
ls node_modules/@angular/common | grep __modified_by_ngcc_for_fesm2015
1016
if [[ $? != 0 ]]; then exit 1; fi
17+
# - esm2015
1118
ls node_modules/@angular/common | grep __modified_by_ngcc_for_esm2015
1219
if [[ $? != 0 ]]; then exit 1; fi
1320

‎package.json

+2
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@
5757
"@types/shelljs": "^0.7.8",
5858
"@types/source-map": "^0.5.1",
5959
"@types/systemjs": "0.19.32",
60+
"@types/yargs": "^11.1.1",
6061
"@webcomponents/custom-elements": "^1.0.4",
6162
"angular": "npm:angular@1.7",
6263
"angular-1.5": "npm:angular@1.5",
@@ -76,6 +77,7 @@
7677
"conventional-changelog": "1.1.0",
7778
"convert-source-map": "^1.5.1",
7879
"cors": "2.8.4",
80+
"dependency-graph": "^0.7.2",
7981
"diff": "^3.5.0",
8082
"domino": "2.0.1",
8183
"entities": "1.1.1",

‎packages/compiler-cli/package.json

+5-1
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,14 @@
1212
"dependencies": {
1313
"reflect-metadata": "^0.1.2",
1414
"minimist": "^1.2.0",
15+
"canonical-path": "0.0.2",
1516
"chokidar": "^1.4.2",
1617
"convert-source-map": "^1.5.1",
18+
"dependency-graph": "^0.7.2",
1719
"magic-string": "^0.25.0",
18-
"source-map": "^0.6.1"
20+
"shelljs": "^0.8.1",
21+
"source-map": "^0.6.1",
22+
"yargs": "9.0.1"
1923
},
2024
"peerDependencies": {
2125
"@angular/compiler": "0.0.0-PLACEHOLDER",

‎packages/compiler-cli/src/ngcc/canonical-path.d.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,4 +11,4 @@ declare module 'canonical-path' {
1111
export var delimiter: string;
1212
export function parse(p: string): ParsedPath;
1313
export function format(pP: ParsedPath): string;
14-
}
14+
}

‎packages/compiler-cli/src/ngcc/src/main.ts

+44-76
Original file line numberDiff line numberDiff line change
@@ -7,85 +7,53 @@
77
*/
88
import * as path from 'canonical-path';
99
import {existsSync, lstatSync, readFileSync, readdirSync} from 'fs';
10+
import * as yargs from 'yargs';
1011

11-
import {PackageTransformer} from './transform/package_transformer';
12+
import {DependencyHost} from './packages/dependency_host';
13+
import {DependencyResolver} from './packages/dependency_resolver';
14+
import {EntryPointFormat} from './packages/entry_point';
15+
import {EntryPointFinder} from './packages/entry_point_finder';
16+
import {Transformer} from './packages/transformer';
1217

1318
export function mainNgcc(args: string[]): number {
14-
const formats = args[0] ? args[0].split(',') : ['fesm2015', 'esm2015', 'fesm5', 'esm5'];
15-
const packagePaths = args[1] ? [path.resolve(args[1])] : findPackagesToCompile();
16-
const targetPath = args[2] ? args[2] : 'node_modules';
17-
18-
const transformer = new PackageTransformer();
19-
packagePaths.forEach(packagePath => {
20-
formats.forEach(format => {
21-
// TODO: remove before flight
22-
console.warn(`Compiling ${packagePath} : ${format}`);
23-
transformer.transform(packagePath, format, targetPath);
24-
});
25-
});
26-
27-
return 0;
28-
}
29-
30-
// TODO - consider nested node_modules
31-
32-
/**
33-
* Check whether the given folder needs to be included in the ngcc compilation.
34-
* We do not care about folders that are:
35-
*
36-
* - symlinks
37-
* - node_modules
38-
* - do not contain a package.json
39-
* - do not have a typings property in package.json
40-
* - do not have an appropriate metadata.json file
41-
*
42-
* @param folderPath The absolute path to the folder.
43-
*/
44-
function hasMetadataFile(folderPath: string): boolean {
45-
const folderName = path.basename(folderPath);
46-
if (folderName === 'node_modules' || lstatSync(folderPath).isSymbolicLink()) {
47-
return false;
19+
const options =
20+
yargs
21+
.option('s', {
22+
alias: 'source',
23+
describe: 'A path to the root folder to compile.',
24+
default: './node_modules'
25+
})
26+
.option('f', {
27+
alias: 'formats',
28+
array: true,
29+
describe: 'An array of formats to compile.',
30+
default: ['fesm2015', 'esm2015', 'fesm5', 'esm5']
31+
})
32+
.option('t', {
33+
alias: 'target',
34+
describe: 'A path to a root folder where the compiled files will be written.',
35+
defaultDescription: 'The `source` folder.'
36+
})
37+
.help()
38+
.parse(args);
39+
40+
const sourcePath: string = path.resolve(options['s']);
41+
const formats: EntryPointFormat[] = options['f'];
42+
const targetPath: string = options['t'] || sourcePath;
43+
44+
const transformer = new Transformer(sourcePath, targetPath);
45+
const host = new DependencyHost();
46+
const resolver = new DependencyResolver(host);
47+
const finder = new EntryPointFinder(resolver);
48+
49+
try {
50+
const {entryPoints} = finder.findEntryPoints(sourcePath);
51+
entryPoints.forEach(
52+
entryPoint => formats.forEach(format => transformer.transform(entryPoint, format)));
53+
} catch (e) {
54+
console.error(e.stack);
55+
return 1;
4856
}
49-
const packageJsonPath = path.join(folderPath, 'package.json');
50-
if (!existsSync(packageJsonPath)) {
51-
return false;
52-
}
53-
const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8'));
54-
if (!packageJson.typings) {
55-
return false;
56-
}
57-
// TODO: avoid if packageJson contains built marker
58-
const metadataPath =
59-
path.join(folderPath, packageJson.typings.replace(/\.d\.ts$/, '.metadata.json'));
60-
return existsSync(metadataPath);
61-
}
6257

63-
/**
64-
* Look for packages that need to be compiled.
65-
* The function will recurse into folders that start with `@...`, e.g. `@angular/...`.
66-
* Without an argument it starts at `node_modules`.
67-
*/
68-
function findPackagesToCompile(folder: string = 'node_modules'): string[] {
69-
const fullPath = path.resolve(folder);
70-
const packagesToCompile: string[] = [];
71-
readdirSync(fullPath)
72-
.filter(p => !p.startsWith('.'))
73-
.filter(p => lstatSync(path.join(fullPath, p)).isDirectory())
74-
.forEach(p => {
75-
const packagePath = path.join(fullPath, p);
76-
if (p.startsWith('@')) {
77-
packagesToCompile.push(...findPackagesToCompile(packagePath));
78-
} else {
79-
packagesToCompile.push(packagePath);
80-
}
81-
});
82-
83-
return packagesToCompile.filter(path => recursiveDirTest(path, hasMetadataFile));
84-
}
85-
86-
function recursiveDirTest(dir: string, test: (dir: string) => boolean): boolean {
87-
return test(dir) || readdirSync(dir).some(segment => {
88-
const fullPath = path.join(dir, segment);
89-
return lstatSync(fullPath).isDirectory() && recursiveDirTest(fullPath, test);
90-
});
58+
return 0;
9159
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
/**
2+
* @license
3+
* Copyright Google Inc. All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {resolve} from 'canonical-path';
10+
import {existsSync, readFileSync, writeFileSync} from 'fs';
11+
import {EntryPoint, EntryPointFormat} from './entry_point';
12+
13+
export const NGCC_VERSION = '0.0.0-PLACEHOLDER';
14+
15+
function getMarkerPath(entryPointPath: string, format: EntryPointFormat) {
16+
return resolve(entryPointPath, `__modified_by_ngcc_for_${format}__`);
17+
}
18+
19+
/**
20+
* Check whether there is a build marker for the given entry point and format.
21+
* @param entryPoint the entry point to check for a marker.
22+
* @param format the format for which we are checking for a marker.
23+
*/
24+
export function checkMarkerFile(entryPoint: EntryPoint, format: EntryPointFormat): boolean {
25+
const markerPath = getMarkerPath(entryPoint.path, format);
26+
const markerExists = existsSync(markerPath);
27+
if (markerExists) {
28+
const previousVersion = readFileSync(markerPath, 'utf8');
29+
if (previousVersion !== NGCC_VERSION) {
30+
throw new Error(
31+
'The ngcc compiler has changed since the last ngcc build.\n' +
32+
'Please completely remove `node_modules` and try again.');
33+
}
34+
}
35+
return markerExists;
36+
}
37+
38+
export function writeMarkerFile(entryPoint: EntryPoint, format: EntryPointFormat) {
39+
const markerPath = getMarkerPath(entryPoint.path, format);
40+
writeFileSync(markerPath, NGCC_VERSION, 'utf8');
41+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
/**
2+
* @license
3+
* Copyright Google Inc. All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import * as path from 'canonical-path';
10+
import * as fs from 'fs';
11+
import * as ts from 'typescript';
12+
13+
/**
14+
* Helper functions for computing dependencies.
15+
*/
16+
export class DependencyHost {
17+
/**
18+
* Get a list of the resolved paths to all the dependencies of this entry point.
19+
* @param from An absolute path to the file whose dependencies we want to get.
20+
* @param resolved A set that will have the resolved dependencies added to it.
21+
* @param missing A set that will have the dependencies that could not be found added to it.
22+
* @param internal A set that is used to track internal dependencies to prevent getting stuck in a
23+
* circular dependency loop.
24+
* @returns an object containing an array of absolute paths to `resolved` depenendencies and an
25+
* array of import specifiers for dependencies that were `missing`.
26+
*/
27+
computeDependencies(
28+
from: string, resolved: Set<string>, missing: Set<string>,
29+
internal: Set<string> = new Set()): void {
30+
const fromContents = fs.readFileSync(from, 'utf8');
31+
if (!this.hasImportOrReeportStatements(fromContents)) {
32+
return;
33+
}
34+
35+
// Parse the source into a TypeScript AST and then walk it looking for imports and re-exports.
36+
const sf =
37+
ts.createSourceFile(from, fromContents, ts.ScriptTarget.ES2015, false, ts.ScriptKind.JS);
38+
sf.statements
39+
// filter out statements that are not imports or reexports
40+
.filter(this.isStringImportOrReexport)
41+
// Grab the id of the module that is being imported
42+
.map(stmt => stmt.moduleSpecifier.text)
43+
// Resolve this module id into an absolute path
44+
.forEach(importPath => {
45+
if (importPath.startsWith('.')) {
46+
// This is an internal import so follow it
47+
const internalDependency = this.resolveInternal(from, importPath);
48+
// Avoid circular dependencies
49+
if (!internal.has(internalDependency)) {
50+
internal.add(internalDependency);
51+
this.computeDependencies(internalDependency, resolved, missing, internal);
52+
}
53+
} else {
54+
const externalDependency = this.tryResolveExternal(from, importPath);
55+
if (externalDependency !== null) {
56+
resolved.add(externalDependency);
57+
} else {
58+
missing.add(importPath);
59+
}
60+
}
61+
});
62+
}
63+
64+
/**
65+
* Resolve an internal module import.
66+
* @param from the absolute file path from where to start trying to resolve this module
67+
* @param to the module specifier of the internal dependency to resolve
68+
* @returns the resolved path to the import.
69+
*/
70+
resolveInternal(from: string, to: string): string {
71+
const fromDirectory = path.dirname(from);
72+
// `fromDirectory` is absolute so we don't need to worry about telling `require.resolve`
73+
// about it - unlike `tryResolve` below.
74+
return require.resolve(path.resolve(fromDirectory, to));
75+
}
76+
77+
/**
78+
* We don't want to resolve external dependencies directly because if it is a path to a
79+
* sub-entry-point (e.g. @angular/animations/browser rather than @angular/animations)
80+
* then `require.resolve()` may return a path to a UMD bundle, which may actually live
81+
* in the folder containing the sub-entry-point
82+
* (e.g. @angular/animations/bundles/animations-browser.umd.js).
83+
*
84+
* Instead we try to resolve it as a package, which is what we would need anyway for it to be
85+
* compilable by ngcc.
86+
*
87+
* If `to` is actually a path to a file then this will fail, which is what we want.
88+
*
89+
* @param from the file path from where to start trying to resolve this module
90+
* @param to the module specifier of the dependency to resolve
91+
* @returns the resolved path to the entry point directory of the import or null
92+
* if it cannot be resolved.
93+
*/
94+
tryResolveExternal(from: string, to: string): string|null {
95+
const externalDependency = this.tryResolve(from, `${to}/package.json`);
96+
return externalDependency && path.dirname(externalDependency);
97+
}
98+
99+
/**
100+
* Resolve the absolute path of a module from a particular starting point.
101+
*
102+
* @param from the file path from where to start trying to resolve this module
103+
* @param to the module specifier of the dependency to resolve
104+
* @returns an absolute path to the entry-point of the dependency or null if it could not be
105+
* resolved.
106+
*/
107+
tryResolve(from: string, to: string): string|null {
108+
try {
109+
return require.resolve(to, {paths: [from]});
110+
} catch (e) {
111+
return null;
112+
}
113+
}
114+
115+
/**
116+
* Check whether the given statement is an import with a string literal module specifier.
117+
* @param stmt the statement node to check.
118+
* @returns true if the statement is an import with a string literal module specifier.
119+
*/
120+
isStringImportOrReexport(stmt: ts.Statement): stmt is ts.ImportDeclaration&
121+
{moduleSpecifier: ts.StringLiteral} {
122+
return ts.isImportDeclaration(stmt) ||
123+
ts.isExportDeclaration(stmt) && !!stmt.moduleSpecifier &&
124+
ts.isStringLiteral(stmt.moduleSpecifier);
125+
}
126+
127+
/**
128+
* Check whether a source file needs to be parsed for imports.
129+
* This is a performance short-circuit, which saves us from creating
130+
* a TypeScript AST unnecessarily.
131+
*
132+
* @param source The content of the source file to check.
133+
*
134+
* @returns false if there are definitely no import or re-export statements
135+
* in this file, true otherwise.
136+
*/
137+
hasImportOrReeportStatements(source: string): boolean {
138+
return /(import|export)\s.+from/.test(source);
139+
}
140+
}

0 commit comments

Comments
 (0)
Please sign in to comment.