Skip to content

Commit

Permalink
fix(ngcc): correctly identify the package path of secondary entry-poi…
Browse files Browse the repository at this point in the history
…nts (#36249)

Previously we only searched for package paths below the set of `basePaths`
that were computed from the `basePath` provided to ngcc and the set of
`pathMappings`.

In some scenarios, such as hoisted packages, the entry-point is not within
any of the `basePaths` identified above. For example:

```
project
  packages
    app
      node_modules
        app-lib (depends on lib1)
  node_modules
    lib1 (depends on lib2)
      node_modules
        lib2 (depends on lib3/entry-point)
    lib3
      entry-point
```

When CLI is compiling `app-lib` ngcc will be given
`project/packages/app/node_modules` as the `basePath.

If ngcc is asked to target `lib2`, the `targetPath` will be
`project/node_modules/lib1/node_modules/lib2`.

Since `lib2` depends upon `lib3/entry-point`, ngcc will need to compute
the package path for `project/node_modules/lib3/entry-point`.

Since `project/node_modules/lib3/entry-point` is not contained in the `basePath`
`project/packages/app/node_modules`, ngcc failed to compute the `packagePath`
correctly, instead assuming that it was the same as the entry-point path.

Now we also consider the nearest `node_modules` folder to the entry-point
path as an additional `basePath`. If one is found then we use the first
directory directly below that `node_modules` directory as the package path.

In the case of our example this extra `basePath` would be `project/node_modules`
which allows us to compute the `packagePath` of `project/node_modules/lib3`.

Fixes #35747

PR Close #36249
  • Loading branch information
petebacondarwin authored and alxhub committed Mar 27, 2020
1 parent 1649743 commit 995cd15
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 7 deletions.
Expand Up @@ -150,13 +150,35 @@ export class TargetedEntryPointFinder implements EntryPointFinder {
break;
}
}
// If we get here then none of the `basePaths` matched the `entryPointPath`, which
// is somewhat unexpected and means that this entry-point lives completely outside
// any of the `basePaths`.
// All we can do is assume that his entry-point is a primary entry-point to a package.
return entryPointPath;
}

// We couldn't find a `packagePath` using `basePaths` so try to find the nearest `node_modules`
// that contains the `entryPointPath`, if there is one, and use it as a `basePath`.
let packagePath = entryPointPath;
let scopedPackagePath = packagePath;
let containerPath = this.fs.dirname(packagePath);
while (!this.fs.isRoot(containerPath) && !containerPath.endsWith('node_modules')) {
scopedPackagePath = packagePath;
packagePath = containerPath;
containerPath = this.fs.dirname(containerPath);
}

if (this.fs.exists(join(packagePath, 'package.json'))) {
// The directory directly below `node_modules` is a package - use it
return packagePath;
} else if (
this.fs.basename(packagePath).startsWith('@') &&
this.fs.exists(join(scopedPackagePath, 'package.json'))) {
// The directory directly below the `node_modules` is a scope and the directory directly
// below that is a scoped package - use it
return scopedPackagePath;
} else {
// If we get here then none of the `basePaths` contained the `entryPointPath` and the
// `entryPointPath` contains no `node_modules` that contains a package or a scoped
// package. All we can do is assume that this entry-point is a primary entry-point to a
// package.
return entryPointPath;
}
}

/**
* Split the given `path` into path segments using an FS independent algorithm.
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler-cli/ngcc/src/entry_point_finder/utils.ts
Expand Up @@ -30,7 +30,7 @@ import {PathMappings} from '../utils';
export function getBasePaths(
sourceDirectory: AbsoluteFsPath, pathMappings: PathMappings | undefined): AbsoluteFsPath[] {
const fs = getFileSystem();
let basePaths = [sourceDirectory];
const basePaths = [sourceDirectory];
if (pathMappings) {
const baseUrl = resolve(pathMappings.baseUrl);
Object.values(pathMappings.paths).forEach(paths => paths.forEach(path => {
Expand Down
Expand Up @@ -166,6 +166,81 @@ runInEachFileSystem(() => {
]);
});

it('should handle external node_modules folders (e.g. in a yarn workspace)', () => {
// Note that neither the basePath and targetPath contain each other
const basePath = _Abs('/nested_node_modules/packages/app/node_modules');
const targetPath = _Abs('/nested_node_modules/node_modules/package/entry-point');
loadTestFiles([
...createPackage(_Abs('/nested_node_modules/node_modules'), 'package'),
...createPackage(_Abs('/nested_node_modules/node_modules/package'), 'entry-point'),
]);
const finder = new TargetedEntryPointFinder(
fs, config, logger, resolver, basePath, targetPath, undefined);
const {entryPoints} = finder.findEntryPoints();
expect(dumpEntryPointPaths(_Abs('/nested_node_modules'), entryPoints)).toEqual([
['node_modules/package', 'node_modules/package/entry-point'],
]);
});

it('should handle external node_modules folders (e.g. in a yarn workspace) for dependencies',
() => {
// The application being compiled is at `/project/packages/app` so the basePath sent to
// ngcc is the `node_modules` below it
const basePath = _Abs('/project/packages/app/node_modules');
// `packages/app` depends upon lib1, which has a private dependency on lib2 in its
// own `node_modules` folder
const lib2 = createPackage(
_Abs('/project/node_modules/lib1/node_modules'), 'lib2', ['lib3/entry-point']);
// `lib2` depends upon `lib3/entry-point` which has been hoisted all the way up to the
// top level `node_modules`
const lib3 = createPackage(_Abs('/project/node_modules'), 'lib3');
const lib3EntryPoint = createPackage(_Abs('/project/node_modules/lib3'), 'entry-point');
loadTestFiles([...lib2, ...lib3, ...lib3EntryPoint]);
// The targetPath being processed is `lib2` and we expect it to find the correct
// entry-point info for the `lib3/entry-point` dependency.
const targetPath = _Abs('/project/node_modules/lib1/node_modules/lib2');
const finder = new TargetedEntryPointFinder(
fs, config, logger, resolver, basePath, targetPath, undefined);
const {entryPoints} = finder.findEntryPoints();
expect(dumpEntryPointPaths(_Abs('/project/node_modules'), entryPoints)).toEqual([
['lib3', 'lib3/entry-point'],
['lib1/node_modules/lib2', 'lib1/node_modules/lib2'],
]);
});

it('should handle external node_modules folders (e.g. in a yarn workspace) for scoped dependencies',
() => {
// The application being compiled is at `/project/packages/app` so the basePath sent to
// ngcc is the `node_modules` below it
const basePath = _Abs('/project/packages/app/node_modules');
// `packages/app` depends upon lib1, which has a private dependency on lib2 in its
// own `node_modules` folder
const lib2 = createPackage(
_Abs('/project/node_modules/lib1/node_modules'), 'lib2',
['@scope/lib3/entry-point']);
// `lib2` depends upon `lib3/entry-point` which has been hoisted all the way up to the
// top level `node_modules`
const lib3 = createPackage(_Abs('/project/node_modules/@scope'), 'lib3');
const lib3EntryPoint = createPackage(
_Abs('/project/node_modules/@scope/lib3'), 'entry-point', ['lib4/entry-point']);
const lib4 =
createPackage(_Abs('/project/node_modules/@scope/lib3/node_modules'), 'lib4');
const lib4EntryPoint = createPackage(
_Abs('/project/node_modules/@scope/lib3/node_modules/lib4'), 'entry-point');
loadTestFiles([...lib2, ...lib3, ...lib3EntryPoint, ...lib4, ...lib4EntryPoint]);
// The targetPath being processed is `lib2` and we expect it to find the correct
// entry-point info for the `lib3/entry-point` dependency.
const targetPath = _Abs('/project/node_modules/lib1/node_modules/lib2');
const finder = new TargetedEntryPointFinder(
fs, config, logger, resolver, basePath, targetPath, undefined);
const {entryPoints} = finder.findEntryPoints();
expect(dumpEntryPointPaths(_Abs('/project/node_modules'), entryPoints)).toEqual([
['@scope/lib3/node_modules/lib4', '@scope/lib3/node_modules/lib4/entry-point'],
['@scope/lib3', '@scope/lib3/entry-point'],
['lib1/node_modules/lib2', 'lib1/node_modules/lib2'],
]);
});

it('should handle dependencies via pathMappings', () => {
const basePath = _Abs('/path_mapped/node_modules');
const targetPath = _Abs('/path_mapped/node_modules/test');
Expand Down

0 comments on commit 995cd15

Please sign in to comment.