docs: module resolution pseudocode corrections

[laverdet]

I found two three errors in the pseudocode for the resolution algorithm. Permalinks to the implementations which show the correct behavior:

node/lib/internal/modules/package_json_reader.js

Line 244 in 579fc67

let packageJSONUrl = new URL(`./node_modules/${packageName}/package.json`, base);

node/lib/internal/modules/esm/resolve.js

Lines 594 to 601 in 579fc67

	if (ObjectPrototypeHasOwnProperty(exports, packageSubpath) &&
	!StringPrototypeIncludes(packageSubpath, '*') &&
	!StringPrototypeEndsWith(packageSubpath, '/')) {
	const target = exports[packageSubpath];
	const resolveResult = resolvePackageTarget(
	packageJSONUrl, target, '', packageSubpath, base, false, false, false,
	conditions,
	);

node/lib/internal/modules/cjs/loader.js

Lines 1982 to 1990 in 6fe0723

	function isRelative(path) {
	if (StringPrototypeCharCodeAt(path, 0) !== CHAR_DOT) { return false; }
	return path.length === 1 || path === '..' ||
	StringPrototypeStartsWith(path, './') ||
	StringPrototypeStartsWith(path, '../') ||
	((isWindows && StringPrototypeStartsWith(path, '.\\')) ||
	StringPrototypeStartsWith(path, '..\\'));
	}

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[laverdet]

Added another correction to the CJS docs. Without this fix require(".") does not work but in reality it is equivalent to require("./index.js") || require("./index.json") || require("./index.node") (sans exceptions obviously).

See:

node/lib/internal/modules/cjs/loader.js

Lines 1982 to 1990 in 6fe0723

	function isRelative(path) {
	if (StringPrototypeCharCodeAt(path, 0) !== CHAR_DOT) { return false; }
	return path.length === 1 || path === '..' ||
	StringPrototypeStartsWith(path, './') ||
	StringPrototypeStartsWith(path, '../') ||
	((isWindows && StringPrototypeStartsWith(path, '.\\')) ||
	StringPrototypeStartsWith(path, '..\\'));
	}

[lpinca]

This needs a rebase to resolve conflicts.

[laverdet]

Rebased and tweaked the quad-"or" sentence a bit.

[guybedford]

Thanks for the careful work here! Would be great to get these in, down to some refinement that is probably needed for trailing slash handling.

[guybedford]

All the steps below this are based on the assumption that * is contained in the matchKey, so this restriction isn't quite right.

This is due to the deprecation path of trailing slash imports, which I believe are supposed to be resolution errors now before reaching this point? Would be worth verifying and updating accordingly.

[laverdet]

Thank you for the keen eye. I think you're correct that this is not the correct place for the check. This case is handled in the ESM pseudocode here:

node/doc/api/esm.md

Lines 872 to 873 in f6ce486

	> 8. If _packageSubpath_ ends in _"/"_, then
	> 1. Throw an _Invalid Module Specifier_ error.

CJS doesn't describe the same precondition, though:

node/doc/api/modules.md

Lines 431 to 443 in 579fc67

	LOAD_PACKAGE_EXPORTS(X, DIR)
	1. Try to interpret X as a combination of NAME and SUBPATH where the name
	may have a @scope/ prefix and the subpath begins with a slash (`/`).
	2. If X does not match this pattern or DIR/NAME/package.json is not a file,
	return.
	3. Parse DIR/NAME/package.json, and look for "exports" field.
	4. If "exports" is null or undefined, return.
	5. If `--experimental-require-module` is enabled
	a. let CONDITIONS = ["node", "require", "module-sync"]
	b. Else, let CONDITIONS = ["node", "require"]
	6. let MATCH = PACKAGE_EXPORTS_RESOLVE(pathToFileURL(DIR/NAME), "." + SUBPATH,
	`package.json` "exports", CONDITIONS) <a href="esm.md#resolver-algorithm-specification">defined in the ESM resolver</a>.
	7. RESOLVE_ESM_MATCH(MATCH)

So what I'm seeing is that the CJS pseudocode allows a deprecated trailing-slash require to be passed to PACKAGE_EXPORTS_RESOLVE, but the ESM pseudocode forbids the deprecated behavior before it reaches PACKAGE_EXPORTS_RESOLVE. Which behavior do you want to describe in the pseudocode? Deprecated, or only supported?

If we want the pseudocode to describe only supported non-deprecated behavior then my sense is that "If packageSubpath ends in "/", then" should be moved from PACKAGE_RESOLVE to step 1 of PACKAGE_EXPORTS_RESOLVE. That would cover both the CJS and ESM resolvers.

[laverdet]

I'm flip flopping on whether or not this is a good place for it. By specifying this restriction in PACKAGE_IMPORTS_EXPORTS_RESOLVE you cover the imports, exports, and CJS cases. If the endsWith check goes here then step 8 in PACKAGE_RESOLVE can be removed since it is then redundant.

Another inconsistency in the specification discovered:

node/doc/api/esm.md

Lines 932 to 936 in f6ce486

	**PACKAGE\_IMPORTS\_RESOLVE**(_specifier_, _parentURL_, _conditions_)
	> 1. Assert: _specifier_ begins with _"#"_.
	> 2. If _specifier_ is exactly equal to _"#"_ or starts with _"#/"_, then
	> 1. Throw an _Invalid Module Specifier_ error.

Implementation:

node/lib/internal/modules/esm/resolve.js

Lines 694 to 699 in 8c2df73

	function packageImportsResolve(name, base, conditions) {
	if (name === '#' || StringPrototypeStartsWith(name, '#/') ||
	StringPrototypeEndsWith(name, '/')) {
	const reason = 'is not a valid internal imports specifier name';
	throw new ERR_INVALID_MODULE_SPECIFIER(name, reason, fileURLToPath(base));
	}

As specified if you have "imports": { "#test/test/": "./index.js" } in your package.json then import("#test/test/") should work, but this specifier throws both with import & require today. With the check in PACKAGE_IMPORTS_EXPORTS_RESOLVE then the specification matches reality.

I pushed that tweak and addressed the "contains '*'" invariant you raised earlier. Let me know what you think.

Also I noticed that the double-slash restriction is not specified at all. I suppose that could also go in the same place if you wanted to specify it.

[guybedford]

This feels like the right place for it to me, since CJS will otherwise still support trailing / resolutions fine - it's only the exports field that bans them.

Your latest change therefore looks correct to me, even though the implementation does not yet match it!

This case is handled in the ESM pseudocode here:

Given this I think I also found another implementation bug, as the following doesn't give me a deprecation warning package own-name resolution when it should:

package.json

{
  "name": "test",
  "exports": {
    "./a/*": "./a/*z.js"
  }
}

test.js

import 'test/a/b/'

I think this might be a good sign that it is time to complete the full deprecation cycle on the trailing slash imports and ensure they apply for own-name as well as normal resolutions.

The last thing I'm trying to figure out is if this new check can replace the higher up one in ESM_RESOLVE to avoid redundancy.

For packages without exports, they would only fail at resolution finalization time so it would be a different error but we did used to do this as well, and otherwise it seems like the error will still apply fine in all cases so it might even be an option.

[laverdet]

If it's helpful, I faithfully implemented the pseudocode (see: ESM, CJS) for some loaders I'm working on which is how I stumbled on these issues. My tests are hardly comprehensive but here's the trailing slash tests.

Anyway let me know if there's anything else I should cover for this PR or if we're good to go.

[guybedford]

The last thing I'm trying to figure out is if this new check can replace the higher up one in ESM_RESOLVE to avoid redundancy.

This is my last question here - if instead of checking the trailing slash in two separate places we just make this new location the only place to do that, and rely on ESM resolution always failing at resolution time for trailing slashes in the non exports case.

[laverdet]

The check at the top of PACKAGE_IMPORTS_EXPORTS_RESOLVE appears to cover all the cases we want.

Semi-related, I just noticed this quirk:

marcel[10:45:40PM] [~/test] cat a.mjs 
console.log(import.meta.resolve('./does-not-exist'));

marcel[10:45:40PM] [~/test] node a.mjs 
file:///Users/marcel/test/does-not-exist

According to the pseudocode this should fail to resolve since step 7.3 in ESM_RESOLVE verifies that the file exists.

[guybedford]

Let me rephrase the question: Given the new check inside of PACKAGE_IMPORTS_EXPORTS_RESOLVE, can we depreacte the existing previous check in step 8 of PACKAGE_RESOLVE? The argument being that in the non-exports case / trailers are always a not found or directory error when hitting the filesystem.

[guybedford]

The import.meta.resolve implementation does not do FS existence checks on the final resolution as an exception to better align with browser implementations.

[laverdet]

Given the new check inside of PACKAGE_IMPORTS_EXPORTS_RESOLVE, can we depreacte the existing previous check in step 8 of PACKAGE_RESOLVE?

Yes, I believe so. That change was included in 4305537 attached to the PR.

[guybedford]

Thanks again for working through this so thoroughly.

[nodejs-github-bot]

Landed in 86d8540