Skip to content

Commit

Permalink
fix(core): hangs when used with yarn PnP
Browse files Browse the repository at this point in the history
The `findNpmPackage` function in `runtime-info.ts` has a subtle and
mostly innocuous bug.  Its parent, `collectRuntimeInformation`, iterates
over the modules in `require.cache` and resolves the library version for
each.

It uses the `paths` property of each entry to locate the package
description for that module.  An example:

For `/foo/bar/aws-cdk/packages/aws-cdk/lib/index.js`:

```js
[ '/foo/aws-cdk/packages/aws-cdk/lib/node_modules',
'/foo/aws-cdk/packages/aws-cdk/node_modules',
'/foo/aws-cdk/packages/node_modules',
'/foo/aws-cdk/node_modules',
'/foo/node_modules',
'/node_modules' ]
```

`findNpmPackage` maps over these, and if the string ends with
`/node_modules` or `\\node_modules` it strips that off, ending with:

```js
[ '/foo/aws-cdk/packages/aws-cdk/lib',
'/foo/aws-cdk/packages/aws-cdk',
'/foo/aws-cdk/packages',
'/foo/aws-cdk',
'/foo',
'' ]
```

...and passes this array as the `paths` option to `require.resolve`.

The problem is that `''` is interpreted as the current working
directory, and instead of walking the file tree to the root (as was
probably intended) it appends whatever node's current working directory
is to the search.

It's hard to imagine this ever really having a negative impact on
package resolution, but unfortunately this triggered a likewise obscure
bug in yarn 2 when configured with PnP.  (See yarnpkg/berry#1298).  This
bug is fixed in yarn 2 master but hasn't been released yet.  The effect
of these two bugs when triggered is for the CDK CLI to hang.

This change seems to meet the intent of the code without having the
above problem.  It seems canonically correct and uses the `fs` lib more
idiomatically.  It's also less code :)

This passes all existing tests.  It's not clear how this would be tested
without exporting `findNpmPackage` and adding tests for it.

**CAVEAT:** I can't test this on windows, but it's worth noting the
following:

```js
// Node 12.16.3
path.win32.resolve('C:')
// => 'C:\\foo\\aws-cdk' (current working directory)
```

```js
// Node 10.13.0
path.win32.resolve('C:')
// => 'C:\\'
```

Both of these results come on OSX.  But it appears that the yarn bug
would not trigger on either version, while node may or may not have the
same wrong behavior for `findNpmVersion` on windows.
  • Loading branch information
keithlayne committed May 13, 2020
1 parent ed206a5 commit 8579100
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 14 deletions.
17 changes: 3 additions & 14 deletions packages/@aws-cdk/core/lib/private/runtime-info.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { basename, dirname } from 'path';
import { major as nodeMajorVersion } from './node-version';

// list of NPM scopes included in version reporting e.g. @aws-cdk and @aws-solutions-konstruk
Expand Down Expand Up @@ -72,7 +73,8 @@ function findNpmPackage(fileName: string): { name: string, version: string, priv
return undefined;
}

const paths = mod.paths.map(stripNodeModules);
// For any path in ``mod.paths`` that is a node_modules folder, use its parent directory instead.
const paths = mod.paths.map((path: string) => basename(path) === 'node_modules' ? dirname(path) : path);

try {
const packagePath = require.resolve(
Expand All @@ -85,19 +87,6 @@ function findNpmPackage(fileName: string): { name: string, version: string, priv
} catch (e) {
return undefined;
}

/**
* @param s a path.
* @returns ``s`` with any terminating ``/node_modules``
* (or ``\\node_modules``) stripped off.)
*/
function stripNodeModules(s: string): string {
if (s.endsWith('/node_modules') || s.endsWith('\\node_modules')) {
// /node_modules is 13 characters
return s.substr(0, s.length - 13);
}
return s;
}
}

function getJsiiAgentVersion() {
Expand Down
26 changes: 26 additions & 0 deletions packages/@aws-cdk/core/test/test.runtime-info.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,30 @@ export = {
});
test.done();
},

'version reporting finds no version with no associated package.json'(test: Test) {
const pkgdir = fs.mkdtempSync(path.join(os.tmpdir(), 'runtime-info-find-npm-package-fixture'));
const mockVersion = '1.2.3';

fs.writeFileSync(path.join(pkgdir, 'index.js'), 'module.exports = \'this is bar\';');
fs.mkdirSync(path.join(pkgdir, 'bar'));
fs.writeFileSync(path.join(pkgdir, 'bar', 'package.json'), JSON.stringify({
name: '@aws-solutions-konstruk/bar',
version: mockVersion,
}));

// eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies
require(pkgdir);

const cwd = process.cwd();

// Switch to `bar` where the package.json is, then resolve version. Fails when module.resolve
// is passed an empty string in the paths array.
process.chdir(path.join(pkgdir, 'bar'));
const runtimeInfo = collectRuntimeInformation();
process.chdir(cwd);

test.equal(runtimeInfo.libraries['@aws-solutions-konstruk/bar'], undefined);
test.done();
},
};

0 comments on commit 8579100

Please sign in to comment.