Resolve doesn't resolve real path for basedir if it is relative path

[malash]

resolve doesn't resolve real path for basedir if it is relative path.

I'v created a bad case example ( by lerna ) here https://github.com/malash/resolve-bad-case

The issue is related to #130 .
The issue effected peerigon/extract-loader#48 and may affect #163 .

@zkochan @ljharb

[ljharb]

It seems like you're not setting the preserveSymlinks option to false, (it currently defaults to true in v1, altho master has the breaking change of defaulting it to false).

What happens if you set that option?

[malash]

I update the test case and the bug still exists.

malash/resolve-issue-177@c5ea1c8

In fact preserveSymlinks isn't used at all in https://github.com/browserify/resolve/blob/master/lib/sync.js#L49-L52

[ljharb]

hmm, i'm trying to reproduce this error, and i do get this failure:
Cannot find module '../../node_modules/jquery' from './node_modules/@my-scope/package-b'

However, after npm install and after npx lerna bootstrap, node_modules still lacks a @my-scope directory, so i'm not sure why you'd expect something to be resolved there.

[malash]

@ljharb I will clarify it.

When running node packages/package-a

it equals to

cd packages/package-a && node ./index.js

so the basedir would be

the-repo/packages/package-a/node_modules/@my-scope/package-b

which is a symlink to the-repo/packages/package-b.

There is a real issue when I use webpack with extract-loader peerigon/extract-loader#48 (comment)

[ljharb]

OK, but the error says "from node_modules/@my-scope/package-b" - and then "../../node_modules/jquery'" - so, the final path it's looking for is "node_modules/jquery" (inside package-a) - but package-a doesn't have jquery listed as a dependency, and it's not present on disk.

[ljharb]

(and package-b doesn't have jquery either)

[malash]

No. The final path is not 'node_modules/jquery' but 'packages/package-a/node_modules/node_modules/jquery' and it's wrong even if package-a had jquery.

---

First, I fixed the basedir in test case with abs path so the error trace is more clear.

/Users/malash/Projects/resolve-bad-case/node_modules/jquery/dist/jquery.js
/Users/malash/Projects/resolve-bad-case/node_modules/resolve/lib/sync.js:46
    throw err;
    ^

Error: Cannot find module '../../node_modules/jquery' from '/Users/malash/Projects/resolve-bad-case/packages/package-a/node_modules/@my-scope/package-b'
    at Function.module.exports [as sync] (/Users/malash/Projects/resolve-bad-case/node_modules/resolve/lib/sync.js:44:15)
    at Object.<anonymous> (/Users/malash/Projects/resolve-bad-case/packages/package-a/index.js:8:21)
    at Module._compile (internal/modules/cjs/loader.js:707:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:718:10)
    at Module.load (internal/modules/cjs/loader.js:605:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
    at Function.Module._load (internal/modules/cjs/loader.js:536:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
    at startup (internal/bootstrap/node.js:303:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:872:3)

---

The current logic equals to path.resolve because of this line :

'/path/to/repo/packages/package-a/node_modules/@my-scope/package-b'
+
'../../node_modules/jquery'
=>
'/path/to/repo/packages/package-a/node_modules/node_modules/jquery'

It's a non-exist path.

The correct way should be:

1. Get the real path of basedir. E.g.

'/path/to/repo/packages/package-a/node_modules/@my-scope/package-b' =>
'/path/to/repo/packages/package-b/'

2. Resolve the path.

'/path/to/repo/packages/package-b/' + '../../node_modules/jquery' =>
/path/to/repo/node_modules/jquery

That line could be refactor like this ( just for demo ):

// if not preserveSymlinks
path.resolve(fs.realpathSync(basedir), x)
// else
path.resolve(basedir, x)

fs.realpathSync will handle the basedir if it is a symlink ( even inside a symlink ).

[ljharb]

Got it, thanks for explaining.

[ljharb]

OK, so part of the problem is that the basedir you're passing in isn't resolved - it's relative to package-a/index, but that's not information that resolve can retain - so you first have to path.resolve(__dirname, './node_modules/@my-scope/package-b').

This makes the first assertion pass, but the second still fails for me.

[malash]

Normally the basedir should be absolute path. It's my fault to pass a relative path for basedir in the first version test case so I fixed it. That could be another good question how relative basedir resolved.

For this issue the inconsistency is that user have to resolve real path of basedir for relative path but not NPM package.

// packages/package-a/index.js
console.log(resolve.sync("jquery", { basedir: basedir }));
console.log(resolve.sync("../../node_modules/jquery", { basedir: fs.realpathSync(basedir) }));

⬆️These lines worked for me.

[malash]

Compression between preserveSymlinks === true and preserveSymlinks === false

// preserveSymlinks === false
// will search NPM package from
// * packages/package-b/node_modules
// * packages/node_modules
// * node_modules
console.log(resolve.sync('jquery', { basedir: basedir, preserveSymlinks: false }));
console.log(resolve.sync('../../node_modules/jquery', { basedir: fs.realpathSync(basedir), preserveSymlinks: false }));

// preserveSymlinks === true
// will search NPM package from
// * packages/package-a/node_modules/@my-scope/packages/package-b/node_modules
// * packages/package-a/node_modules/@my-scope/packages/node_modules
// * packages/package-a/node_modules/@my-scope/node_modules
// * packages/package-a/node_modules/node_modules
// * packages/package-a/node_modules
// * packages/node_modules
// * node_modules
console.log(resolve.sync('jquery', { basedir: basedir, preserveSymlinks: true }));
console.log(resolve.sync('../../../../../node_modules/jquery', { basedir: basedir, preserveSymlinks: true }));

[malash]

@ljharb Thank you very much for your commit. This commit works for me.

[ljharb]

Thank you for your excellent repro case!

[john-kurkowski]

It seems like you're not setting the preserveSymlinks option to false, (it currently defaults to true in v1, altho master has the breaking change of defaulting it to false).

Did this breaking change get resolved before publishing v1.9.0?

master (04e6c79) has a different check

resolve/lib/async.js

Line 57 in 04e6c79

if (!opts || !opts.preserveSymlinks) {

vs. what I think was published at tag v1.9.0.

https://github.com/browserify/resolve/blob/v1.9.0/lib/sync.js#L36

[ljharb]

@john-kurkowski v1.9.0 does not include any of the breaking changes. I cherry-picked commits onto the v1.8.1 tag, and released, and merged the v1.9.0 tag back to master.

[simonhaenisch]

@ljharb what are the plans on releasing this fix? Having issues with this in stenciljs/core#1310.

rollup-plugin-node-resolve@3.4.0 uses resolve@1.1.7 and rollup-plugin-node-resolve@4.0.0 uses resolve@1.9.0.

Problem is that with path ./components/cmp-a/cmp-a.js and { basedir: '/src' } , resolve@1.1.7 gives an error through the callback

`Error: Cannot find module './components/cmp-a/cmp-a.js' from '/src'

, but resolve@1.9.0 does the fs.realpath check and then calls init(undefined) which throws

TypeError: Path must be a string. Received undefined

and makes the code fail.

When I replaced init(realStart) with init(realStart || absoluteStart), it was fixed... but I guess the problem could also be that the base dir is an absolute path /src even though that is meant as a relative path ./src in this case (adding if (basedir === '/src') basedir = './src'; also fixed it).

I'm not that familiar with the topic so what would you suggest? Is this more related to bad code that needs to be fixed, or should I wait for the fix?

[ljharb]

@simonhaenisch it’s released in v1.9.0. If you’re still seeing a problem, please file a new issue and we can talk it out there.