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

doesn't support $HOME/.node_modules or $HOME/.node_libraries #163

Closed
mk-pmb opened this issue Jun 2, 2018 · 30 comments · Fixed by #273
Closed

doesn't support $HOME/.node_modules or $HOME/.node_libraries #163

mk-pmb opened this issue Jun 2, 2018 · 30 comments · Fixed by #273

Comments

@mk-pmb
Copy link

mk-pmb commented Jun 2, 2018

Hi, thanks for this module!
It seems to have trouble finding my packages, though:

$ readlink -m ~/.node_modules
/mnt/…/nodejs/modules
$ nodejs -e 'console.log(require.resolve("lodash")); require("resolve")("lodash", console.log);'
/mnt/…/nodejs/modules/lodash/lodash.js
{ Error: Cannot find module 'lodash' from '.'
    at /mnt/…/nodejs/modules/resolve/lib/async.js:50:31
    at processDirs (/mnt/…/nodejs/modules/resolve/lib/async.js:184:39)
    at ondir (/mnt/…/nodejs/modules/resolve/lib/async.js:199:13)
    at load (/mnt/…/nodejs/modules/resolve/lib/async.js:82:43)
    at onex (/mnt/…/nodejs/modules/resolve/lib/async.js:107:17)
    at /mnt/…/nodejs/modules/resolve/lib/async.js:12:69
    at FSReqWrap.oncomplete (fs.js:152:21) code: 'MODULE_NOT_FOUND' }

So require.resolve was able to find it, but resolve wasn't. However, it works when I create a symlink /mnt/…/nodejs/node_modules to modules.

Update: Work-around: It also works when I pass option { paths: [process.env.HOME + "/.node_modules"] }
Update 2: However, in that case I have to then explicitly resolve the returned path in order to get the exact same result.

@ljharb
Copy link
Member

ljharb commented Jun 2, 2018

I think you'll need to set the preserveSymlinks option to true.

@mk-pmb
Copy link
Author

mk-pmb commented Jun 3, 2018

Nope, doesn't seem to help:

$ nodejs -e 'console.log(require.resolve("lodash")); require("resolve")("lodash", { preserveSymlinks: true }, console.log);' 
/mnt/…/nodejs/modules/lodash/lodash.js
{ Error: Cannot find module 'lodash' from '.'
[…]

@mk-pmb
Copy link
Author

mk-pmb commented Jun 3, 2018

To clarify, with "explicitly resolve the returned path" I meant I have to fs.realpath() it myself.

@ljharb
Copy link
Member

ljharb commented Jun 17, 2018

(Possibly related to #122)

@safizn
Copy link

safizn commented Aug 26, 2018

preserveSymlinks doesn't seem to work on Windows Symbolic link or Junction. Is there a problem with the implementation in the module ?
It never resolves to the real path, which what I needed from this module anyway. But the options preserveSymlinks: false, should work.
There seems to be a bug.

@ljharb
Copy link
Member

ljharb commented Aug 26, 2018

@myuseringithub does node itself work with windows symlinks?

@mk-pmb
Copy link
Author

mk-pmb commented Aug 26, 2018

I reproduced the problem and took an strace capture. The diff shows what really happened vs. what I'd expect to happen:

-stat64("/mnt/…/nodejs/modules/node_modules/lodash/package.json", …
+stat64("/mnt/…/nodejs/modules/lodash/package.json", …
 stat64("/mnt/…/nodejs/node_modules/lodash/package.json", …
 stat64("/mnt/…/node_modules/lodash/package.json", …
 stat64("/mnt/node_modules/lodash/package.json", …
 stat64("/node_modules/lodash/package.json", …

It seems resolve searches for modules in module directories inside of module search path directories, but not inside the module search path directories themselves.

@safizn
Copy link

safizn commented Aug 27, 2018

@ljharb Yes, I use symbolic link of type Junction, as it allows me to create symlinks that are supported between the host machine (Windows OS), and the Docker container (Linux OS) which I'm running the app on. In my setup both the host machine and container use JS scripts to run the project.
Node's require.resolve('<a symlink>') returns the real path of the symlink as Node's command option --reserve-symlink is set to false by default.

@ljharb
Copy link
Member

ljharb commented Jan 3, 2022

I'm unclear on what needs to be done here. Could anyone provide a repro repo, or better, a failing test case?

@mk-pmb
Copy link
Author

mk-pmb commented Jan 9, 2022

Bug demo in GitLab Actions: https://github.com/mk-pmb/browserify-resolve-symlink-163

@ljharb
Copy link
Member

ljharb commented Jan 11, 2022

@mk-pmb thanks - is there any way to repro it on the command line? When i run npm test I get:

> browserify-resolve-symlink-163@0.0.1 test
> bash demo.sh

readlink: illegal option -- m
usage: readlink [-n] [file ...]
demo.sh: line 11: cd: /prepare: No such file or directory

@ljharb
Copy link
Member

ljharb commented Jan 11, 2022

@mk-pmb ah! resolve 1 has preserveSymlinks: true set by default. resolve 2 has preserveSymlinks: false set by default.

Try setting preserveSymlinks: false explicitly (or try a resolve 2 prerelease).

@mk-pmb
Copy link
Author

mk-pmb commented Jan 11, 2022

is there any way to repro it on the command line?

Yes, if you run demo.sh in an environment similar enough to the GitHub Actions env.

readlink: illegal option -- m

You'll need GNU coreutils 8.21 or later installed. Ubuntu ships it by default, so I use that in the GitHub Actions.

Try setting preserveSymlinks: false explicitly (or try a resolve 2 prerelease).

Done in the no-preserve-symlinks branch, results are in.

@ljharb
Copy link
Member

ljharb commented Jan 11, 2022

pify?
41    node.js resolver: /…/browserify-resolve-symlink-163/mnt/projects/webdev/js/modules/pify/index.js
42    "resolve" module: /…/browserify-resolve-symlink-163/mnt/projects/webdev/js/modules/pify/index.js

Seems like it's good! Does that mean this can be closed?

@mk-pmb
Copy link
Author

mk-pmb commented Jan 11, 2022

The first two tests are the part that had been easy to fix ever since. The bug in my opening post is reproduced in the 3rd test case, and it still fails:

global-module-163?
    node.js resolver: /…/browserify-resolve-symlink-163/mnt/projects/webdev/js/secret_modules/global-module-163/dummy.js
    "resolve" module: Error: Cannot find module 'global-module-163' from '/home/runner/work/browserify-resolve-symlink-163/browserify-resolve-symlink-163/home/demo/my-cool-app'

@mk-pmb
Copy link
Author

mk-pmb commented Jan 11, 2022

I see now why you thought it was about the pify path. I'll clarify the tutorial in the demo readme.

@ljharb
Copy link
Member

ljharb commented Jan 12, 2022

the words "global module" concern me a bit; global modules aren't requireable or resolveable (unless you use the deprecated NODE_PATHS env var, which this package intentionally doesn't support and likely never will). Can you elaborate on that one?

@mk-pmb
Copy link
Author

mk-pmb commented Jan 12, 2022

I'm not sure if my naming of "global" in the example is correct. I'm also not up-to-date on the deprecation. If you can determine the cause and conclude that the difference in the 3rd example is out of project scope, we shall instead check whether the readme could use better info about why.

@mk-pmb
Copy link
Author

mk-pmb commented Jan 12, 2022

However, what I can provide, is to say I didn't write anything about NODE_PATHS in the example repo. Also git grep -nFe NODE_PATH came back empty, so it hasn't crept in.

@ljharb
Copy link
Member

ljharb commented Jan 12, 2022

In the no-preserve-symlinks, I'm able to cd into home/demo/my-cool-app and run node app.js, and require.resolve itself can't find global-module-163 - which makes sense, since there's no node_modules/global-module-163 present, in that directory or any above it. What do you expect will happen?

@mk-pmb
Copy link
Author

mk-pmb commented Jan 15, 2022

I expect it will consider ~/.node_modules at some stage, probably after ~/node_modules.

@ljharb
Copy link
Member

ljharb commented Jan 15, 2022

@mk-pmb it doesn't and shouldn't, no - .node_modules is never a thing.

Either way, in your repro repo there's no global-module-163 anywhere, so it's supposed to fail to resolve, best I can tell.

@mk-pmb
Copy link
Author

mk-pmb commented Jan 15, 2022

What do you mean by ".node_modules is never a thing."? It's still in current node master: lib/internal/modules/cjs/loader.js

  if (homeDir) {
    ArrayPrototypeUnshift(paths, path.resolve(homeDir, '.node_libraries'));
    ArrayPrototypeUnshift(paths, path.resolve(homeDir, '.node_modules'));
  }

Which explains why on GitHub Actions, node.js can successfully resolve all examples. (See logs, or example output in readme).

global-module-163?
    node.js resolver: /…/browserify-resolve-symlink-163/mnt/projects/webdev/js/secret_modules/global-module-163/dummy.js

Either way, in your repro repo there's no global-module-163 anywhere,

The resolving chain is (all inside repo):

~/.node_modules = home/demo/.node_modules = symlink to ../../mnt/projects/webdev/js/modules
mnt/projects/webdev/js/modules contains symlink global-module-163 to ../secret_modules/global-module-163
mnt/projects/webdev/js/secret_modules/global-module-163 contains package.json

@ljharb
Copy link
Member

ljharb commented Jan 15, 2022

whoa, that was not a thing I was aware of.

resolve has no knowledge of those paths and isn't set up to use them, and I'm not sure why anybody else would. $HOME shouldn't have any node_modules.

@ljharb
Copy link
Member

ljharb commented Jan 15, 2022

In other words, this issue has nothing to do with symlinks - it's about some kind of special $HOME/.node_modules path that nobody's ever mentioned in a decade of this package not supporting it :-) I'll update the title to reflect that.

(note that "global" means npm root -g; this home dir thing is something else)

@ljharb ljharb changed the title Doesn't find modules behind symlinks? doesn't support $HOME/.node_modules Jan 15, 2022
@ljharb
Copy link
Member

ljharb commented Jan 15, 2022

For history's sake: nodejs/node@5a49f96#diff-cdd77182e73349d6c83fbbce1cccf6f15972fbece4dbd9c52eececf1ca3bfcd5R307-R308 is in every node version since basically ever.

@ljharb ljharb changed the title doesn't support $HOME/.node_modules doesn't support $HOME/.node_modules or $HOME/.node_libraries Jan 15, 2022
@mk-pmb
Copy link
Author

mk-pmb commented Jan 16, 2022

I updated my test project;

  • Fix the naming of the module to be resolved via ~/.node_modules
  • Add test for ~/.node_libraries
  • Add test for an actual global module

Update: Today I learned that my global modules being require()-able system-wide is just accidential. The node design concept seems to understand npm --global as related only to modules' CLI commands, not the modules themselves.

@ljharb
Copy link
Member

ljharb commented Jan 16, 2022

indeed; global things aren’t supposed to be requireable.

@ljharb
Copy link
Member

ljharb commented Jan 18, 2022

The current workaround for this is to pass [$HOME/.node_libraries, $HOME/.node_modules] as the paths option.

ljharb added a commit that referenced this issue Jan 21, 2022
…s,libraries}`

Note, this is a rarely used feature that should be aggressively avoided, but it‘s important to minimize gaps between node and this package.

Fixes #163
@ljharb
Copy link
Member

ljharb commented Jan 21, 2022

@mk-pmb can you confirm that #273 works for you? (or the equivalent change on 1.x)

ljharb added a commit that referenced this issue Jan 21, 2022
…s,libraries}`

Note, this is a rarely used feature that should be aggressively avoided, but it‘s important to minimize gaps between node and this package.

Fixes #163
ljharb added a commit that referenced this issue Jan 21, 2022
…s,libraries}`

Note, this is a rarely used feature that should be aggressively avoided, but it‘s important to minimize gaps between node and this package.

Fixes #163
@ljharb ljharb closed this as completed in 66066fd Jan 22, 2022
ljharb added a commit that referenced this issue Jan 22, 2022
…s,libraries}` (#273)

Note, this is a rarely used feature that should be aggressively avoided, but it‘s important to minimize gaps between node and this package.

Fixes #163
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants