module: allow passing a directory to createRequireFromPath

[gillesdemey]

Fixes: #23710

I still need to update the documentation for this change, I'm not quite sure if we should update the documentation to encourage users to pass a directory or do we prefer a file path?

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/18730/

[guybedford]

CI failure seems to be unrelated as far as I can tell, but perhaps someone can double check.

[gillesdemey]

Seems like a flaky streams test, from the CI output:

02:54:38 not ok 410 parallel/test-stream-pipeline
02:54:38 ---
02:54:38 duration_ms: 120.94
02:54:38 severity: fail
02:54:38 exitcode: 1
02:54:38 stack: |-
02:54:38 timeout
02:54:38 ...

[Trott]

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/18906/

[Trott]

This affects CJS modules so I want to make sure it gets sufficient review, so.... @nodejs/collaborators (Maybe we should have a CJS modules team?)

[guybedford]

Note it is important to merge this or the docs change as effectively the documentation is incorrect until then!

[mcollina]

I'd expect both of this to pass. fixtures is a directory, so they should both work IMHO. This probably requires adding a round-trip to OS, but I don't think it could be avoided.

[guybedford]

That would be a very different approach to this function, but it could work. //cc @devsnek

[gillesdemey]

I'm happy to change the implementation to call stat.isDirectory() if that's the way we want to move this forward :)

[guybedford]

If we want to get this merged, I'd suggest we stick with the current behaviour which matches the behaviour of path resolution.

[MylesBorins]

+1 to sticking to behavior that matches path... we should change this behavior in the path module if we want to fix it imho

[BridgeAR]

I personally think calling stat is a good idea in this case. createRequireFromPath is IMO special enough to work differently from the rest of Module.

[MylesBorins]

I would like to land this to improve the behavior and introduce the stat separately in a follow up. Is that reasonable for you @BridgeAR ?

[BridgeAR]

This has three different open comments at the moment (actually four but two are about using stat). I personally would not land this but I won't block it either.

[MylesBorins]

@BridgeAR afaict all open comments are about stat, which I think should be an incremental improvement discussed separately.

Did I miss any other open comments?

[MylesBorins]

LGTM

[MylesBorins]

went ahead and rebased against master and added a very minor doc fix to at the very least get the docs working.

@Trott I've dropped the semver-minor label on this as I believe it should be considered a bug fix. The docs at the very least imply that being able to pass a directory to createRequireFromPath was expected behavior.

The function is not used anywhere in core so AFAIK I don't think we need to consider it a change to the way CJS modules work.

@mcollina do you have any objections to this landing?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/22714/

[BridgeAR]

Should we maybe just call stat to verify if it's a file or directory? That way it's even safer and works without a trailing slash.

[MylesBorins]

I think the issue isn't being different from module it is being different from how Path works... I'm not 100% we should be adding the stat here

[BridgeAR]

I personally think calling stat is a good idea in this case. createRequireFromPath is IMO special enough to work differently from the rest of Module.

[MylesBorins]

@targos why did you add back the semver-minor label? I don't believe this is a minor as lack of directory support is a bug, original docs implied feature was intended

[targos]

Sorry, I was just passing through PRs without any label. I didn't see it was added and removed.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/22840/

[MylesBorins]

rebased PR and added changes based on review. PTAL

[MylesBorins]

only failure is infra failure on AIX... will land in 24 hours if there are no objections

[BridgeAR]

LGTM. It would be nice to get a follow-up PR that implements the suggested stat call.

[addaleax]

LGTM – I don’t think a stat call necessarily makes sense, or at least I’d like to see an opt-out switch of that, because it’s only a help to avoid accidentally passing in non-directories, not something that actually influences how the feature works.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/22871/

[BridgeAR]

@addaleax

I don’t think a stat call necessarily makes sense, or at least I’d like to see an opt-out switch of that, because it’s only a help to avoid accidentally passing in non-directories, not something that actually influences how the feature works.

I would not use an opt-out but up to two stat calls: one for the filename as it is and if that fails one for path.dirname(filename).

[addaleax]

@BridgeAR I think it’s a bad idea because I think the whole idea of tying the module system to the actual filesystem breaks orthogonality and is consequentially a design flaw; but that’s a bigger discussion.

[MylesBorins]

landed in e5c8be2