Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ngcc): capture path-mapped entry-points that start with same string #35592

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 18 additions & 9 deletions packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts
Expand Up @@ -5,7 +5,7 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {AbsoluteFsPath, getFileSystem, join, resolve} from '../../../src/ngtsc/file_system';
import {AbsoluteFsPath, getFileSystem, join, relative, resolve} from '../../../src/ngtsc/file_system';
import {PathMappings} from '../utils';

/**
Expand Down Expand Up @@ -42,8 +42,8 @@ export function getBasePaths(
basePaths.push(basePath);
}));
}
basePaths.sort(); // Get the paths in order with the shorter ones first.
return basePaths.filter(removeDeeperPaths);
basePaths.sort().reverse(); // Get the paths in order with the longer ones first.
return basePaths.filter(removeContainedPaths);
}

/**
Expand All @@ -56,16 +56,25 @@ function extractPathPrefix(path: string) {
}

/**
* A filter function that removes paths that are already covered by higher paths.
* A filter function that removes paths that are contained by other paths.
*
* For example:
* Given `['a/b/c', 'a/b/x', 'a/b', 'd/e', 'd/f']` we will end up with `['a/b', 'd/e', 'd/f]`.
* (Note that we do not get `d` even though `d/e` and `d/f` share a base directory, since `d` is not
* one of the base paths.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

*
* @param value The current path.
* @param index The index of the current path.
* @param array The array of paths (sorted alphabetically).
* @returns true if this path is not already covered by a previous path.
* @param array The array of paths (sorted in reverse alphabetical order).
* @returns true if this path is not contained by another path.
*/
function removeDeeperPaths(value: AbsoluteFsPath, index: number, array: AbsoluteFsPath[]) {
for (let i = 0; i < index; i++) {
if (value.startsWith(array[i])) return false;
function removeContainedPaths(value: AbsoluteFsPath, index: number, array: AbsoluteFsPath[]) {
// We only need to check the following paths since the `array` is sorted in reverse alphabetic
// order.
for (let i = index + 1; i < array.length; i++) {
// We need to use `relative().startsWith()` rather than a simple `startsWith()` to ensure we
// don't assume that `a/b` contains `a/b-2`.
if (!relative(array[i], value).startsWith('..')) return false;
}
return true;
}
Expand Up @@ -189,8 +189,8 @@ runInEachFileSystem(() => {
fs, config, logger, resolver, basePath, pathMappings);
const {entryPoints} = finder.findEntryPoints();
expect(dumpEntryPointPaths(basePath, entryPoints)).toEqual([
['../dist/pkg2', '../dist/pkg2'],
['test', 'test'],
['../dist/pkg2', '../dist/pkg2'],
]);
});

Expand Down
Expand Up @@ -228,6 +228,39 @@ runInEachFileSystem(() => {
expect(entryPoint.package).toEqual(_Abs('/path_mapped/dist/primary'));
});

it('should correctly compute an entry-point whose path starts with the same string as another entry-point, via pathMappings',
() => {
const basePath = _Abs('/path_mapped/node_modules');
const targetPath = _Abs('/path_mapped/node_modules/test');
const pathMappings: PathMappings = {
baseUrl: '/path_mapped/dist',
paths: {
'lib1': ['my-lib/my-lib', 'my-lib'],
'lib2': ['my-lib/a', 'my-lib/a'],
'lib3': ['my-lib/b', 'my-lib/b'],
'lib4': ['my-lib-other/my-lib-other', 'my-lib-other']
}
};
loadTestFiles([
...createPackage(_Abs('/path_mapped/node_modules'), 'test', ['lib2', 'lib4']),
...createPackage(_Abs('/path_mapped/dist/my-lib'), 'my-lib'),
...createPackage(_Abs('/path_mapped/dist/my-lib'), 'a'),
...createPackage(_Abs('/path_mapped/dist/my-lib'), 'b'),
...createPackage(_Abs('/path_mapped/dist/my-lib-other'), 'my-lib-other'),
]);
const srcHost = new EsmDependencyHost(fs, new ModuleResolver(fs, pathMappings));
const dtsHost = new DtsDependencyHost(fs, pathMappings);
resolver = new DependencyResolver(fs, logger, {esm2015: srcHost}, dtsHost);
const finder = new TargetedEntryPointFinder(
fs, config, logger, resolver, basePath, targetPath, pathMappings);
const {entryPoints} = finder.findEntryPoints();
expect(dumpEntryPointPaths(basePath, entryPoints)).toEqual([
['../dist/my-lib/a', '../dist/my-lib/a'],
['../dist/my-lib-other/my-lib-other', '../dist/my-lib-other/my-lib-other'],
['test', 'test'],
]);
});

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