Review requested:

• @nodejs/loaders

Please always include tests in a PR. The test should fail on current main and pass in your branch.

Hi! I'm about to push tests that will fail in the current version of node, but not in my patch.

This change seems like it would be a semver major breaking change. It's not clear to me that the current behavior is buggy; is there documentation that describes the intended behavior, where a test can confirm that Node behaves not in accordance with that?

Consider the directory setup below (assuming your current working directory is ./app):

.
├── app.js
└── app
    └── index.js

In the latest version of NodeJS, executing node . will launch app.js. However, in the REPL, calling require('.') will execute ./app/index.js.

In the latest version of NodeJS, executing node . will launch app.js.

I believe this is the correct behaviour.

In the latest version of NodeJS, executing node . will launch app.js.

I believe this is the correct behaviour.

IMO it's not, as one would assume:
node . -> node ./index.js
instead of
node . -> node ../(cwd).js

IMO it’s not, as one would assume

The relevant documentation is at https://nodejs.org/api/cli.html#program-entry-point:

The program entry point is a specifier-like string. If the string is not an absolute path, it’s resolved as a relative path from the current working directory. That path is then resolved by CommonJS module loader, or by the ES module loader if --experimental-default-type=module is passed. If no corresponding file is found, an error is thrown.

So reading this literally, an entry point string of . is not an absolute path, so it’s resolved as a relative path from the current working directory. Resolving /path/to/app/. from a relative path to an absolute path gets you /path/to/app; resolving that as a CommonJS specifier gets you /path/to/app.js (assuming ../app.js exists). Nonintuitive, but correct, per the documentation.

You might argue that this is nonsensical, that no one would want this behavior, and I would agree with that. In which case it’s not a bugfix but just a change, and the documentation needs to be updated along with the behavior. I would argue, though, that if we’re going to make a semver-major change to how the resolution of the entry point happens, we might want to go a bit bigger and eliminate the CommonJS resolution from this entirely: if you want to run ./index.js, then you need to run node ./index.js or node index.js and not rely on . getting resolved to index.js. But that’s a much larger breaking change and should probably be proposed in its own issue to gauge how people feel about it.

True, with all of the changes that could be made around this functionality, you'd probably right that a discussion with @nodejs/loaders (or the relevant team).