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

Use the real path for resolving dependencies #130

Closed
zkochan opened this issue Jul 24, 2017 · 4 comments
Closed

Use the real path for resolving dependencies #130

zkochan opened this issue Jul 24, 2017 · 4 comments

Comments

@zkochan
Copy link
Contributor

zkochan commented Jul 24, 2017

Node uses the real path of modules (except when executed with --preserve-symlinks) to resolve dependencies. That is why pnpm's symlinked node_modules work fine with Node applications.

However, resolve is a really popular and widely used package and it does not use the real path of modules to resolve dependencies. As a consequence, a lot of tooling fails with pnpm. See the most recent issue: pnpm/pnpm#857. And also many of these might be related: pnpm/pnpm#801

Would you accept a pull request to change the behavior of resolve to work the way Node does? It won't change anything for projects that use npm/Yarn because they don't leverage symlinks but it would be a huge help for the pnpm community!

@ljharb
Copy link
Member

ljharb commented Jul 24, 2017

Sure, that sounds good - let's ensure that a) existing tests keep passing (ofc), b) we add a "preserve symlinks" option, so users can control the behavior if they like, and c) let's think carefully about if it's possible to avoid a semver-major.

zkochan added a commit to zkochan/node-resolve that referenced this issue Jul 25, 2017
BREAKING CHANGE:

resolve from real path by default

Close browserify#130
zkochan added a commit to zkochan/node-resolve that referenced this issue Jul 25, 2017
BREAKING CHANGE:

resolve from real path by default

Close browserify#130
ljharb pushed a commit to zkochan/node-resolve that referenced this issue Jul 26, 2017
BREAKING CHANGE:

resolve from real path by default

Close browserify#130
zkochan added a commit to zkochan/node-resolve that referenced this issue Jul 26, 2017
Add the possibilty to make `resolve` use real paths
during resolution

Close browserify#130
zkochan added a commit to zkochan/node-resolve that referenced this issue Jul 26, 2017
Add the possibilty to make `resolve` use real paths
during resolution

Close browserify#130
@ljharb ljharb closed this as completed in fed3f0f Jul 26, 2017
ljharb added a commit that referenced this issue Jul 26, 2017
 - [New]: add `preserveSymlinks` option (#130)
 - [Fix] `sync`: fix when package.json main = ‘.’ or main = ‘./‘ (#125)
 - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `tape`
 - [Tests] up to `node` `v8.2`, `v7.10`, `v6.11`; npm 4.6+ breaks on node < 4
 - [Tests] fix 0.6 and linting
@malash
Copy link

malash commented Nov 26, 2018

@zkochan Did you forget to change this condition when resolving real path for basedir ?

var res = path.resolve(y, x);

E.g. this line may fail require('resolve').sync('../../path/to/another/package', { basedir: '/path/to/a/symlink' }).

FYI @ljharb

@ljharb
Copy link
Member

ljharb commented Nov 26, 2018

@malash could you file a separate issue for that, with a test case?

@malash
Copy link

malash commented Nov 27, 2018

I'v create a new issue with bad case example #177

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

No branches or pull requests

3 participants