Skip to content

Commit

Permalink
fix(ngcc): compute the correct package paths for target entry-points (#…
Browse files Browse the repository at this point in the history
…40376)

Previously, if there were path-mapped entry-points, where one contaied the
string of another - for example `worker-client` and `worker` - then the
base paths were incorrectly computed resulting in the wrong package path
for the longer entry-point. This was because, when searching for a matching
base path, the strings were tested using `startsWith()`, whereas we should
only match if the path was contained in a directory from a file-system
point of view.

Now we not only check whether the target path "starts with" the base path
but then also whether the target path is actually contained in the base path
using `fs.relative()`.

Fixes #40352
Fixes #40357

PR Close #40376
  • Loading branch information
petebacondarwin authored and atscott committed Jan 11, 2021
1 parent 6c8cb5b commit 584b78a
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 2 deletions.
Expand Up @@ -119,7 +119,7 @@ export class TargetedEntryPointFinder extends TracingEntryPointFinder {
private computePackagePath(entryPointPath: AbsoluteFsPath): AbsoluteFsPath {
// First try the main basePath, to avoid having to compute the other basePaths from the paths
// mappings, which can be computationally intensive.
if (entryPointPath.startsWith(this.basePath)) {
if (this.isPathContainedBy(this.basePath, entryPointPath)) {
const packagePath = this.computePackagePathFromContainingPath(entryPointPath, this.basePath);
if (packagePath !== null) {
return packagePath;
Expand All @@ -129,7 +129,7 @@ export class TargetedEntryPointFinder extends TracingEntryPointFinder {
// The main `basePath` didn't work out so now we try the `basePaths` computed from the paths
// mappings in `tsconfig.json`.
for (const basePath of this.getBasePaths()) {
if (entryPointPath.startsWith(basePath)) {
if (this.isPathContainedBy(basePath, entryPointPath)) {
const packagePath = this.computePackagePathFromContainingPath(entryPointPath, basePath);
if (packagePath !== null) {
return packagePath;
Expand All @@ -147,6 +147,18 @@ export class TargetedEntryPointFinder extends TracingEntryPointFinder {
return this.computePackagePathFromNearestNodeModules(entryPointPath);
}

/**
* Compute whether the `test` path is contained within the `base` path.
*
* Note that this doesn't use a simple `startsWith()` since that would result in a false positive
* for `test` paths such as `a/b/c-x` when the `base` path is `a/b/c`.
*
* Since `fs.relative()` can be quite expensive we check the fast possibilities first.
*/
private isPathContainedBy(base: AbsoluteFsPath, test: AbsoluteFsPath): boolean {
return test === base ||
(test.startsWith(base) && !this.fs.relative(base, test).startsWith('..'));
}

/**
* Search down to the `entryPointPath` from the `containingPath` for the first `package.json` that
Expand Down
Expand Up @@ -355,6 +355,56 @@ runInEachFileSystem(() => {
]);
});

it('should correctly compute the package path for a target whose name contains the string of another package',
() => {
// Create the "my-lib" package - it doesn't need to be a real entry-point
const myLibPath = _Abs('/project/dist/my-lib');
loadTestFiles([{
name: fs.resolve(myLibPath, 'package.json'),
contents: JSON.stringify({name: 'my-lib'})
}]);

// Create the "my-lib-other" Angular entry-point
const myLibOtherPath = _Abs('/project/dist/my-lib-other');
loadTestFiles([
{
name: fs.resolve(myLibOtherPath, 'package.json'),
contents: JSON.stringify({
name: `my-lib-other`,
typings: `./my-lib-other.d.ts`,
fesm2015: `./fesm2015/my-lib-other.js`,
esm5: `./esm5/my-lib-other.js`,
main: `./common/my-lib-other.js`,
})
},
{name: fs.resolve(myLibOtherPath, 'my-lib-other.metadata.json'), contents: 'metadata'},
{name: fs.resolve(myLibOtherPath, 'my-lib-other.d.ts'), contents: 'typings'},
{name: fs.resolve(myLibOtherPath, 'fesm2015/my-lib-other.js'), contents: ''},
{name: fs.resolve(myLibOtherPath, 'esm5/my-lib-other.js'), contents: ''},
{name: fs.resolve(myLibOtherPath, 'commonjs/my-lib-other.js'), contents: ''},
]);

const basePath = _Abs('/project/node_modules');
const pathMappings: PathMappings = {
baseUrl: '/project',
paths: {
'lib1': ['dist/my-lib'],
'lib2': ['dist/my-lib-other'],
}
};

const srcHost = new EsmDependencyHost(fs, new ModuleResolver(fs, pathMappings));
const dtsHost = new DtsDependencyHost(fs, pathMappings);
resolver = new DependencyResolver(fs, logger, config, {esm2015: srcHost}, dtsHost);
const finder = new TargetedEntryPointFinder(
fs, config, logger, resolver, basePath, pathMappings, myLibOtherPath);
const {entryPoints} = finder.findEntryPoints();

expect(dumpEntryPointPaths(basePath, entryPoints)).toEqual([
['../dist/my-lib-other', '../dist/my-lib-other'],
]);
});

it('should handle pathMappings that map to files or non-existent directories', () => {
const basePath = _Abs('/path_mapped/node_modules');
const targetPath = _Abs('/path_mapped/node_modules/test');
Expand Down

0 comments on commit 584b78a

Please sign in to comment.