From a7b91e363eb90e394c59faac474185327651cab9 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Sat, 28 Dec 2024 15:54:19 +0100 Subject: [PATCH 1/8] module: fix async resolution error within the sync `findPackageJSON` --- lib/internal/modules/package_json_reader.js | 7 ++++ test/parallel/test-find-package-json.js | 46 +++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js index ab9842ee53a7c8..3fec243ce275bb 100644 --- a/lib/internal/modules/package_json_reader.js +++ b/lib/internal/modules/package_json_reader.js @@ -299,6 +299,13 @@ function findPackageJSON(specifier, base = 'data:') { cascadedLoader ??= require('internal/modules/esm/loader').getOrInitializeCascadedLoader(); try { + // The problem is here: when an async resolve hook is registered, resolve returns a promise + // I think 2 things are needed: + // 1. detect whether findPackageJSON is being used within a loader + // (possibly piggyback on `allowImportMetaResolve`) + // 2a. When inside, use the default resolve + // (I think it's impossible to use the chain because of re-entry & a deadlock from atomics). + // 2b. When outside, use cascadedLoader.resolveSync (not implemented yet, but the pieces exist). resolvedTarget = cascadedLoader.resolve(specifier, `${parentURL}`, pjsonImportAttributes).url; } catch (err) { if (err.code === 'ERR_UNSUPPORTED_DIR_IMPORT') { diff --git a/test/parallel/test-find-package-json.js b/test/parallel/test-find-package-json.js index cc74b46c031aa1..03ab73c2d97249 100644 --- a/test/parallel/test-find-package-json.js +++ b/test/parallel/test-find-package-json.js @@ -4,6 +4,7 @@ const common = require('../common'); const fixtures = require('../common/fixtures'); const tmpdir = require('../common/tmpdir'); const assert = require('node:assert'); +const { spawnSync } = require('node:child_process'); const fs = require('node:fs'); const { findPackageJSON } = require('node:module'); const path = require('node:path'); @@ -149,4 +150,49 @@ describe('findPackageJSON', () => { // Throws when no arguments are provided }); })); }); + + it('should work within a loader', () => { + const importMetaUrl = `${fixtures.fileURL('whatever.ext')}`; + const specifierBase = './packages/root-types-field'; + const foundPjsonPath = path.toNamespacedPath(fixtures.path(specifierBase, 'package.json')); + const { status: code, stderr, stdout } = spawnSync(process.execPath, [ + '--no-warnings', + '--loader', + [ + 'data:text/javascript,', + 'import module from "node:module";', + `module.findPackageJSON('${specifierBase}','${importMetaUrl}');`, + 'export const resolve = async (s, c, n) => n(s);', + ].join(''), + '--eval', + 'import module from "node:module";', // can be anything that triggers the resolve hook chain + ], { encoding: 'utf-8' }); + + // Error you're trying to fix: + // TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance + // of URL. Received undefined + assert.strictEqual(stderr, ''); + assert.match(stdout, new RegExp(foundPjsonPath)); + assert.strictEqual(code, 0); + }); + + it('should work with an async resolve hook registered', () => { + const importMetaUrl = `${fixtures.fileURL('whatever.ext')}`; + const specifierBase = './packages/root-types-field'; + const foundPjsonPath = path.toNamespacedPath(fixtures.path(specifierBase, 'package.json')); + const { status: code, stderr, stdout } = spawnSync(process.execPath, [ + '--no-warnings', + '--loader', + 'data:text/javascript,export const resolve = async (s, c, n) => n(s);', + '--eval', + `console.log(require("node:module").findPackageJSON('${specifierBase}','${importMetaUrl}'));` + ], { encoding: 'utf-8' }); + + // Error you're trying to fix: + // TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance + // of URL. Received undefined + assert.strictEqual(stderr, ''); + assert.match(stdout, new RegExp(foundPjsonPath)); + assert.strictEqual(code, 0); + }); }); From 10fa02d3a3bba88393af633c90b29ab7cb8048c7 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 28 Dec 2024 19:14:29 +0200 Subject: [PATCH 2/8] implement resolveSync with defaultResolve & polish test-cases Signed-off-by: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> --- lib/internal/modules/esm/hooks.js | 4 +-- lib/internal/modules/package_json_reader.js | 9 +----- test/parallel/test-find-package-json.js | 35 +++++++++------------ 3 files changed, 18 insertions(+), 30 deletions(-) diff --git a/lib/internal/modules/esm/hooks.js b/lib/internal/modules/esm/hooks.js index 4571922ed5a0e9..bf24e0be954f11 100644 --- a/lib/internal/modules/esm/hooks.js +++ b/lib/internal/modules/esm/hooks.js @@ -319,8 +319,8 @@ class Hooks { }; } - resolveSync(_originalSpecifier, _parentURL, _importAttributes) { - throw new ERR_METHOD_NOT_IMPLEMENTED('resolveSync()'); + resolveSync(originalSpecifier, parentURL, importAttributes) { + return defaultResolve(originalSpecifier, parentURL, importAttributes); } /** diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js index 3fec243ce275bb..41ed84a031e7cc 100644 --- a/lib/internal/modules/package_json_reader.js +++ b/lib/internal/modules/package_json_reader.js @@ -299,14 +299,7 @@ function findPackageJSON(specifier, base = 'data:') { cascadedLoader ??= require('internal/modules/esm/loader').getOrInitializeCascadedLoader(); try { - // The problem is here: when an async resolve hook is registered, resolve returns a promise - // I think 2 things are needed: - // 1. detect whether findPackageJSON is being used within a loader - // (possibly piggyback on `allowImportMetaResolve`) - // 2a. When inside, use the default resolve - // (I think it's impossible to use the chain because of re-entry & a deadlock from atomics). - // 2b. When outside, use cascadedLoader.resolveSync (not implemented yet, but the pieces exist). - resolvedTarget = cascadedLoader.resolve(specifier, `${parentURL}`, pjsonImportAttributes).url; + resolvedTarget = cascadedLoader.resolveSync(specifier, `${parentURL}`, pjsonImportAttributes).url; } catch (err) { if (err.code === 'ERR_UNSUPPORTED_DIR_IMPORT') { resolvedTarget = err.url; diff --git a/test/parallel/test-find-package-json.js b/test/parallel/test-find-package-json.js index 03ab73c2d97249..264c2288c3e2b3 100644 --- a/test/parallel/test-find-package-json.js +++ b/test/parallel/test-find-package-json.js @@ -151,48 +151,43 @@ describe('findPackageJSON', () => { // Throws when no arguments are provided })); }); - it('should work within a loader', () => { - const importMetaUrl = `${fixtures.fileURL('whatever.ext')}`; + it('should work within a loader', async () => { const specifierBase = './packages/root-types-field'; + const target = fixtures.fileURL(specifierBase, 'index.js'); const foundPjsonPath = path.toNamespacedPath(fixtures.path(specifierBase, 'package.json')); - const { status: code, stderr, stdout } = spawnSync(process.execPath, [ + const { code, stderr, stdout } = await common.spawnPromisified(process.execPath, [ '--no-warnings', '--loader', [ 'data:text/javascript,', + 'import fs from "node:fs";', 'import module from "node:module";', - `module.findPackageJSON('${specifierBase}','${importMetaUrl}');`, + encodeURIComponent(`fs.writeSync(1, module.findPackageJSON(${JSON.stringify(target)}));`), 'export const resolve = async (s, c, n) => n(s);', ].join(''), '--eval', - 'import module from "node:module";', // can be anything that triggers the resolve hook chain - ], { encoding: 'utf-8' }); + 'import "node:os";', // can be anything that triggers the resolve hook chain + ]); - // Error you're trying to fix: - // TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance - // of URL. Received undefined assert.strictEqual(stderr, ''); - assert.match(stdout, new RegExp(foundPjsonPath)); + assert.ok(stdout.includes(foundPjsonPath), stdout); assert.strictEqual(code, 0); }); - it('should work with an async resolve hook registered', () => { - const importMetaUrl = `${fixtures.fileURL('whatever.ext')}`; + it('should work with an async resolve hook registered', async () => { const specifierBase = './packages/root-types-field'; + const target = fixtures.fileURL(specifierBase, 'index.js'); const foundPjsonPath = path.toNamespacedPath(fixtures.path(specifierBase, 'package.json')); - const { status: code, stderr, stdout } = spawnSync(process.execPath, [ + const { code, stderr, stdout } = await common.spawnPromisified(process.execPath, [ '--no-warnings', '--loader', 'data:text/javascript,export const resolve = async (s, c, n) => n(s);', - '--eval', - `console.log(require("node:module").findPackageJSON('${specifierBase}','${importMetaUrl}'));` - ], { encoding: 'utf-8' }); + '--print', + `require("node:module").findPackageJSON(${JSON.stringify(target)})` + ]); - // Error you're trying to fix: - // TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance - // of URL. Received undefined assert.strictEqual(stderr, ''); - assert.match(stdout, new RegExp(foundPjsonPath)); + assert.ok(stdout.includes(foundPjsonPath), stdout); assert.strictEqual(code, 0); }); }); From 16194597c9ce4a430902bf8d58f3f3e81644c3e8 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Mon, 30 Dec 2024 12:36:51 +0100 Subject: [PATCH 3/8] revert basic implementation of resolveSync; use defaultResolve directly --- lib/internal/modules/esm/hooks.js | 4 ++-- lib/internal/modules/package_json_reader.js | 13 +++++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/internal/modules/esm/hooks.js b/lib/internal/modules/esm/hooks.js index bf24e0be954f11..4571922ed5a0e9 100644 --- a/lib/internal/modules/esm/hooks.js +++ b/lib/internal/modules/esm/hooks.js @@ -319,8 +319,8 @@ class Hooks { }; } - resolveSync(originalSpecifier, parentURL, importAttributes) { - return defaultResolve(originalSpecifier, parentURL, importAttributes); + resolveSync(_originalSpecifier, _parentURL, _importAttributes) { + throw new ERR_METHOD_NOT_IMPLEMENTED('resolveSync()'); } /** diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js index 41ed84a031e7cc..6e1da13fdc479f 100644 --- a/lib/internal/modules/package_json_reader.js +++ b/lib/internal/modules/package_json_reader.js @@ -267,8 +267,8 @@ function getPackageJSONURL(specifier, base) { throw new ERR_MODULE_NOT_FOUND(packageName, fileURLToPath(base), null); } -const pjsonImportAttributes = { __proto__: null, type: 'json' }; -let cascadedLoader; +/** @type {import('./esm/resolve.js').defaultResolve} */ +let defaultResolve; /** * @param {URL['href'] | string | URL} specifier The location for which to get the "root" package.json * @param {URL['href'] | string | URL} [base] The location of the current module (ex file://tmp/foo.js). @@ -296,10 +296,15 @@ function findPackageJSON(specifier, base = 'data:') { } let resolvedTarget; - cascadedLoader ??= require('internal/modules/esm/loader').getOrInitializeCascadedLoader(); + defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve; try { - resolvedTarget = cascadedLoader.resolveSync(specifier, `${parentURL}`, pjsonImportAttributes).url; + // TODO(@JakobJingleheimer): Detect whether findPackageJSON is being used within a loader + // (possibly piggyback on `allowImportMetaResolve`) + // - When inside, use the default resolve + // - (I think it's impossible to use the chain because of re-entry & a deadlock from atomics). + // - When outside, use cascadedLoader.resolveSync (not implemented yet, but the pieces exist). + resolvedTarget = defaultResolve(specifier, { parentURL: `${parentURL}` }).url; } catch (err) { if (err.code === 'ERR_UNSUPPORTED_DIR_IMPORT') { resolvedTarget = err.url; From 9cef635bab386d8e9a318a9e04ba0adb30e9ba68 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Mon, 30 Dec 2024 12:55:33 +0100 Subject: [PATCH 4/8] de-lint --- test/parallel/test-find-package-json.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-find-package-json.js b/test/parallel/test-find-package-json.js index 264c2288c3e2b3..5e55e81da1e7b8 100644 --- a/test/parallel/test-find-package-json.js +++ b/test/parallel/test-find-package-json.js @@ -4,7 +4,6 @@ const common = require('../common'); const fixtures = require('../common/fixtures'); const tmpdir = require('../common/tmpdir'); const assert = require('node:assert'); -const { spawnSync } = require('node:child_process'); const fs = require('node:fs'); const { findPackageJSON } = require('node:module'); const path = require('node:path'); @@ -166,7 +165,7 @@ describe('findPackageJSON', () => { // Throws when no arguments are provided 'export const resolve = async (s, c, n) => n(s);', ].join(''), '--eval', - 'import "node:os";', // can be anything that triggers the resolve hook chain + 'import "node:os";', // Can be anything that triggers the resolve hook chain ]); assert.strictEqual(stderr, ''); @@ -183,7 +182,7 @@ describe('findPackageJSON', () => { // Throws when no arguments are provided '--loader', 'data:text/javascript,export const resolve = async (s, c, n) => n(s);', '--print', - `require("node:module").findPackageJSON(${JSON.stringify(target)})` + `require("node:module").findPackageJSON(${JSON.stringify(target)})`, ]); assert.strictEqual(stderr, ''); From e3972d2d85888570739b1e009f56024132527201 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Mon, 30 Dec 2024 12:55:36 +0100 Subject: [PATCH 5/8] add (temporary) limitation caveat to `findPackageJSON` doc entry --- doc/api/module.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/doc/api/module.md b/doc/api/module.md index e50b3c8b44be21..a68603767399b6 100644 --- a/doc/api/module.md +++ b/doc/api/module.md @@ -81,14 +81,18 @@ added: v23.2.0 * `base` {string|URL} The absolute location (`file:` URL string or FS path) of the containing module. For CJS, use `__filename` (not `__dirname`!); for ESM, use `import.meta.url`. You do not need to pass it if `specifier` is an `absolute specifier`. -* Returns: {string|undefined} A path if the `package.json` is found. When `startLocation` +* Returns: {string|undefined} A path if the `package.json` is found. When `specifier` is a package, the package's root `package.json`; when a relative or unresolved, the closest - `package.json` to the `startLocation`. + `package.json` to the `specifier`. > **Caveat**: Do not use this to try to determine module format. There are many things effecting > that determination; the `type` field of package.json is the _least_ definitive (ex file extension > superceeds it, and a loader hook superceeds that). +> **Caveat**: This currently leverages only the built-in default resolver; if +> [`resolve` customization hooks][resolve hook] are registered, they will not effect the resolution. +> This may change in future. + ```text /path/to/project ├ packages/ From 9ea11460a94f51a3167b65c32352e888b112e807 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Mon, 30 Dec 2024 12:50:52 -0500 Subject: [PATCH 6/8] =?UTF-8?q?english=20=E2=86=92=20american=20?= =?UTF-8?q?=F0=9F=98=85?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jordan Harband --- doc/api/module.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/module.md b/doc/api/module.md index a68603767399b6..e2a04c1a1646e6 100644 --- a/doc/api/module.md +++ b/doc/api/module.md @@ -91,7 +91,7 @@ added: v23.2.0 > **Caveat**: This currently leverages only the built-in default resolver; if > [`resolve` customization hooks][resolve hook] are registered, they will not effect the resolution. -> This may change in future. +> This may change in the future. ```text /path/to/project From fcb44708dbc7b1516c518be76b86318d00256bd5 Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Mon, 30 Dec 2024 12:52:48 -0500 Subject: [PATCH 7/8] fix typos Co-authored-by: Jordan Harband --- doc/api/module.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/module.md b/doc/api/module.md index e2a04c1a1646e6..6285b74df82fc2 100644 --- a/doc/api/module.md +++ b/doc/api/module.md @@ -85,12 +85,12 @@ added: v23.2.0 is a package, the package's root `package.json`; when a relative or unresolved, the closest `package.json` to the `specifier`. -> **Caveat**: Do not use this to try to determine module format. There are many things effecting +> **Caveat**: Do not use this to try to determine module format. There are many things affecting > that determination; the `type` field of package.json is the _least_ definitive (ex file extension -> superceeds it, and a loader hook superceeds that). +> supercedes it, and a loader hook supercedes that). > **Caveat**: This currently leverages only the built-in default resolver; if -> [`resolve` customization hooks][resolve hook] are registered, they will not effect the resolution. +> [`resolve` customization hooks][resolve hook] are registered, they will not affect the resolution. > This may change in the future. ```text From 208fa8f5218fd71d9ecfba5701ee9a05f69c255f Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Mon, 30 Dec 2024 12:58:24 -0500 Subject: [PATCH 8/8] other typos --- doc/api/module.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/module.md b/doc/api/module.md index 6285b74df82fc2..bf0171fad54b31 100644 --- a/doc/api/module.md +++ b/doc/api/module.md @@ -87,7 +87,7 @@ added: v23.2.0 > **Caveat**: Do not use this to try to determine module format. There are many things affecting > that determination; the `type` field of package.json is the _least_ definitive (ex file extension -> supercedes it, and a loader hook supercedes that). +> supersedes it, and a loader hook supersedes that). > **Caveat**: This currently leverages only the built-in default resolver; if > [`resolve` customization hooks][resolve hook] are registered, they will not affect the resolution.