module: restore and warn on require('.') usage with NODE_PATH

[silverwind]

related: #1356, #1358

require('.'), like require('./') did not use module search paths, which inself is probably alright, as it is unexpected that . would refer to anything outside PWD.

It appears some users (#1356) were relying on unintended (and in my eyes bugged) behaviour when NODE_PATH contains a index.js, and surprisingly, require('.') did resolve to that module (which it probably never should have done).

This patch fixes the case of require('.') + NODE_PATH by letting require('.') look up the search paths. require('.') still works as expected when NODE_PATH doesn't contain a index.js.

If both NODE_PATH and PWD contain index.js, PWD is preferred (as it comes first in the array of search paths), which I think is the more sane way to fix that conflict, as it doesn't encourage relying on undefined behaviour.

[rlidwka]

Deprecation warning + note to remove this quirk in 2.0 maybe?

[silverwind]

@rlidwka you mean NODE_PATH deprecation? What are the alternatives (cc: @rvagg)?

[rlidwka]

I mean, if . points to something else than cwd, we could ask user not to do that.

Looking at the code, this suggestion might be too complex though. :(

[silverwind]

Looking into adding a warning.

Few more notes:

• This also searches node_modules and other default paths on require('.'). Probably not intended, but that's how it worked before.
• I would've liked to add a test, but as NODE_PATH is only read once during startup, I can't modify it during a test without restarting the process on each change.

[silverwind]

One more blocking issue I've noticed: path.resolve('.') does resolve differently when doing a process.chdir(). require shouldn't do that.

It seems the suitable __dirname isn't defined inside internal modules. Any suggestions on how to obtain the current script's directory (process.env.PWD, but cross-platform)?

[rlidwka]

Any suggestions on how to obtain the current script's directory (process.env.PWD, but cross-platform)?

Is process.cwd() available from there?

[bnoordhuis]

Note that process.cwd() can fail with ENOENT if the working directory has been unlinked.

[silverwind]

process.cwd() is unsuitable as it changes with process.chdir(). I need something static.

Edit: I think I have an idea how to solve it. Hold on.

[silverwind]

Updated with a better way of obtaining PWD, which is also resilient to process.cwd().

Not sure the fallback to path.resolve is really necessary, but there is this codepath, which can lead to parent = undefined, so I left it there.

Also added a warning if . did not resolve to PWD, but I'm not sure if that's the best case of action as we only have one other warning like this in the codebase.

[silverwind]

Will look into changing that console.error to util.deprecate.

[silverwind]

Any advice on how to use util.deprecate in this case? It seems util.deprecate is tuned for deprecating functions. In this case, I'd like to print a single warning if this condition is met.

[rvagg]

just a thought, and perhaps this is too much of a hack but:

var noopDeprecateRequireDotFile = util.deprecate(function() {}, 'message here')

...

     if (request === '.' && i > 0) noopDeprecateRequireDotFile();

[silverwind]

I'd be fine with something like that, but in the long term, we probably need something like util.warnOnce(msg). These probably shouldn't be public methods either.

[silverwind]

Updated warning with suggested usage of module.deprecate. I've tested enough to be sure this won't cause any regression. All it does is to allow '.' to resolve outside the module dir. It does so by putting the equivalent of __dirname in first position of the module search paths, so these look somthing like this:

['.', /* NODE_PATH entries */ ,'node_modules', '.node_libraries']

If we hit a index in that array except the first, we warn once. Note that require('.') will prefer a module in __dirname over one in PATH. This is the only difference to the previous unintended workings of require('.').

Please review @iojs/collaborators

[rvagg]

the use of a template string seems unnecessary here

[rvagg]

also, "did resolve" could be just "resolved"

[rvagg]

lgtm, perhaps could do with a test though? we may want to at least flag it in the tests as functionality that's being used in the wild--not having that covered is why we got into this position in the first place

[silverwind]

Agreed. I first thought doing a test wasn't easily possible because NODE_ENV was only read once at startup, but i've just discovered I can call module._initPaths(); to reinitialze paths.

[silverwind]

Nits fixed and test added, please have a final look.

[Fishrock123]

will be deprecated soon.

sounds like it already is deprecated?

[silverwind]

How about "will be removed soon" ?

[Fishrock123]

"is deprecated" should be fine, or "is deprecated and will be removed"

[silverwind]

@Fishrock123 fixed the message. Adding semver-minor to satisfy semver deprecation requirements.

[chrisdickinson]

Feel free to merge this at will.

[silverwind]

@Fishrock123 LGTY?

[Fishrock123]

yeah LGTM

[silverwind]

Landed in #1363

I'll follow up with the removal commit for 2.0 shortly.