If $NODE_PATH ends with a delimiter, require will act unexpected

[chrisyip]

This is a copy of #1487 that follows contributing guide. Sorry for the trouble.

---

Here is a repo that explains the difference: https://github.com/chrisyip/NODE_PATH_DEMO.

Using node v1.8.1
Current NODE_PATH is '/usr/local/lib/node_modules'
Executing 'node foo/bar.js', you should seeing an error...

In foo/bar.js
module.js:336
    throw err;
          ^
Error: Cannot find module 'index.js'
    at Function.Module._resolveFilename (module.js:334:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:363:17)
    at require (module.js:382:17)
    at Object.<anonymous> (/Users/Chris/Workspace/node/NODE_PATH_DEMO/foo/bar.js:2:13)
    at Module._compile (module.js:428:26)
    at Object.Module._extensions..js (module.js:446:10)
    at Module.load (module.js:353:32)
    at Function.Module._load (module.js:308:12)
    at Function.Module.runMain (module.js:469:10)

Appeding a trailing colon to NODE_PATH
NODE_PATH now is '/usr/local/lib/node_modules:' <- notice the colon in the end
Executing 'node foo/bar.js'...

In foo/bar.js
index.js loaded
index.js: I should not be required

After separation, nodePath will become [ '/usr/local/lib/node_modules', '' ] (in the above case).

I know node should not take the responsibility for env variables, but since '' is meaningless, I suggest to remove '' from nodePath before using it.

[Fishrock123]

I suppose a question would be if we want to bother fixing this? (The fix itself is simple enough.)

I've never used NODE_PATH before, but it sounds like there are a multitude of other things that could be fed to it where it would break horribly.

[rlidwka]

style: line should not exceed 80 characters unless you really have to

[rlidwka]

I suppose a question would be if we want to bother fixing this? (The fix itself is simple enough.)

Yes.

Funny enough, this is a breaking change similar to #1356. So it's better to fix it, so people won't rely on this in the future.

[Fishrock123]

@rlidwka any idea when it broke, out of curiosity?

[silverwind]

@Fishrock123 I don't think anything 'broke' here. The PR looks to fix an unintended resolution of '' to $PWD, which could possibly break things for people with an invalid NODE_PATH containing trailing separators. The chance to encounter such a broken setup in the wild is probably close to zero.

[chrisyip]

Yes, this PR will only affect $NODE_PATH containing trailing separators.

I create this PR is because there're so many articles encouraging people to use $NODE_PATH to resolve some specific requirements. For people that knows how env variables work, it should not be a problem, but for novices, it's too dangerous.

Here's an example why it's dangerous.

First, Let's assume there's a module foo, and it has a dependency called bar, but due to some reason, bar is missing. What will we get if $NODE_PATH contains a trailing separator:

foo is accessing global module!

Without trailing separators, what we get is just an error: Error: Cannot find module 'bar'.

So I strongly recommend fixing this issue.

[silverwind]

No need to filter paths here, its entries aren't user-supplied. A small perf gain would be:

paths = nodePath.split(path.delimiter).filter(function(path) {
  return !!path;
}).concat(paths);

Also, semicolon.

[silverwind]

Also, you could do

return path !== '';

to make it more clear that the intend is to filter the empy string.

[Fishrock123]

I had been thinking path.length !== 0, but really, !!path is fine.

[silverwind]

Yeah it probably is, given the input is a string it should be quite obvious what it does.

[chrisyip]

@silverwind @Fishrock123 path !== '' is slightly faster than !!path, but !!path can handle some special situations, e.g. path == null, in this case, path.length !== 0 might throw an exception.

Using node 1.7.1
!!str x 81,087,829 ops/sec ±1.46% (183 runs sampled)
str !== '' x 88,814,451 ops/sec ±0.98% (184 runs sampled)
str.length !== 0 x 87,047,827 ops/sec ±0.96% (186 runs sampled)

[silverwind]

LGTM

I've researched the cause and it boils down to path.resolve('','index.js') returning $PWD/index.js.

@chrisyip if you can, please squash the commits into one and add a commit message and short decription of the problem solved in the commit body as detailed here: https://github.com/iojs/io.js/blob/v1.x/CONTRIBUTING.md#step-3-commit

[silverwind]

Thanks! merged in 7384ca8

If anyone calls this a breaking change, we can point them to the docs about their invalid NODE_PATH :)

[Fishrock123]

If anyone calls this a breaking change, we can point them to the docs about their invalid NODE_PATH

I'm slightly uneasy about touching modules now too, but it's in a major anyways.