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

resolve result is inconsistent with require.resolve if windows virtual drive is used #283

Closed
pos777 opened this issue May 20, 2022 · 9 comments

Comments

@pos777
Copy link
Contributor

pos777 commented May 20, 2022

I'm researching the rollup/rollup#4260 issue and found that the resolve and the require.resolve results differ in windows virtual drive (subst).

To reproduce run the following index.js script:

const resolve = require('resolve');

console.log('require.resolve: ' + require.resolve('./index.js'));
console.log('resolve.sync:    ' + resolve.sync('./index.js', { basedir: __dirname, preserveSymlinks: false }));
resolve('./index.js', { basedir: __dirname, preserveSymlinks: false }, (err, res) => {
    if (err) console.error(err);
    else console.log('resolve:         ' + res);
});

in the following way:

> npm install resolve
> subst X: .
> X:
> node index.js

Output:

require.resolve: X:\index.js
resolve.sync:    C:\repo\index.js
resolve:         C:\repo\index.js

image

@ljharb
Copy link
Member

ljharb commented May 20, 2022

Which version of resolve are you using? v1 is "latest", but there's a v2 prerelease that's worth trying as well.

It's possible that the preserveSymlinks option (which v2 changes the default of) is the difference you're seeing.

@pos777
Copy link
Contributor Author

pos777 commented May 20, 2022

The result is the same for v1.22.0 and v2.0.0-next.3.
The preserveSymlinks option is set explicitly in script to ensure that it is false.
I have added the index.js.link file to the test (via mklink). Result for it is inconsistent too:

v1.22.0

X:\>node index.js

require.resolve: X:\index.js
resolve.sync:    C:\repo\index.js
resolve:         C:\repo\index.js

require.resolve (link): X:\index.js
resolve.sync (link):    C:\repo\index.js
resolve (link):         C:\repo\index.js.link

X:\>npm ls resolve
null@ X:\
`-- [email protected]

image

v2.0.0-next.3

X:\>node index.js

require.resolve: X:\index.js
resolve.sync:    C:\repo\index.js
resolve:         C:\repo\index.js

require.resolve (link): X:\index.js
resolve.sync (link):    C:\repo\index.js
resolve (link):         C:\repo\index.js.link

X:\>npm ls resolve
null@ X:\
`-- [email protected]

image

@pos777
Copy link
Contributor Author

pos777 commented May 21, 2022

It seems that the fs.realpathSync.native and the fs.realpath.native functions cause the behavior:

var realpathFS = fs.realpathSync && typeof fs.realpathSync.native === 'function' ? fs.realpathSync.native : fs.realpathSync;

var realpathFS = fs.realpath && typeof fs.realpath.native === 'function' ? fs.realpath.native : fs.realpath;

Node's loader uses the fs.realpathSync if I look at correct place:

https://github.com/nodejs/node/blob/26846a05e2ac232742e6a0bfaa7baac5e86a015b/lib/internal/modules/cjs/loader.js#L394-L398

In the test scenario these functions give the following result:

X:\>node index.js

fs.realpathSync (./index.js):        X:\index.js
fs.realpathSync.native (./index.js): C:\repo\index.js
fs.realpath (./index.js):            X:\index.js
fs.realpath.native (./index.js):     C:\repo\index.js

As I see native functions were introduced to improve performance (#217).
If there is not another reason and there are performance tests I can compare realpath vs realpath.native.

@ljharb
Copy link
Member

ljharb commented May 21, 2022

If the native function gives the wrong answer on windows, then we could certainly avoid using it only on windows.

@ljharb
Copy link
Member

ljharb commented May 31, 2022

Fixed by #284.

@ljharb ljharb closed this as completed May 31, 2022
@pos777
Copy link
Contributor Author

pos777 commented Jun 2, 2022

@ljharb, thank you! Is it possible to publish a new patch (v1.22.1) with the fix?

@ljharb
Copy link
Member

ljharb commented Jun 2, 2022

I will do so this week.

@pos777
Copy link
Contributor Author

pos777 commented Jun 9, 2022

Hello @ljharb, any updates?

@ljharb
Copy link
Member

ljharb commented Jun 17, 2022

v1.22.1 is released.

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

2 participants