module: createRequireFromPath documentation is incorrect

[gillesdemey]

• Version: v10.12.0
• Platform: MacOS 10.14 (Darwin)
• Subsystem:

The documentation for createRequireFromPath makes it seem like you can pass a directory to the function, but that will never correctly resolve packages.

Using the following directory structure;

src/
└── utils
    └── some-tool.js

The following code will throw a MODULE_NOT_FOUND exception (example from the docs):

const { createRequireFromPath } = require('module');
createRequireFromPath('../src/utils')('some-tool');

However, if we pass a file path we can resolve it just fine.

const { createRequireFromPath } = require('module');
createRequireFromPath('../src/utils/index.js')('some-tool');

Is there a bug here or is the documentation wrong? I'm willing to make a PR to either fix the issue or update the documentation :)

[demurgos]

I don't believe the documentation is incorrect, but it could be improved.

Filename to be used to construct the relative require function.

Only a file can be a module. Your code is resolving from an extension-less file named utils in the src directory. You can't reliably use the path to differentiate directories from files (you can have a directory named utils.js, trailing separators are usually a source of bugs, etc.) and this function can be used with paths that do not exist on the FS.

It may be worth clarifying that a commonjs module id is the absolute system-dependent path to the corresponding file.

Edit: I missed the example below: I agree that it using a directory seems to be an error.

[targos]

The example in the documentation seems indeed wrong because it passes a directory to the function.

/cc @devsnek

[guybedford]

Yes it looks like the example should be changed to include a trailing separator:

const requireUtil = createRequireFromPath('../src/utils/');

[gillesdemey]

@guybedford It works when appending a trailing slash and a ., not when just appending a trailing slash.

const requireUtil = createRequireFromPath('../src/utils/.');

[guybedford]

@gillesdemey thanks for confirming. That sounds incorrect to me. I specially included logic to handle retaining trailing slashes in the pathToFileURL function.

[guybedford]

Ah, I get we're in path resolution and this is how Node path resolution works generally, which is a difference with how URLs behave.

[guybedford]

It would seem sensible to me to specifically add support for a trailing separator in this function though.

[gillesdemey]

I can submit a PR to support passing a directory?

The following change seems to do the trick :)

const searchPath = filename.endsWith('/') ?
    filename :
    path.dirname(filename);

  m.paths = Module._nodeModulePaths(searchPath);

[adeelibr]

Can I pretty please make a PR for this. This can be my first PR to NodeJS.

[guybedford]

I'd be glad to see either PR along these lines for the docs or the trailing separator behaviour.

[gillesdemey]

I've created a PR that will allow passing a directory, updating the documentation is still on my todo list 😄

[doertedev]

@gillesdemey 's that okay?

[creativecomposer]

Hi folks, stumbled upon this issue when searching for "good first issue" and "help wanted".

1. There is a PR to allow passing a directory to createRequireFromPath that is still open.

2. There is a doc PR that adds the comment // can also be a file to the example assuming that the aforementioned PR is landed.

If I understand correctly, the assumption made at (2) is incorrect.

Until the PR at (1) is accepted, the best option I believe is @Trott suggestion at #24763 (comment)

Shall I go ahead and make the doc change accordingly?

Just want to make sure my understanding is correct before creating a PR.

[guybedford]

A docs PR to align the docs would be really great and is much needed, thank you @antonyj75 . I'd suggest pushing on the directory PR if you can as well - it could still likely land as-is I think.

[creativecomposer]

@guybedford thank you for your response.
Since doc alignment is much needed and is easy, I went ahead and created a PR for that.

The directory PR seem to be stuck while @gillesdemey added a negative test case.
I will need a bit more time to figure out what is going on there.