-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
require has issues when run from / #6679
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Comments
@mhart thanks for reporting! would you be able to test this on v4 to see that it is indeed a regression in v6. |
It appears that this behavior is broken on v6, but works on v4 and v5. I have a sneaking suspicion that #6537 is going to fix this. What would be the best way of writing a regression test for this? I doubt we are going to want to be writing to |
Building jasnell:revert-5950 as we speak – will let you know if it fixes the issue |
Ok, I built $ node --version
v7.0.0-pre
$ cd /
$ npm install express
$ node -p "require('express')"
module.js:431
throw err;
^
Error: Cannot find module 'merge-descriptors'
at Function.Module._resolveFilename (module.js:429:15)
at Function.Module._load (module.js:377:25)
at Module.require (module.js:457:17)
at require (internal/module.js:20:19)
at Object.<anonymous> (/node_modules/express/lib/express.js:16:13)
at Module._compile (module.js:532:32)
at Object.Module._extensions..js (module.js:541:10)
at Module.load (module.js:447:32)
at tryModuleLoad (module.js:406:12)
at Function.Module._load (module.js:398:3) |
This leads me to believe it might be something in libuv /cc @saghul |
Some more fuel to add to the fire: It works with v6 and npm@2 – so it must be something to do with the directory layout of |
So if I do: $ npm --version
3.8.9
$ node --version
v7.0.0-pre
$ npm install express
$ node -p "require('express')"
# errors
$ mkdir node_modules/express/node_modules
$ mv node_modules/* node_modules/express/node_modules/
# (ignore rename error)
$ ls node_modules
express
$ node -p "require('express')"
# works |
@mhart can you try running with the |
@evanlucas sure thing!
|
So for some reason |
But it is in the first search (which is why |
Here's the relevant output from v5: MODULE 15: Module._load REQUEST vm parent: [eval]
MODULE 15: load native module vm
MODULE 15: Module._load REQUEST express parent: [eval]
MODULE 15: looking for "express" in ["//node_modules","/node_modules","/root/.node_modules","/root/.node_libraries","/usr/lib/node"]
MODULE 15: load "/node_modules/express/index.js" for module "/node_modules/express/index.js"
MODULE 15: Module._load REQUEST ./lib/express parent: /node_modules/express/index.js
MODULE 15: RELATIVE: requested: ./lib/express set ID to: /node_modules/express/index.js/lib/express from /node_modules/express/index.js
MODULE 15: looking for "/node_modules/express/index.js/lib/express" in ["/node_modules/express"]
MODULE 15: load "/node_modules/express/lib/express.js" for module "/node_modules/express/lib/express.js"
MODULE 15: Module._load REQUEST events parent: /node_modules/express/lib/express.js
MODULE 15: load native module events
MODULE 15: Module._load REQUEST merge-descriptors parent: /node_modules/express/lib/express.js
MODULE 15: looking for "merge-descriptors" in ["/node_modules/express/lib/node_modules","/node_modules/express/node_modules","/node_modules","/root/.node_modules","/root/.node_libraries","/usr/lib/node"]
MODULE 15: load "/node_modules/merge-descriptors/index.js" for module "/node_modules/merge-descriptors/index.js"
... |
yea, I wonder if this could be related to @mscdex's perf improvements? |
This patch fixes the issue: diff --git a/lib/module.js b/lib/module.js
index 344ea97..4589f21 100644
--- a/lib/module.js
+++ b/lib/module.js
@@ -253,7 +253,7 @@ if (process.platform === 'win32') {
// note: this approach *only* works when the path is guaranteed
// to be absolute. Doing a fully-edge-case-correct path.split
// that works on both Windows and Posix is non-trivial.
- const paths = [];
+ const paths = ['/node_modules'];
var p = 0;
var last = from.length;
for (var i = from.length - 1; i >= 0; --i) { I'm not sure it is the correct approach and if a similar change has to be done for win32. |
The only relation between libuv and the module system is the use of |
#6670 may fix this issue actually |
I have a similar issue having v6.1.0 node and project in UNC share which I serve in Windows through a network drive Z:\ (or that could be any letter it does not matter). ... |
am having this issue trying to use nodeunit too test my code. it shows this error `module.js:440 Error: Cannot find module '../modules/math' |
The `p < nmLen` condition will fail when a module's name is end with `node_modules` like `foo_node_modules`. The old logic will miss the `foo_node_modules/node_modules` in node_modules paths. TL;TR, a module named like `foo_node_modules` can't require any module in the node_modules folder. Fixes: #6679 PR-URL: #6670 Reviewed-By: Evan Lucas <evanlucas@me.com>
Uh oh!
There was an error while loading. Please reload this page.
Darwin ReBuke-Pro.local 15.4.0 Darwin Kernel Version 15.4.0: Fri Feb 26 22:08:05 PST 2016; root:xnu-3248.40.184~3/RELEASE_X86_64 x86_64
andLinux f50a089ff63e 4.4.8-boot2docker #1 SMP Mon Apr 25 21:57:27 UTC 2016 x86_64 Linux
The following command works in v5/v4/v0.12/v0.10 with npm@2.15.5 or npm@3.8.9, works in v6 with npm@2.15.5, but fails in v6 with npm@3.8.9:
The following command works in v6 (and below):
$ sudo mkdir /app && cd /app && sudo npm install express && sudo node -p "require('express')"
You can reproduce this fairly easily without messing up your filesystem if you have docker (can use
mhart/alpine-node
instead ofnode
below if you want a smaller download):$ docker run node:5 sh -c 'npm install express && node -p "require(\"express\")"'
(^works)
$ docker run node:6 sh -c 'npm install express && node -p "require(\"express\")"'
(^doesn't work)
$ docker run node:6 sh -c 'mkdir app && cd app && npm install express && node -p "require(\"express\")"'
(^works)
It's unclear to me whether it's the fact that it's being run from the root directory specifically that's causing it to fail – or whether it's the presence of one of the other directories (eg,
bin
) that's getting in the wayThe text was updated successfully, but these errors were encountered: