Skip to content

Commit 7bb3588

Browse files
petebacondarwinalxhub
authored andcommitted
fix(ngcc): handle bad path mappings when finding entry-points (#36331)
Previously, a bad baseUrl or path mapping passed to an `EntryPointFinder` could cause the original `sourceDirectory` to be superceded by a higher directory. This could result in none of the sourceDirectory entry-points being processed. Now missing basePaths computed from path-mappings are discarded with a warning. Further, if the `baseUrl` is the root directory then a warning is given as this is most likely an error in the tsconfig.json. Resolves #36313 Resolves #36283 PR Close #36331
1 parent 03a3c75 commit 7bb3588

File tree

5 files changed

+178
-9
lines changed

5 files changed

+178
-9
lines changed

packages/compiler-cli/ngcc/src/entry_point_finder/directory_walker_entry_point_finder.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import {getBasePaths, trackDuration} from './utils';
2121
* `pathMappings`.
2222
*/
2323
export class DirectoryWalkerEntryPointFinder implements EntryPointFinder {
24-
private basePaths = getBasePaths(this.sourceDirectory, this.pathMappings);
24+
private basePaths = getBasePaths(this.logger, this.sourceDirectory, this.pathMappings);
2525
constructor(
2626
private fs: FileSystem, private config: NgccConfiguration, private logger: Logger,
2727
private resolver: DependencyResolver, private entryPointManifest: EntryPointManifest,

packages/compiler-cli/ngcc/src/entry_point_finder/targeted_entry_point_finder.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import {getBasePaths} from './utils';
2525
export class TargetedEntryPointFinder implements EntryPointFinder {
2626
private unprocessedPaths: AbsoluteFsPath[] = [];
2727
private unsortedEntryPoints = new Map<AbsoluteFsPath, EntryPoint>();
28-
private basePaths = getBasePaths(this.basePath, this.pathMappings);
28+
private basePaths = getBasePaths(this.logger, this.basePath, this.pathMappings);
2929

3030
constructor(
3131
private fs: FileSystem, private config: NgccConfiguration, private logger: Logger,

packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55
* Use of this source code is governed by an MIT-style license that can be
66
* found in the LICENSE file at https://angular.io/license
77
*/
8-
import {AbsoluteFsPath, getFileSystem, join, relative, resolve} from '../../../src/ngtsc/file_system';
8+
import {AbsoluteFsPath, getFileSystem, relative, resolve} from '../../../src/ngtsc/file_system';
9+
import {Logger} from '../logging/logger';
910
import {PathMappings} from '../utils';
1011

1112
/**
@@ -28,22 +29,44 @@ import {PathMappings} from '../utils';
2829
* @param pathMappings Path mapping configuration, from which to extract additional base-paths.
2930
*/
3031
export function getBasePaths(
31-
sourceDirectory: AbsoluteFsPath, pathMappings: PathMappings | undefined): AbsoluteFsPath[] {
32+
logger: Logger, sourceDirectory: AbsoluteFsPath,
33+
pathMappings: PathMappings | undefined): AbsoluteFsPath[] {
3234
const fs = getFileSystem();
3335
const basePaths = [sourceDirectory];
3436
if (pathMappings) {
3537
const baseUrl = resolve(pathMappings.baseUrl);
38+
if (fs.isRoot(baseUrl)) {
39+
logger.warn(
40+
`The provided pathMappings baseUrl is the root path ${baseUrl}.\n` +
41+
`This is likely to mess up how ngcc finds entry-points and is probably not correct.\n` +
42+
`Please check your path mappings configuration such as in the tsconfig.json file.`);
43+
}
3644
Object.values(pathMappings.paths).forEach(paths => paths.forEach(path => {
3745
// We only want base paths that exist and are not files
38-
let basePath = join(baseUrl, extractPathPrefix(path));
39-
while (basePath !== baseUrl && (!fs.exists(basePath) || fs.stat(basePath).isFile())) {
46+
let basePath = fs.resolve(baseUrl, extractPathPrefix(path));
47+
if (fs.exists(basePath) && fs.stat(basePath).isFile()) {
4048
basePath = fs.dirname(basePath);
4149
}
42-
basePaths.push(basePath);
50+
if (fs.exists(basePath)) {
51+
basePaths.push(basePath);
52+
} else {
53+
logger.warn(
54+
`The basePath "${basePath}" computed from baseUrl "${baseUrl}" and path mapping "${path}" does not exist in the file-system.\n` +
55+
`It will not be scanned for entry-points.`);
56+
}
4357
}));
4458
}
4559
basePaths.sort().reverse(); // Get the paths in order with the longer ones first.
46-
return basePaths.filter(removeContainedPaths);
60+
const dedupedBasePaths = basePaths.filter(removeContainedPaths);
61+
62+
// We want to ensure that the `sourceDirectory` is included when it is a node_modules folder.
63+
// Otherwise our entry-point finding algorithm would fail to walk that folder.
64+
if (fs.basename(sourceDirectory) === 'node_modules' &&
65+
!dedupedBasePaths.includes(sourceDirectory)) {
66+
dedupedBasePaths.unshift(sourceDirectory);
67+
}
68+
69+
return dedupedBasePaths;
4770
}
4871

4972
/**

packages/compiler-cli/ngcc/test/entry_point_finder/directory_walker_entry_point_finder_spec.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ runInEachFileSystem(() => {
340340
const pathMappings: PathMappings = {
341341
baseUrl: '/path_mapped/dist',
342342
paths: {
343-
'@test': ['pkg2/fesm2015/pkg2.js'],
343+
'@test': ['pkg2/pkg2.d.ts'],
344344
'@missing': ['pkg3'],
345345
}
346346
};
@@ -371,6 +371,10 @@ runInEachFileSystem(() => {
371371
fesm2015: `./fesm2015/${packageName}.js`,
372372
})
373373
},
374+
{
375+
name: _Abs(`${basePath}/${packageName}/${packageName}.d.ts`),
376+
contents: deps.map((dep, i) => `import * as i${i} from '${dep}';`).join('\n'),
377+
},
374378
{
375379
name: _Abs(`${basePath}/${packageName}/fesm2015/${packageName}.js`),
376380
contents: deps.map((dep, i) => `import * as i${i} from '${dep}';`).join('\n'),
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
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+
import {absoluteFrom, getFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system';
9+
10+
import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing';
11+
import {getBasePaths} from '../../src/entry_point_finder/utils';
12+
import {MockLogger} from '../helpers/mock_logger';
13+
14+
runInEachFileSystem(() => {
15+
let _: typeof absoluteFrom;
16+
let logger: MockLogger;
17+
18+
beforeEach(() => {
19+
_ = absoluteFrom;
20+
logger = new MockLogger();
21+
});
22+
23+
describe('getBasePaths', () => {
24+
it('should just return the `sourceDirectory if there are no `pathMappings', () => {
25+
const sourceDirectory = _('/path/to/project/node_modules');
26+
const basePaths = getBasePaths(logger, sourceDirectory, undefined);
27+
expect(basePaths).toEqual([sourceDirectory]);
28+
});
29+
30+
it('should use each path mapping prefix and sort in descending order', () => {
31+
const projectDirectory = _('/path/to/project');
32+
const fs = getFileSystem();
33+
fs.ensureDir(fs.resolve(projectDirectory, 'dist-1'));
34+
fs.ensureDir(fs.resolve(projectDirectory, 'sub-folder/dist-2'));
35+
fs.ensureDir(fs.resolve(projectDirectory, 'libs'));
36+
37+
const sourceDirectory = _('/path/to/project/node_modules');
38+
const pathMappings = {
39+
baseUrl: projectDirectory,
40+
paths: {'@dist': ['dist-1', 'sub-folder/dist-2'], '@lib/*': ['libs/*']}
41+
};
42+
const basePaths = getBasePaths(logger, sourceDirectory, pathMappings);
43+
expect(basePaths).toEqual([
44+
fs.resolve(projectDirectory, 'sub-folder/dist-2'),
45+
sourceDirectory,
46+
fs.resolve(projectDirectory, 'libs'),
47+
fs.resolve(projectDirectory, 'dist-1'),
48+
]);
49+
});
50+
51+
it('should discard paths that are already contained by another path', () => {
52+
const projectDirectory = _('/path/to/project');
53+
const fs = getFileSystem();
54+
fs.ensureDir(fs.resolve(projectDirectory, 'dist-1'));
55+
fs.ensureDir(fs.resolve(projectDirectory, 'dist-1/sub-folder'));
56+
fs.ensureDir(fs.resolve(projectDirectory, 'node_modules/libs'));
57+
58+
const sourceDirectory = _('/path/to/project/node_modules');
59+
const pathMappings = {
60+
baseUrl: projectDirectory,
61+
paths: {'@dist': ['dist-1', 'dist-1/sub-folder'], '@lib/*': ['node_modules/libs/*']}
62+
};
63+
const basePaths = getBasePaths(logger, sourceDirectory, pathMappings);
64+
expect(basePaths).toEqual([
65+
sourceDirectory,
66+
fs.resolve(projectDirectory, 'dist-1'),
67+
]);
68+
});
69+
70+
it('should use the containing directory of path mapped files', () => {
71+
const projectDirectory = _('/path/to/project');
72+
const fs = getFileSystem();
73+
fs.ensureDir(fs.resolve(projectDirectory, 'dist-1'));
74+
fs.writeFile(fs.resolve(projectDirectory, 'dist-1/file.js'), 'dummy content');
75+
76+
const sourceDirectory = _('/path/to/project/node_modules');
77+
const pathMappings = {baseUrl: projectDirectory, paths: {'@dist': ['dist-1/file.js']}};
78+
const basePaths = getBasePaths(logger, sourceDirectory, pathMappings);
79+
expect(basePaths).toEqual([
80+
sourceDirectory,
81+
fs.resolve(projectDirectory, 'dist-1'),
82+
]);
83+
});
84+
85+
it('should always include the `sourceDirectory` if it is a node_modules directory in the returned basePaths, even if it is contained by another basePath',
86+
() => {
87+
const projectDirectory = _('/path/to/project');
88+
const sourceDirectory = _('/path/to/project/node_modules');
89+
const fs = getFileSystem();
90+
fs.ensureDir(fs.resolve(sourceDirectory));
91+
92+
const pathMappings = {baseUrl: projectDirectory, paths: {'*': ['./*']}};
93+
const basePaths = getBasePaths(logger, sourceDirectory, pathMappings);
94+
expect(basePaths).toEqual([
95+
sourceDirectory,
96+
projectDirectory,
97+
]);
98+
});
99+
100+
it('should log a warning if baseUrl is the root path', () => {
101+
const fs = getFileSystem();
102+
fs.ensureDir(fs.resolve('/dist'));
103+
104+
const sourceDirectory = _('/path/to/project/node_modules');
105+
const pathMappings = {baseUrl: _('/'), paths: {'@dist': ['dist']}};
106+
const basePaths = getBasePaths(logger, sourceDirectory, pathMappings);
107+
expect(basePaths).toEqual([
108+
sourceDirectory,
109+
fs.resolve('/dist'),
110+
]);
111+
expect(logger.logs.warn).toEqual([
112+
[`The provided pathMappings baseUrl is the root path ${_('/')}.\n` +
113+
`This is likely to mess up how ngcc finds entry-points and is probably not correct.\n` +
114+
`Please check your path mappings configuration such as in the tsconfig.json file.`]
115+
]);
116+
});
117+
118+
it('should discard basePaths that do not exists and log a warning', () => {
119+
const projectDirectory = _('/path/to/project');
120+
const fs = getFileSystem();
121+
fs.ensureDir(fs.resolve(projectDirectory, 'dist-1'));
122+
fs.ensureDir(fs.resolve(projectDirectory, 'sub-folder'));
123+
124+
const sourceDirectory = _('/path/to/project/node_modules');
125+
const pathMappings = {
126+
baseUrl: projectDirectory,
127+
paths: {'@dist': ['dist-1', 'sub-folder/dist-2'], '@lib/*': ['libs/*']}
128+
};
129+
const basePaths = getBasePaths(logger, sourceDirectory, pathMappings);
130+
expect(basePaths).toEqual([
131+
sourceDirectory,
132+
fs.resolve(projectDirectory, 'dist-1'),
133+
]);
134+
expect(logger.logs.warn).toEqual([
135+
[`The basePath "${fs.resolve(projectDirectory, 'sub-folder/dist-2')}" computed from baseUrl "${projectDirectory}" and path mapping "sub-folder/dist-2" does not exist in the file-system.\n` +
136+
`It will not be scanned for entry-points.`],
137+
[`The basePath "${fs.resolve(projectDirectory, 'libs')}" computed from baseUrl "${projectDirectory}" and path mapping "libs/*" does not exist in the file-system.\n` +
138+
`It will not be scanned for entry-points.`],
139+
]);
140+
});
141+
});
142+
});

0 commit comments

Comments
 (0)