Skip to content

Commit

Permalink
Fix NODE_PATH resolving for relative paths (#3616)
Browse files Browse the repository at this point in the history
* Add failing integration test for relative NODE_PATH

It is failing because babel-jest traverses directories up searching for package.json, but the path is relative.

It is exposing the underlying issue that NODE_PATH can be relative, but the code assumes all paths are absolute by that point.

* Resolve NODE_PATH to absolute paths

This fixes the issue because the rest of the code assumes the path is already absolute.
We also resolve symlink if necessary.

* Fix unrelated test to match the code

This test started failing, but it was not testing NODE_PATH in the first place.

It just happened to include its output, and it's not empty on Travis.

The test used to have some copy pasta from how we calculate nodePaths, so I just synced that copy pasta.

* Nitty nit

* Exclude empty items from the array

They are empty if NODE_PATH is empty, and split() returns [''].

* fs.realpathSync() can return undefined

* Fix test to match last change
  • Loading branch information
gaearon authored and cpojer committed May 25, 2017
1 parent 3fe5866 commit 29dd4d4
Show file tree
Hide file tree
Showing 7 changed files with 75 additions and 3 deletions.
19 changes: 19 additions & 0 deletions integration_tests/__tests__/node_path-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @emails oncall+jsinfra
*/
'use strict';

const runJest = require('../runJest');

test('supports NODE_PATH', () => {
const result = runJest('node_path', [], {
nodePath: ['../node_path/src'],
});
expect(result.status).toBe(0);
});
12 changes: 12 additions & 0 deletions integration_tests/node_path/__tests__/node_path-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
'use strict';

test('can require by absolute path', () => {
expect(require('path/file.js')).toBe(42);
});
8 changes: 8 additions & 0 deletions integration_tests/node_path/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"jest": {
"testEnvironment": "node",
"transform": {
"^.+\\.jsx?$": "../../packages/babel-jest"
}
}
}
10 changes: 10 additions & 0 deletions integration_tests/node_path/src/path/file.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
'use strict';

module.exports = 42;
11 changes: 10 additions & 1 deletion integration_tests/runJest.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const {fileExists} = require('./utils');
const JEST_PATH = path.resolve(__dirname, '../packages/jest-cli/bin/jest.js');

type RunJestOptions = {
nodePath?: string,
skipPkgJsonCheck?: boolean, // don't complain if can't find package.json
};

Expand Down Expand Up @@ -45,7 +46,15 @@ function runJest(
);
}

const result = spawnSync(JEST_PATH, args || [], {cwd: dir});
const env = options.nodePath
? Object.assign({}, process.env, {
NODE_PATH: options.nodePath,
})
: process.env;
const result = spawnSync(JEST_PATH, args || [], {
cwd: dir,
env,
});

result.stdout = result.stdout && result.stdout.toString();
result.stderr = result.stderr && result.stderr.toString();
Expand Down
8 changes: 7 additions & 1 deletion packages/jest-resolve/src/__tests__/resolve-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

'use strict';

const fs = require('fs');
const path = require('path');
const ModuleMap = require('jest-haste-map').ModuleMap;
const Resolver = require('../');
Expand Down Expand Up @@ -41,8 +42,13 @@ describe('isCoreModule', () => {

describe('findNodeModule', () => {
it('is possible to override the default resolver', () => {
const cwd = process.cwd();
const resolvedCwd = fs.realpathSync(cwd) || cwd;
const nodePaths = process.env.NODE_PATH
? process.env.NODE_PATH.split(path.delimiter)
? process.env.NODE_PATH
.split(path.delimiter)
.filter(Boolean)
.map(p => path.resolve(resolvedCwd, p))
: null;

jest.mock('../__mocks__/userResolver');
Expand Down
10 changes: 9 additions & 1 deletion packages/jest-resolve/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import type {Path} from 'types/Config';
import type {ModuleMap} from 'types/HasteMap';

const fs = require('fs');
const path = require('path');
const nodeModulesPaths = require('resolve/lib/node-modules-paths');
const isBuiltinModule = require('is-builtin-module');
Expand Down Expand Up @@ -49,8 +50,15 @@ export type ResolveModuleConfig = {|

const NATIVE_PLATFORM = 'native';

// We might be inside a symlink.
const cwd = process.cwd();
const resolvedCwd = fs.realpathSync(cwd) || cwd;
const nodePaths = process.env.NODE_PATH
? process.env.NODE_PATH.split(path.delimiter)
? process.env.NODE_PATH
.split(path.delimiter)
.filter(Boolean)
// The resolver expects absolute paths.
.map(p => path.resolve(resolvedCwd, p))
: null;

class Resolver {
Expand Down

0 comments on commit 29dd4d4

Please sign in to comment.