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

require.resolve('./', [options]) with paths option will resolve with root index.js file while it should error with MODULE_NOT_FOUND #23643

Closed
niksajanjic opened this issue Oct 13, 2018 · 3 comments
Labels
module Issues and PRs related to the module subsystem.

Comments

@niksajanjic
Copy link

  • Version: both 8.12 and 10.12
  • Platform: Darwin Niksas-MacBook-Pro.local 17.7.0 Darwin Kernel Version 17.7.0: Thu Jun 21 22:53:14 PDT 2018; root:xnu-4570.71.2~1/RELEASE_X86_64 x86_64

Folder structure:

Test
├── OtherFolder
|   └── main.js
├── SubFolder
|   └── index.js
└── index.js

index.js

console.log(require.resolve.paths('./'))
console.log(require.resolve('./'))

SubFolder/index.js

console.log(require.resolve.paths('./'))
console.log(require.resolve('./'))
console.log(require.resolve('./', { paths: ['/Users/niksajanjic/Projects/Test/OtherFolder'] }))

OtherFolder/main.js

console.log(require.resolve.paths('./'))
console.log(require.resolve('./'))

If I run node index.js:

[ '/Users/niksajanjic/Projects/Test' ]
/Users/niksajanjic/Projects/Test/index.js

If I run node SubFolder/index.js (expected an error in the last line, not the resolvement):

[ '/Users/niksajanjic/Projects/Test/SubTest' ]
/Users/niksajanjic/Projects/Test/SubTest/index.js
/Users/niksajanjic/Projects/Test/index.js

If I run node OtherFolder/main.js:

[ '/Users/niksajanjic/Projects/Test/BugTest' ]
internal/modules/cjs/loader.js:582
    throw err;
    ^

Error: Cannot find module './'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:580:15)
    at Function.resolve (internal/modules/cjs/helpers.js:30:19)
    at Object.<anonymous> (/Users/niksajanjic/Projects/Test/BugTest/main.js:2:21)
    at Module._compile (internal/modules/cjs/loader.js:688:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:699:10)
    at Module.load (internal/modules/cjs/loader.js:598:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:537:12)
    at Function.Module._load (internal/modules/cjs/loader.js:529:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:741:12)
    at startup (internal/bootstrap/node.js:285:19)

I feel the bug is the resolvement of this line console.log(require.resolve('./', { paths: ['/Users/niksajanjic/Projects/Test/OtherFolder'] })) inside SubFolder/index.js into /Users/niksajanjic/Projects/Test/index.js. That was unexpected for me and I expected it to error out same as running console.log(require.resolve('./')) inside OtherFolder/main.js errored out with MODULE_NOT_FOUND. I feel the result of those 2 calls should be exactly the same.

This means that if you have a file in the root folder (let's say main.js) and if you try to look for the file from A folder into B folder that has the same name as the one in root folder (require.resolve('./main.js', { paths: [ /path/to/folder/B ] })) it will never error out. Whether you have main.js inside B folder or not, the resolvement would still happen and return the path to the file in the root folder. This is very dangerous, this basically means you can't rely on making relative path resolvements inside other paths without the fear of them being resolved to files in the root directory with the same name, and that is a huge probability while using filenames like index.js, main.js, app.js or style.js.

While we're at this subject, could we benefit from an optional options argument inside require.resolve.paths(request, options) so we can see which paths does it resolve to?

If this is a bug and not an indended behavior, I could dig it up a bit and debug, maybe even come out with some PR. I know the problem is somewhere inside Module._resolveFilename function inside internal/modules/cjs/loader file.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 13, 2018

Is this the same issue as #18408? If so, see #18408 (comment)

@niksajanjic
Copy link
Author

Hm, seems like it is. I just tried running my code in 9.2.1 as the author of that issue reported and I got the expected result. I'll just double check tomorrow to make sure it's the same issue and close this one in favor of the later.

Not sure how this one eluded my eyes when I was trying to find similar issues before reporting it.

@Fishrock123 Fishrock123 added the module Issues and PRs related to the module subsystem. label Oct 15, 2018
@niksajanjic
Copy link
Author

This other issue seems similar, so I'll close this one in favor of it.

cjihrig added a commit to cjihrig/node that referenced this issue Dec 11, 2018
The paths used by require.resolve() should be treated as
starting points for module resolution, and not actually
searched.

PR-URL: nodejs#23683
Fixes: nodejs#18408
Refs: nodejs#23643
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
The paths used by require.resolve() should be treated as
starting points for module resolution, and not actually
searched.

PR-URL: nodejs#23683
Fixes: nodejs#18408
Refs: nodejs#23643
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants