module: prioritize current directory for local lookup #5689

phillipj · 2016-03-13T20:54:19Z

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

module, test

Description of change

This fixes an issue reported in #5684 about module in node_modules being preferred over a local file with the same name, when resolved in repl (or --eval mode). The bug originates from the fact that Module._resolveLookupPaths() puts node_modules before the current directory when run from repl or --eval.

Previous results from Module._resolveLookupPaths('./lodash'):

[ './lodash',
  [ '/Users/phillipj/Dev/repl-lookup-test/node_modules',
    '/Users/phillipj/Dev/node_modules',
    '/Users/phillipj/node_modules',
    '/Users/node_modules',
    '/node_modules',
    '.',
    '/Users/phillipj/.node_modules',
    '/Users/phillipj/.node_libraries',
    '/Users/phillipj/Dev/node/out/lib/node' ] ]

Fixed results from Module._resolveLookupPaths('./lodash'):

[ './lodash',
  [ '.',
    '/Users/phillipj/Dev/repl-lookup-test/node_modules',
    '/Users/phillipj/Dev/node_modules',
    '/Users/phillipj/node_modules',
    '/Users/node_modules',
    '/node_modules',
    '/Users/phillipj/.node_modules',
    '/Users/phillipj/.node_libraries',
    '/Users/phillipj/Dev/node/out/lib/node' ] ]

MylesBorins · 2016-03-13T21:05:57Z

@phillipj thanks for getting this in! I think it might make sense for you to include an extra test that I have written that can be found in MylesBorins@7f2f0cb

To quickly add that commit to your branch you can run the command

$ curl https://github.com/TheAlphaNerd/node/commit/7f2f0cb946d7889fcdef7aff9d895574da9c14b3.patch | git am --whitespace=fix

MylesBorins · 2016-03-13T21:12:09Z

@phillipj it looks like your test is failing the linter

/Users/thealphanerd/code/node/test/parallel/test-module-relative-lookup.js
   3:5  error  'common' is defined but never used             no-unused-vars
  11:1  error  Line 11 exceeds the maximum line length of 80  max-len

phillipj · 2016-03-13T21:13:08Z

Thanks for the protip cherry-picking that with ease @thealphanerd! Would you prefer two commits or should I squash your test into my initial commit?

phillipj · 2016-03-13T21:14:04Z

it looks like your test is failing the linter

Yupp, sorry about that. Just pushed an updated.

MylesBorins · 2016-03-13T21:15:43Z

@phillipj I would keep them separate as you didn't write the code. Better to keep the blames on who wrote it 😄

phillipj · 2016-03-13T21:18:28Z

@thealphanerd haha good point :)

MylesBorins · 2016-03-13T21:19:27Z

once you push the update I'll run CI + citgm... this is one of those fixes that could be argued to be either patch / major... let's see what we break 😄

phillipj · 2016-03-13T21:19:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1914/

MylesBorins · 2016-03-13T21:25:51Z

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/112/

It looks like things in smoker land might be weird... the results might not be useful until I have more time to dig into infra problems tomorrow

phillipj · 2016-03-13T21:28:03Z

Alright, fingers crossed CI will pass at least.

MylesBorins · 2016-03-13T21:53:15Z

CI looks good... only failure was infra related

benjamingr · 2016-03-13T23:32:38Z

LGTM. Thanks for the quick fix.

MylesBorins · 2016-03-13T23:40:10Z

citgm has some failures, but they all seems to be unrelated. I would like to try and get another run going tomorrow though to confirm

evanlucas · 2016-03-14T03:36:45Z

Was this a regression or has this always been the behavior?

mscdex · 2016-03-14T07:19:29Z

@evanlucas The behavior before this PR has existed at least since v0.10.

jasnell · 2016-03-14T15:43:24Z

/cc @nodejs/ctc

phillipj · 2016-03-18T14:23:18Z

I'm assuming @nodejs/ctc got pinged for a patch/major decision.

Although I fully acknowledge the fact we have to be a little too careful at times about what's decided to be a breaking change, I see this as a bug and therefore a patch.

FWIW I can't imagine someone out there relying on not being able to require their own local .js-file because it's over shadowed by a 3rd party module. That's very confusing at best and could potentially be a debugging nightmare.

cjihrig · 2016-03-18T14:29:26Z

test/parallel/test-repl-require.js

Please use strictEqual() here.

cjihrig · 2016-03-18T14:32:43Z

Some comments, but LGTM pending CI.

MylesBorins · 2016-03-29T18:17:19Z

@Fishrock123 that stylus bug is unrelated

phillipj · 2016-03-29T18:48:11Z

@thealphanerd which means this is ready to land as is, right?

MylesBorins · 2016-03-29T18:49:31Z

none of the citgm results are a problem.

Fishrock123 · 2016-03-29T19:13:14Z

@phillipj I believe so. LGTM 🚢

This fixes a bug where a 3rd party module found in node_modules, would be preferred over a ./local module with the same name. Fixes: nodejs#5684 PR-URL: nodejs#5689 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Currently we are not testing that resolution of local paths is resolved first in the repl. This addition to `test-repl-require` adds an additional fixture an ensures we won't regress in the future PR-URL: nodejs#5689 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

This fixes a bug where a 3rd party module found in node_modules, would be preferred over a ./local module with the same name. Fixes: #5684 PR-URL: #5689 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Currently we are not testing that resolution of local paths is resolved first in the repl. This addition to `test-repl-require` adds an additional fixture an ensures we won't regress in the future PR-URL: #5689 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

phillipj · 2016-03-29T19:43:59Z

Landed in d38503a and 652782d

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

phillipj added the module Issues and PRs related to the module subsystem. label Mar 13, 2016

phillipj mentioned this pull request Mar 13, 2016

relative module paths resolve differently in REPL. #5684

Closed

phillipj force-pushed the module-relative-lookup-fix branch from 25ce2ae to 1f510ed Compare March 13, 2016 21:07

MylesBorins mentioned this pull request Mar 13, 2016

module: the node_modules paths always should be placed below the current directory #5690

Closed

4 tasks

mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 13, 2016

cjihrig reviewed Mar 18, 2016
View reviewed changes

test/parallel/test-repl-require.js Outdated

Copy link

Contributor

cjihrig Mar 18, 2016

Choose a reason for hiding this comment

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

Please use strictEqual() here.

phillipj force-pushed the module-relative-lookup-fix branch from 8b52296 to 20866e2 Compare March 18, 2016 19:29

phillipj and others added 2 commits March 29, 2016 21:28

phillipj force-pushed the module-relative-lookup-fix branch from b311eb8 to fc19540 Compare March 29, 2016 19:30

phillipj closed this Mar 29, 2016

phillipj deleted the module-relative-lookup-fix branch March 29, 2016 19:44

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

ChALkeR mentioned this pull request May 2, 2016

API stability level «Locked» is inaccurate #6528

Closed

ChALkeR mentioned this pull request Feb 8, 2017

Changing Definition of "Locked" for Stability Index in API Docs #11200

Closed

Uh oh!

module: prioritize current directory for local lookup #5689

module: prioritize current directory for local lookup #5689

Uh oh!

Conversation

phillipj commented Mar 13, 2016

Affected core subsystem(s)

Description of change

Uh oh!

MylesBorins commented Mar 13, 2016

Uh oh!

MylesBorins commented Mar 13, 2016

Uh oh!

phillipj commented Mar 13, 2016

Uh oh!

phillipj commented Mar 13, 2016

Uh oh!

MylesBorins commented Mar 13, 2016

Uh oh!

phillipj commented Mar 13, 2016

Uh oh!

MylesBorins commented Mar 13, 2016

Uh oh!

phillipj commented Mar 13, 2016

Uh oh!

MylesBorins commented Mar 13, 2016

Uh oh!

phillipj commented Mar 13, 2016

Uh oh!

MylesBorins commented Mar 13, 2016

Uh oh!

benjamingr commented Mar 13, 2016

Uh oh!

MylesBorins commented Mar 13, 2016

Uh oh!

evanlucas commented Mar 14, 2016

Uh oh!

mscdex commented Mar 14, 2016

Uh oh!

jasnell commented Mar 14, 2016

Uh oh!

phillipj commented Mar 18, 2016

Uh oh!

cjihrig Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Mar 18, 2016

Uh oh!

MylesBorins commented Mar 29, 2016

Uh oh!

phillipj commented Mar 29, 2016

Uh oh!

MylesBorins commented Mar 29, 2016

Uh oh!

Fishrock123 commented Mar 29, 2016

Uh oh!

phillipj commented Mar 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants