From 71b5970450fcbf72ee5b09be7ea9f6318167e727 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Thu, 20 Feb 2020 21:03:14 +0100 Subject: [PATCH] fix(ngcc): capture path-mapped entry-points that start with same string (#35592) Previously if there were two path-mapped libraries that are in different directories but the path of one started with same string as the path of the other, we would incorrectly return the shorter path - e.g. `dist/my-lib` and `dist/my-lib-second`. This was because the list of `basePaths` was searched in ascending alphabetic order and we were using `startsWith()` to match the path. Now the `basePaths` are searched in reverse alphabetic order so the longer path will be matched correctly. // FW-1873 Fixes #35536 PR Close #35592 --- .../ngcc/src/entry_point_finder/utils.ts | 27 ++++++++++----- ...irectory_walker_entry_point_finder_spec.ts | 2 +- .../targeted_entry_point_finder_spec.ts | 33 +++++++++++++++++++ 3 files changed, 52 insertions(+), 10 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts b/packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts index a264166d01..7a43c237a7 100644 --- a/packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts +++ b/packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts @@ -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'; /** @@ -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); } /** @@ -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.) * * @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; } diff --git a/packages/compiler-cli/ngcc/test/entry_point_finder/directory_walker_entry_point_finder_spec.ts b/packages/compiler-cli/ngcc/test/entry_point_finder/directory_walker_entry_point_finder_spec.ts index 11fc602663..e8c36e8888 100644 --- a/packages/compiler-cli/ngcc/test/entry_point_finder/directory_walker_entry_point_finder_spec.ts +++ b/packages/compiler-cli/ngcc/test/entry_point_finder/directory_walker_entry_point_finder_spec.ts @@ -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'], ]); }); diff --git a/packages/compiler-cli/ngcc/test/entry_point_finder/targeted_entry_point_finder_spec.ts b/packages/compiler-cli/ngcc/test/entry_point_finder/targeted_entry_point_finder_spec.ts index c7e2cc64dd..454458cf30 100644 --- a/packages/compiler-cli/ngcc/test/entry_point_finder/targeted_entry_point_finder_spec.ts +++ b/packages/compiler-cli/ngcc/test/entry_point_finder/targeted_entry_point_finder_spec.ts @@ -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');