module: CJS handle deleted directory relative require

[bmeck]

This makes sure the following code works:

const fs = require('fs');
const path = require('path');
const dir = path.join(__dirname, 'delete_me');
fs.mkdirSync(dir, {
    recursive: true
});
const barPath = path.join(dir, 'bar.cjs');
fs.writeFileSync(barPath, `
    module.exports = () => require('../foo.cjs')
`);
const bar = require(barPath);
fs.rmSync(dir, {recursive: true});
require('assert').strict.equal(bar(), exports);

I found this while fixing up some policy stuff and realizing baseURL wouldn't work if synchronized to disk exactly right for things like, https://github.com/bmeck/local-fs-https-imports , due to an over eager optimization in the code.

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[bmeck]

we could also not use normalized form in the further parts of the PR if people are worried about compat

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ljharb]

Is there a reason to use charcode instead of:

Suggested change

	(StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_FORWARD_SLASH || (
	(request[request.length - 1] === '/' || (

?

[anonrig]

I believe charcode comparison is faster

[ljharb]

in recent v8? I’d love to see benchmarks for that!

[aduh95]

Same here, I'd be up for landing this as is, and open a new PR to run benchmarks and see if that changes something.

[ljharb]

Totally agree, perf changes should be done separately with benchmarks - i was thinking the simple string comparison was simpler than the charCode, and thus the better choice absent said benchmarks.

[anonrig]

I believe storing request.length in a separate variable will result in faster code execution since the next 30~40 lines call request.length

[ljharb]

Historically caching length lookups caused a huge perf hit, since it defeats a JIT optimization - not sure if that’s still true, but i doubt caching the length can beat the JIT.

[aduh95]

I'd be up for landing this as is, and open a new PR to run benchmarks and see if that changes something.

[nodejs-github-bot]

Landed in ccd81e8