Skip to content

Commit 4f77920

Browse files
JakobJingleheimeraduh95
authored andcommitted
module: fix async resolution error within the sync findPackageJSON
PR-URL: #56382 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
1 parent 6afe363 commit 4f77920

File tree

3 files changed

+57
-8
lines changed

3 files changed

+57
-8
lines changed

doc/api/module.md

+8-4
Original file line numberDiff line numberDiff line change
@@ -81,13 +81,17 @@ added: v23.2.0
8181
* `base` {string|URL} The absolute location (`file:` URL string or FS path) of the
8282
containing module. For CJS, use `__filename` (not `__dirname`!); for ESM, use
8383
`import.meta.url`. You do not need to pass it if `specifier` is an `absolute specifier`.
84-
* Returns: {string|undefined} A path if the `package.json` is found. When `startLocation`
84+
* Returns: {string|undefined} A path if the `package.json` is found. When `specifier`
8585
is a package, the package's root `package.json`; when a relative or unresolved, the closest
86-
`package.json` to the `startLocation`.
86+
`package.json` to the `specifier`.
8787
88-
> **Caveat**: Do not use this to try to determine module format. There are many things effecting
88+
> **Caveat**: Do not use this to try to determine module format. There are many things affecting
8989
> that determination; the `type` field of package.json is the _least_ definitive (ex file extension
90-
> superceeds it, and a loader hook superceeds that).
90+
> supersedes it, and a loader hook supersedes that).
91+
92+
> **Caveat**: This currently leverages only the built-in default resolver; if
93+
> [`resolve` customization hooks][resolve hook] are registered, they will not affect the resolution.
94+
> This may change in the future.
9195
9296
```text
9397
/path/to/project

lib/internal/modules/package_json_reader.js

+9-4
Original file line numberDiff line numberDiff line change
@@ -267,8 +267,8 @@ function getPackageJSONURL(specifier, base) {
267267
throw new ERR_MODULE_NOT_FOUND(packageName, fileURLToPath(base), null);
268268
}
269269

270-
const pjsonImportAttributes = { __proto__: null, type: 'json' };
271-
let cascadedLoader;
270+
/** @type {import('./esm/resolve.js').defaultResolve} */
271+
let defaultResolve;
272272
/**
273273
* @param {URL['href'] | string | URL} specifier The location for which to get the "root" package.json
274274
* @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:') {
296296
}
297297

298298
let resolvedTarget;
299-
cascadedLoader ??= require('internal/modules/esm/loader').getOrInitializeCascadedLoader();
299+
defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve;
300300

301301
try {
302-
resolvedTarget = cascadedLoader.resolve(specifier, `${parentURL}`, pjsonImportAttributes).url;
302+
// TODO(@JakobJingleheimer): Detect whether findPackageJSON is being used within a loader
303+
// (possibly piggyback on `allowImportMetaResolve`)
304+
// - When inside, use the default resolve
305+
// - (I think it's impossible to use the chain because of re-entry & a deadlock from atomics).
306+
// - When outside, use cascadedLoader.resolveSync (not implemented yet, but the pieces exist).
307+
resolvedTarget = defaultResolve(specifier, { parentURL: `${parentURL}` }).url;
303308
} catch (err) {
304309
if (err.code === 'ERR_UNSUPPORTED_DIR_IMPORT') {
305310
resolvedTarget = err.url;

test/parallel/test-find-package-json.js

+40
Original file line numberDiff line numberDiff line change
@@ -149,4 +149,44 @@ describe('findPackageJSON', () => { // Throws when no arguments are provided
149149
});
150150
}));
151151
});
152+
153+
it('should work within a loader', async () => {
154+
const specifierBase = './packages/root-types-field';
155+
const target = fixtures.fileURL(specifierBase, 'index.js');
156+
const foundPjsonPath = path.toNamespacedPath(fixtures.path(specifierBase, 'package.json'));
157+
const { code, stderr, stdout } = await common.spawnPromisified(process.execPath, [
158+
'--no-warnings',
159+
'--loader',
160+
[
161+
'data:text/javascript,',
162+
'import fs from "node:fs";',
163+
'import module from "node:module";',
164+
encodeURIComponent(`fs.writeSync(1, module.findPackageJSON(${JSON.stringify(target)}));`),
165+
'export const resolve = async (s, c, n) => n(s);',
166+
].join(''),
167+
'--eval',
168+
'import "node:os";', // Can be anything that triggers the resolve hook chain
169+
]);
170+
171+
assert.strictEqual(stderr, '');
172+
assert.ok(stdout.includes(foundPjsonPath), stdout);
173+
assert.strictEqual(code, 0);
174+
});
175+
176+
it('should work with an async resolve hook registered', async () => {
177+
const specifierBase = './packages/root-types-field';
178+
const target = fixtures.fileURL(specifierBase, 'index.js');
179+
const foundPjsonPath = path.toNamespacedPath(fixtures.path(specifierBase, 'package.json'));
180+
const { code, stderr, stdout } = await common.spawnPromisified(process.execPath, [
181+
'--no-warnings',
182+
'--loader',
183+
'data:text/javascript,export const resolve = async (s, c, n) => n(s);',
184+
'--print',
185+
`require("node:module").findPackageJSON(${JSON.stringify(target)})`,
186+
]);
187+
188+
assert.strictEqual(stderr, '');
189+
assert.ok(stdout.includes(foundPjsonPath), stdout);
190+
assert.strictEqual(code, 0);
191+
});
152192
});

0 commit comments

Comments
 (0)