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.

Sign up for GitHub

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: add global path in require.resolve.paths #13633

Merged
merged 10 commits into from
Dec 2, 2022
Merged

fix: add global path in require.resolve.paths #13633

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
46da204
fix: add global path in require.resolve.paths
lvqq Nov 21, 2022
c96b300
test: update snapshot
lvqq Nov 21, 2022
6dc8310
test: update snapshot
lvqq Nov 21, 2022
349dd7d
test: add builtin module test case
lvqq Nov 23, 2022
4ae19b9
refactor: grap global paths only once
lvqq Nov 24, 2022
b010ad2
test: update cases and snapshot
lvqq Nov 24, 2022
1c88b5c
chore: add path join and update test case
lvqq Nov 24, 2022
c026cd9
test: update test cases
lvqq Nov 28, 2022
d27aa8b
clarify and add changelog
jeysal Nov 29, 2022
4a9642c
Merge remote-tracking branch 'origin/main' into fix-require-resolve-p…
jeysal Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions e2e/__tests__/__snapshots__/moduleNameMapper.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ exports[`moduleNameMapper wrong array configuration 1`] = `
12 | module.exports = () => 'test';
13 |

at createNoMappedModuleFoundError (../../packages/jest-resolve/build/resolver.js:752:17)
at createNoMappedModuleFoundError (../../packages/jest-resolve/build/resolver.js:773:17)
at Object.require (index.js:10:1)
at Object.require (__tests__/index.js:10:20)"
`;
Expand Down Expand Up @@ -71,7 +71,7 @@ exports[`moduleNameMapper wrong configuration 1`] = `
12 | module.exports = () => 'test';
13 |

at createNoMappedModuleFoundError (../../packages/jest-resolve/build/resolver.js:752:17)
at createNoMappedModuleFoundError (../../packages/jest-resolve/build/resolver.js:773:17)
at Object.require (index.js:10:1)
at Object.require (__tests__/index.js:10:20)"
`;
2 changes: 1 addition & 1 deletion e2e/__tests__/__snapshots__/requireMissingExt.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ exports[`shows a proper error from deep requires 1`] = `
12 | test('dummy', () => {
13 | expect(1).toBe(1);

at Resolver._throwModNotFoundError (../../packages/jest-resolve/build/resolver.js:425:11)
at Resolver._throwModNotFoundError (../../packages/jest-resolve/build/resolver.js:427:11)
at Object.<anonymous> (node_modules/discord.js/src/index.js:21:12)
at Object.require (__tests__/test.js:10:1)"
`;
2 changes: 1 addition & 1 deletion e2e/__tests__/__snapshots__/resolveNoFileExtensions.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ exports[`show error message with matching files 1`] = `
| ^
9 |

at Resolver._throwModNotFoundError (../../packages/jest-resolve/build/resolver.js:425:11)
at Resolver._throwModNotFoundError (../../packages/jest-resolve/build/resolver.js:427:11)
at Object.require (index.js:8:18)
at Object.require (__tests__/test.js:8:11)"
`;
40 changes: 40 additions & 0 deletions packages/jest-resolve/src/__tests__/resolve.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,3 +707,43 @@ describe('Resolver.getModulePaths() -> nodeModulesPaths()', () => {
expect(dirs_actual).toEqual(expect.arrayContaining(dirs_expected));
});
});

describe('Resolver.getGlobalPaths()', () => {
const _path = path;
let moduleMap: IModuleMap;
beforeEach(() => {
moduleMap = ModuleMap.create('/');
});

it('return global paths with npm package', () => {
jest.doMock('path', () => _path.posix);
const resolver = new Resolver(moduleMap, {} as ResolverConfig);
const cwd = '/temp/project';
const globalPaths = resolver.getGlobalPaths(cwd, 'jest');
expect(globalPaths.length).toBeGreaterThanOrEqual(1);
});

it('return empty array with builtin module', () => {
jest.doMock('path', () => _path.posix);
const resolver = new Resolver(moduleMap, {} as ResolverConfig);
const cwd = '/temp/project';
const globalPaths = resolver.getGlobalPaths(cwd, 'fs');
expect(globalPaths).toStrictEqual([]);
});

it('return empty array with relative path', () => {
jest.doMock('path', () => _path.posix);
const resolver = new Resolver(moduleMap, {} as ResolverConfig);
const cwd = '/temp/project';
const globalPaths = resolver.getGlobalPaths(cwd, './module');
expect(globalPaths).toStrictEqual([]);
});

it('return empty array without module name', () => {
jest.doMock('path', () => _path.posix);
const resolver = new Resolver(moduleMap, {} as ResolverConfig);
const cwd = '/temp/project';
const globalPaths = resolver.getGlobalPaths(cwd);
expect(globalPaths).toStrictEqual([]);
});
});
26 changes: 26 additions & 0 deletions packages/jest-resolve/src/resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export default class Resolver {
private readonly _moduleIDCache: Map<string, string>;
private readonly _moduleNameCache: Map<string, string>;
private readonly _modulePathCache: Map<string, Array<string>>;
private readonly _globalPathCache: Map<string, Array<string>>;
private readonly _supportsNativePlatform: boolean;

constructor(moduleMap: IModuleMap, options: ResolverConfig) {
Expand All @@ -81,6 +82,7 @@ export default class Resolver {
this._moduleIDCache = new Map();
this._moduleNameCache = new Map();
this._modulePathCache = new Map();
this._globalPathCache = new Map();
}

static ModuleNotFoundError = ModuleNotFoundError;
Expand Down Expand Up @@ -526,6 +528,30 @@ export default class Resolver {
return paths;
}

getGlobalPaths(from: string, moduleName?: string): Array<string> {
if (!moduleName || moduleName[0] === '.') return [];

const cachedGlobalPath = this._globalPathCache.get(moduleName);
if (cachedGlobalPath) {
return cachedGlobalPath;
}

const modulePaths = this.getModulePaths(from);
const globalPaths: Array<string> = [];

const paths = require.resolve.paths(moduleName);
if (paths) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be missing something, but do we need to spend CPU time to grab the global paths every time? I'm thinking the solution could be as simple as grabbing those global dirs once, probably just straight away in nodeModulesPaths.ts:

const GLOBAL_PATHS = findGlobalPaths();
function findGlobalPaths() {
  const paths=require.resolve.paths('/')
  // findIndex, slice, etc.
}

and append them to the paths list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor Author

@lvqq lvqq Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with grabbing only once, but it cannot be directly in funtion nodeModulesPaths since module paths will be cached. For instance, if the path without module name is cached. then it won't update. I'd add a new function to export

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand the caching problem. We're talking about the "global paths" that come after /node_modules. Aren't they independent of the moduleName, something like

[
  '/home/user/.node_modules',
  '/home/user/.node_libraries'
]

// find the rootIndex and remain the global paths
const rootIndex = paths.findIndex(
path => path === modulePaths[modulePaths.length - 1],
);
globalPaths.push(...(rootIndex > -1 ? paths.slice(rootIndex + 1) : []));
}

this._globalPathCache.set(moduleName, globalPaths);
return globalPaths;
}

getModuleID(
virtualMocks: Map<string, boolean>,
from: string,
Expand Down
22 changes: 17 additions & 5 deletions packages/jest-runtime/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1066,7 +1066,13 @@ export default class Runtime {
} else {
// Only include the fromPath if a moduleName is given. Else treat as root.
const fromPath = moduleName ? from : null;
this._execModule(localModule, options, moduleRegistry, fromPath);
this._execModule(
localModule,
options,
moduleRegistry,
fromPath,
moduleName,
);
}
localModule.loaded = true;
}
Expand Down Expand Up @@ -1398,6 +1404,7 @@ export default class Runtime {
}

private _requireResolvePaths(from: string, moduleName?: string) {
const fromDir = path.resolve(from, '..');
if (moduleName == null) {
throw new Error(
'The first argument to require.resolve.paths must be a string. Received null or undefined.',
Expand All @@ -1410,19 +1417,22 @@ export default class Runtime {
}

if (moduleName[0] === '.') {
return [path.resolve(from, '..')];
return [fromDir];
}
if (this._resolver.isCoreModule(moduleName)) {
return null;
}
return this._resolver.getModulePaths(path.resolve(from, '..'));
const modulePaths = this._resolver.getModulePaths(fromDir);
const globalPaths = this._resolver.getGlobalPaths(fromDir, moduleName);
return [...modulePaths, ...globalPaths];
}

private _execModule(
localModule: InitialModule,
options: InternalModuleOptions | undefined,
moduleRegistry: ModuleRegistry,
from: string | null,
moduleName?: string,
) {
if (this.isTornDown) {
this._logFormattedReferenceError(
Expand Down Expand Up @@ -1454,8 +1464,10 @@ export default class Runtime {
return moduleRegistry.get(key) || null;
},
});

module.paths = this._resolver.getModulePaths(module.path);
module.paths = [
Copy link
Contributor

Choose a reason for hiding this comment

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

While I haven't profiled this and can't know the perf impact for sure, this is a code path that runs very often, so I'd rather be safe and do something like

    module.paths = this._resolver.getModulePaths(module.path);
    if(moduleName) module.paths=[...module.paths, ...this._resolver.getGlobalPaths(etc)]

But read the other comment first, perhaps this code might change anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, cc @SimenB who knows more about the ESM impl cause I don't know all implications changing module.paths might have

Copy link
Member

@SimenB SimenB Nov 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

module.* is unused in ESM, so any change to it shouldn't affect ESM stuff

...this._resolver.getModulePaths(module.path),
...this._resolver.getGlobalPaths(module.path, moduleName),
];
Object.defineProperty(module, 'require', {
value: this._createRequireImplementation(module, options),
});
Expand Down