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

relative module paths resolve differently in REPL. #5684

Closed
jdalton opened this issue Mar 13, 2016 · 9 comments
Closed

relative module paths resolve differently in REPL. #5684

jdalton opened this issue Mar 13, 2016 · 9 comments
Labels
confirmed-bug Issues with confirmed bugs. path Issues and PRs related to the path subsystem.

Comments

@jdalton
Copy link
Member

jdalton commented Mar 13, 2016

  • Version: v5.5.0
  • Platform: Darwin jdalton.local 15.3.0 Darwin Kernel Version 15.3.0: Thu Dec 10 18:40:58 PST 2015; root:xnu-3248.30.4~1/RELEASE_X86_64 x86_64
npm init
# <init a package>
npm i lodash --save
touch lodash.js

In lodash.js

module.exports = {
  'VERSION': 'should be me'
};

Now in the REPL

console.log(require('./lodash').VERSION);
// => 4.6.1 instead of 'should be me'

Put the same code in a file

touch foo.js

In foo.js

console.log(require('./lodash').VERSION);

Then

node foo
# logs 'should be me'

It would work in the REPL if I changed the path from './lodash' to './lodash.js'.

@benjamingr benjamingr added the path Issues and PRs related to the path subsystem. label Mar 13, 2016
@benjamingr
Copy link
Member

@jdalton thanks for the report!

Operating system?

Locally - On 5.1.0 I see:

$ node
> require("./lodash.js").VERSION
'should be me'

Updating to 5.8 I get:

$ node
> require("./lodash.js").VERSION
'should be me'

So assuming it didn't break between 5.1 and 5.8 I suspect it might be OS related.

@jdalton
Copy link
Member Author

jdalton commented Mar 13, 2016

@benjamingr

Operating system?

OSX 10.11.3

So assuming it didn't break between 5.1 and 5.8 I suspect it might be OS related.

The issue is require('./lodash') didn't work. You're doing require('./lodash.js').

@benjamingr
Copy link
Member

Oh sorry, my bad. I see 4.6.1 and not 'should be me' too for that on windows.

@jdalton
Copy link
Member Author

jdalton commented Mar 13, 2016

Cool, so it repros on Windows and OSX.

@benjamingr benjamingr added the confirmed-bug Issues with confirmed bugs. label Mar 13, 2016
@benjamingr
Copy link
Member

require("module")._resolveFilename("./lodash") // returns the value in node_modules

Since _resolveLookupPaths returns:

[ './lodash',
  [ 'C:\\issue-5684\\node_modules',
    'C:\\node_modules',
    '.',
    'C:\\Users\\Benjamin\\.node_modules',
    'C:\\Users\\Benjamin\\.node_libraries',
    'C:\\Program Files (x86)\\lib\\node' ] ]

Which finds lodash if I do:

var paths = require("module")._resolveLookupPaths("./lodash")
require("module")._findPath("./lodash", paths[1]) // C:\\issue-5684\\node_modules\\lodash\\lodash.js

Which happens because of "hacky" code (terminology used) in repl.js. I have to leave now, but I'm at

node/lib/module.js

Lines 263 to 264 in 7c60328

var mainPaths = ['.'].concat(modulePaths);
mainPaths = Module._nodeModulePaths('.').concat(mainPaths);
and I think the node_modules paths should be placed below the . and not above.

I'll keep debugging later - anyone feel free to pick this up.

@watilde
Copy link
Member

watilde commented Mar 13, 2016

to place the node_modules paths below the . and not above, the part of the _resolveLookupPaths should be like the following?

- var mainPaths = ['.'].concat(modulePaths);
- mainPaths = Module._nodeModulePaths('.').concat(mainPaths);
+ var mainPaths = Module._nodeModulePaths('.').concat(mainPaths);
+ mainPaths = ['.'].concat(modulePaths);

@MylesBorins
Copy link
Contributor

I think I have a working test... going to see if this fix makes things work

@phillipj
Copy link
Member

@thealphanerd anything near what I've done in #5689?

@MylesBorins
Copy link
Contributor

@phillipj the test is different. There is already a test-repl-require so I just bootstrapped on that.

Your test might be good to have as well, but the one I used is running in the repl itself. I'm going to push it to a fork, might make sense for you to cherry pick and add to your PR... I'll post it in there

phillipj added a commit to phillipj/node that referenced this issue Mar 29, 2016
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants