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.

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

Conversation

lvqq
Copy link
Contributor

@lvqq lvqq commented Nov 21, 2022

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
Copy link
Contributor

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 cla@meta.com. Thanks!

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

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

@@ -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

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

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

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

@lvqq
Copy link
Contributor Author

lvqq commented Nov 24, 2022

Thanks for your reviewing! I will update it later

@lvqq lvqq requested a review from jeysal November 24, 2022 15:07
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

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)

expect(globalPaths).toStrictEqual([]);
});

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

Choose a reason for hiding this comment

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

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

@@ -707,3 +707,51 @@ describe('Resolver.getModulePaths() -> nodeModulesPaths()', () => {
expect(dirs_actual).toEqual(expect.arrayContaining(dirs_expected));
});
});

describe('findGlobalPaths', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I will update it

@jeysal jeysal requested a review from SimenB November 28, 2022 14:56
@lvqq lvqq requested review from jeysal and SimenB and removed request for SimenB and jeysal November 28, 2022 15:16
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

jeysal commented Dec 2, 2022

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.

@jeysal jeysal merged commit 2ec6d24 into jestjs:main Dec 2, 2022
@github-actions
Copy link

github-actions bot commented Jan 2, 2023

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: require.resolve.paths cannot reach global path in the scope of jest
4 participants