fix: add global path in require.resolve.paths by lvqq · Pull Request #13633 · jestjs/jest

lvqq · 2022-11-21T16:30:26Z

Summary

Close #13439, add global path supported in require.resolve.paths.

Test plan

Test cases have been added under packages/jest-resolve/src/__tests__/resolve.test.ts.
And with this changes, require.resolve.paths could react global paths under jest's scope.
Example in https://github.com/lvqq/jest-require-resolve-paths with the following changes works fine

facebook-github-bot · 2022-11-21T16:57:11Z

Hi @lvqq!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2022-11-21T17:08:40Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

jeysal

Hi, thanks for the contribution! I left some comments on things that struck me as potentially not ideal

jeysal · 2022-11-23T17:39:44Z

    });
-
-    module.paths = this._resolver.getModulePaths(module.path);
+    module.paths = [


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

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

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

jeysal · 2022-11-23T17:56:57Z

+    const globalPaths: Array<string> = [];
+
+    const paths = require.resolve.paths(moduleName);
+    if (paths) {


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?

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

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' ]

lvqq · 2022-11-24T01:04:23Z

Thanks for your reviewing! I will update it later

jeysal

thanks! few things to perhaps improve on the test other than that looks good.
not sure how worried I should be about any _execModule performance impact @SimenB (other than that it seems to only affect the require.resolve code path)

jeysal · 2022-11-28T14:46:52Z

+    expect(globalPaths).toStrictEqual([]);
+  });
+
+  it('return empty array with absolute path', () => {


Suggested change

it('return empty array with absolute path', () => {

it('return global paths with absolute path', () => {

jeysal · 2022-11-28T14:48:28Z

  });
 });
+
+describe('findGlobalPaths', () => {


can be removed I think, it's kind of internal and we're testing it through Resolver.getGlobalPaths

jeysal · 2022-11-28T14:50:35Z

+    jest.doMock('path', () => _path.posix);
+    const resolver = new Resolver(moduleMap, {} as ResolverConfig);
+    const globalPaths = resolver.getGlobalPaths('jest');
+    expect(globalPaths.length).toBeGreaterThan(0);


Probably we could test this more precisely with something like

Suggested change

expect(globalPaths.length).toBeGreaterThan(0);

globalPaths.forEach(globalPath => expect(require.resolve.paths('jest')).toContain(globalPath))

Thanks! I will update it

jeysal

lgtm, added a changelog entry and clarified logic a bit. waiting a bit to see if @SimenB wants to review

…aths * origin/main: fix typecheck:tests type err

jeysal · 2022-12-02T14:15:22Z

Pushed a commit to main fixing 2 of the 3 typecheck:tests issues that popped up on CI here. Oddly they did not pop up on CI on the TypeScript update commit, even though I suppose that's what must have caused them. cc @SimenB

I did not fix the third one in node_modules/ts-node/dist/ts-compiler-types.d.ts because that seems more complicated. I wanted to report an issue to ts-node or typescript but I can't reproduce it locally atm so I'm not sure what's happening.

Either way it's definitely not caused by this PR so don't need to block this on it.

github-actions · 2023-01-02T00:08:13Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

lvqq added 2 commits November 22, 2022 00:18

fix: add global path in require.resolve.paths

46da204

test: update snapshot

c96b300

test: update snapshot

6dc8310

facebook-github-bot added the cla signed label Nov 21, 2022

test: add builtin module test case

349dd7d

lvqq force-pushed the fix-require-resolve-paths branch from 2bf0efe to 349dd7d Compare November 23, 2022 03:37

jeysal requested changes Nov 23, 2022

View reviewed changes

lvqq added 3 commits November 24, 2022 22:13

refactor: grap global paths only once

4ae19b9

test: update cases and snapshot

b010ad2

chore: add path join and update test case

1c88b5c

lvqq requested a review from jeysal November 24, 2022 15:07

jeysal requested changes Nov 28, 2022

View reviewed changes

jeysal requested a review from SimenB November 28, 2022 14:56

test: update test cases

c026cd9

lvqq requested review from SimenB and jeysal and removed request for SimenB and jeysal November 28, 2022 15:16

clarify and add changelog

d27aa8b

jeysal approved these changes Nov 29, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix-require-resolve-p…

4a9642c

…aths * origin/main: fix typecheck:tests type err

jeysal merged commit 2ec6d24 into jestjs:main Dec 2, 2022

jeysal referenced this pull request Dec 7, 2022

fix typecheck:tests type err

ef91356

github-actions bot locked as resolved and limited conversation to collaborators Jan 2, 2023

	it('return empty array with absolute path', () => {
	it('return global paths with absolute path', () => {

	expect(globalPaths.length).toBeGreaterThan(0);
	globalPaths.forEach(globalPath => expect(require.resolve.paths('jest')).toContain(globalPath))

Uh oh!

Conversation

lvqq commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

facebook-github-bot commented Nov 21, 2022

Action Required

Process

Uh oh!

facebook-github-bot commented Nov 21, 2022

Uh oh!

jeysal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lvqq Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lvqq commented Nov 24, 2022

Uh oh!

jeysal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeysal left a comment

Choose a reason for hiding this comment

Uh oh!

jeysal commented Dec 2, 2022

Uh oh!

github-actions bot commented Jan 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lvqq commented Nov 21, 2022 •

edited

Loading

SimenB Nov 23, 2022 •

edited

Loading

lvqq Nov 24, 2022 •

edited

Loading