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

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

Closed
wants to merge 6 commits into from

Conversation

silverwind
Copy link
Contributor

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.

@silverwind silverwind added the module Issues and PRs related to the module subsystem. label Apr 7, 2015
@rlidwka
Copy link
Contributor

rlidwka commented Apr 7, 2015

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

@silverwind
Copy link
Contributor Author

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

@rlidwka
Copy link
Contributor

rlidwka commented Apr 7, 2015

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

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

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

rlidwka commented Apr 7, 2015

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

Is process.cwd() available from there?

@bnoordhuis
Copy link
Member

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

@silverwind
Copy link
Contributor Author

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

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

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

@@ -169,6 +169,9 @@ Module._findPath = function(request, paths) {
}

if (filename) {
if (request === '.' && i > 0) {
console.error(`(node) warning: require('.') resolved to ${filename}`);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 silverwind changed the title module: search NODE_PATH in require('.') module: restore and warn on require('.') usage with NODE_PATH Apr 11, 2015
@silverwind
Copy link
Contributor Author

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

@@ -125,6 +125,11 @@ function tryExtensions(p, exts) {
}


const noopDeprecateRequireDot = util.deprecate(function() {},
`warning: require('.') did resolve outside the package directory. ` +
Copy link
Member

Choose a reason for hiding this comment

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

the use of a template string seems unnecessary here

Copy link
Member

Choose a reason for hiding this comment

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

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

@rvagg
Copy link
Member

rvagg commented Apr 15, 2015

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

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

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

@@ -125,6 +125,11 @@ function tryExtensions(p, exts) {
}


const noopDeprecateRequireDot = util.deprecate(function() {},
"warning: require('.') resolved outside the package directory. " +
"This functionality will be deprecated soon.");
Copy link
Contributor

Choose a reason for hiding this comment

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

will be deprecated soon.

sounds like it already is deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about "will be removed soon" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@Fishrock123 Fishrock123 added this to the 1.8.0 milestone Apr 16, 2015
@silverwind silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 16, 2015
@silverwind
Copy link
Contributor Author

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

@chrisdickinson
Copy link
Contributor

Feel free to merge this at will.

@silverwind
Copy link
Contributor Author

@Fishrock123 LGTY?

@Fishrock123
Copy link
Contributor

yeah LGTM

silverwind added a commit that referenced this pull request Apr 16, 2015
This commit restores the functionality of adding a module's path to
NODE_PATH and requiring it with require('.'). As NODE_PATH was never
intended to be used as a pointer to a module directory (but instead, to
a directory containing directories of modules), this feature is also
being deprecated in turn, to be removed at a later point in time.

PR-URL: #1363
Fixes: #1356
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@silverwind
Copy link
Contributor Author

Landed in #1363

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

@silverwind silverwind closed this Apr 16, 2015
chrisdickinson added a commit that referenced this pull request Apr 17, 2015
Notable Changes:

* build: Support for building io.js as a static library (Marat Abdullin) #1341
* deps: upgrade openssl to 1.0.2a (Shigeki Ohtsu) #1389
* npm: Upgrade npm to 2.8.3. (Forrest L Norvell) #1448
* src: allow multiple arguments to be passed to process.nextTick (Trevor Norris) #1077
* module: interaction of require('.') with NODE_PATH has been restored and deprecated.
  This functionality will be removed at a later point. (Roman Reiss) #1363
chrisdickinson added a commit that referenced this pull request Apr 17, 2015
Notable Changes:

* build: Support for building io.js as a static
  library (Marat Abdullin) #1341
* deps: upgrade openssl to 1.0.2a (Shigeki Ohtsu) #1389
* npm: Upgrade npm to 2.8.3. (Forrest L Norvell) #1448
* src: allow multiple arguments to be passed to
  process.nextTick (Trevor Norris) #1077
* module: the interaction of require('.') with NODE_PATH has been
  restored and deprecated. This functionality will be removed at
  a later point. (Roman Reiss) #1363
chrisdickinson added a commit that referenced this pull request Apr 20, 2015
Notable Changes:

* build: revert vcbuild.bat changes
* changes inherited from v1.8.0:
  * build: Support for building io.js as a static
    library (Marat Abdullin) #1341
  * npm: Upgrade npm to 2.8.3. (Forrest L Norvell) #1448
  * deps: upgrade openssl to 1.0.2a (Shigeki Ohtsu) #1389
  * src: allow multiple arguments to be passed to
    process.nextTick (Trevor Norris) #1077
  * module: the interaction of require('.') with NODE_PATH has been
    restored and deprecated. This functionality will be removed at
    a later point. (Roman Reiss) #1363
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants