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

require.resolve with paths and non-existent directory returns incorrect local file when using ./ #18408

Closed
BrainBacon opened this issue Jan 27, 2018 · 10 comments · Fixed by #23683

Comments

@BrainBacon
Copy link

  • Version: 9.3.0
  • Platform: Linux 4.9.77 x86_64 GNU/Linux
  • Subsystem: require.resolve

To replicate create a directory with the following files:
test.js
index.js
exists/index.js

both of the index.js files can contain anything. They only have to exist.

test.js should contain the following:

'use strict';

let assert = require('assert');

const OPTS = { paths: ['./imaginary', './exists'] };
const FILE = 'index.js';

let bare = require.resolve(FILE, OPTS);
let dot = require.resolve(`./${FILE}`, OPTS);
console.info(`bare: ${bare}`);
console.info(`dot: ${dot}`);
assert.strictEqual(bare, dot);

Execute the test.js file with node test.js on v9.3.0 or later.

As you can see from the code in the test, simply adding ./ to the file name to be resolved causes a local matching file to be returned.

Specifically:

  • When an earlier path in the paths array doesn't exist
  • Even though there is a matching file in a path that does exist later in the paths array.

This bug was introduced in v9.3.0. v9.2.1 and prior behave correctly.

BrainBacon added a commit to Beg-in/build that referenced this issue Jan 27, 2018
@cjihrig
Copy link
Contributor

cjihrig commented Jan 27, 2018

I think the fix for this would be cjihrig@63beb8c.

It seems to be a matter of whether or not the provided paths are searched, or if they are just used as a starting point for the resolution. In your example, I think bare should throw, because running require('index.js') from the exists directory would throw.

@BrainBacon
Copy link
Author

If that were the case wouldn't one have to try/catch every directory in the paths array individually if the directories could be missing? Personally I'm using require.resolve in this case to find where an entry point is among multiple possible source directories. I thought that was what require.resolve with the paths option was specifically for since it only returns one file.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 27, 2018

If that were the case wouldn't one have to try/catch every directory in the paths array individually if the directories could be missing?

No. An exception is only thrown if no match is found after trying all paths. This is how require() and require.resolve() work. In your original example, the non-existent ./imaginary path doesn't change anything.

Personally I'm using require.resolve in this case to find where an entry point is among multiple possible source directories. I thought that was what require.resolve with the paths option was specifically for since it only returns one file.

require.resolve() acts like require(), but returns the resolved path, instead of the actual module. The paths argument is supposed to be equivalent to calling require.resolve() from each of the provided paths. The issue you're running into is that each of the provided paths is ALSO searched. cjihrig@63beb8c undoes that behavior.

@BrainBacon
Copy link
Author

Oh, excellent. Thank you for the clarification.

@gireeshpunathil
Copy link
Member

Is this resolved, and a close candidate?

@cjihrig
Copy link
Contributor

cjihrig commented Apr 10, 2018

The behavior remains. I'm not sure if the existing behavior is considered a bug or not. Personally, I think it's a bug. I suppose I could PR cjihrig@63beb8c.

@niksajanjic
Copy link

As explained in here I feel this behavior should be considered a bug. If you try to do require.resolve from folder A passing folder B to its paths option, you would expect it to behave same as if you called require.resolve inside folder B without paths option. That is not the case in the current situation.

Calling require.resolve('./') inside folder B will resolve to path/to/B/index.js if it exists or error out with MODULE_NOT_FOUND.

If you call require.resolve('./', { paths: ['path/to/B'] }) inside folder A it will resolve to path/to/B/index.js if it exists, if not then to root/index.js if it exist and only then error out if both files are not found. That was unexpected behavior for me. Also, there is no mentioning of this behavior in documentation unless the root folder is considered to be part of GLOBAL_FOLDERS. But, even if it is, then why it's not included when calling require.resolve without paths option and be resolved in both cases equally.

Due to this behavior I have to consider paths options unreliable and fallback to using path.resolve('path/to/B', './') and have my own custom logic to parse the request and figure out if it's relative or absolute. Also, while at this, I feel options should be added to require.resolve.paths so we can get resolvement paths when we provide paths option.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 15, 2018

I'll open the PR a little bit later today. It will probably have to be a semver major change though.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 16, 2018

@niksajanjic can you try building Node with #23683 and see if that fixes your issue?

@niksajanjic
Copy link

I'll give it a shot next day or 2 and get back to you.

cjihrig added a commit to cjihrig/node that referenced this issue Dec 11, 2018
The paths used by require.resolve() should be treated as
starting points for module resolution, and not actually
searched.

PR-URL: nodejs#23683
Fixes: nodejs#18408
Refs: nodejs#23643
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
The paths used by require.resolve() should be treated as
starting points for module resolution, and not actually
searched.

PR-URL: nodejs#23683
Fixes: nodejs#18408
Refs: nodejs#23643
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants