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
base: v1.x
from

Conversation

Projects
None yet
7 participants
@silverwind
Contributor

silverwind commented Apr 7, 2015

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 label Apr 7, 2015

@rlidwka

This comment has been minimized.

Show comment
Hide comment
@rlidwka

rlidwka Apr 7, 2015

Contributor

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

Contributor

rlidwka commented Apr 7, 2015

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

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 7, 2015

Contributor

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

Contributor

silverwind commented Apr 7, 2015

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

@rlidwka

This comment has been minimized.

Show comment
Hide comment
@rlidwka

rlidwka Apr 7, 2015

Contributor

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. :(

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

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 7, 2015

Contributor

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.
Contributor

silverwind commented Apr 7, 2015

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

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 7, 2015

Contributor

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)?

Contributor

silverwind commented Apr 7, 2015

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

This comment has been minimized.

Show comment
Hide comment
@rlidwka

rlidwka Apr 7, 2015

Contributor

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

Is process.cwd() available from there?

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

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Apr 7, 2015

Member

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

Member

bnoordhuis commented Apr 7, 2015

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

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 7, 2015

Contributor

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.

Contributor

silverwind commented Apr 7, 2015

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

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 7, 2015

Contributor

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.

Contributor

silverwind commented Apr 7, 2015

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

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 8, 2015

Contributor

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

Contributor

silverwind commented Apr 8, 2015

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

Show outdated Hide outdated lib/module.js Outdated

@chrisdickinson chrisdickinson referenced this pull request Apr 9, 2015

Closed

Reversion Policy #48

@silverwind silverwind changed the title from module: search NODE_PATH in require('.') to module: restore and warn on require('.') usage with NODE_PATH Apr 11, 2015

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 11, 2015

Contributor

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

Contributor

silverwind commented Apr 11, 2015

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

Show outdated Hide outdated lib/module.js Outdated
@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Apr 15, 2015

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 15, 2015

Contributor

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.

Contributor

silverwind commented Apr 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 16, 2015

Contributor

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

Contributor

silverwind commented Apr 16, 2015

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

Show outdated Hide outdated lib/module.js Outdated

@Fishrock123 Fishrock123 added this to the 1.8.0 milestone Apr 16, 2015

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 16, 2015

Contributor

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

Contributor

silverwind commented Apr 16, 2015

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

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Apr 16, 2015

Contributor

Feel free to merge this at will.

Contributor

chrisdickinson commented Apr 16, 2015

Feel free to merge this at will.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind
Contributor

silverwind commented Apr 16, 2015

@Fishrock123 LGTY?

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 16, 2015

Member

yeah LGTM

Member

Fishrock123 commented Apr 16, 2015

yeah LGTM

silverwind added a commit that referenced this pull request Apr 16, 2015

module: handle NODE_PATH in require('.')
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 <fishrock123@rocketmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Apr 16, 2015

Contributor

Landed in #1363

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

Contributor

silverwind commented Apr 16, 2015

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

2015-04-17 io.js v1.8.0 Release
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

2015-04-17 io.js v1.8.0 Release
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

2015-04-20 io.js v1.8.1 Release
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment