From 965876c5f2f2de16c37ed34a380ae39bc2efcba7 Mon Sep 17 00:00:00 2001 From: Ross Brandes Date: Tue, 6 Oct 2026 11:40:13 -0400 Subject: [PATCH] Resolve bare ESM specifiers from the caller, not from quibble (pnpm) quibble.esm() and esmImportWithPath() find a bare specifier's URL by doing a marked dummy import() that quibble's resolve hook intercepts. That import() lives in lib/esm-import-functions.js, so Node resolved the specifier from quibble's own directory instead of the caller's. With npm's flat node_modules the two see the same packages, but under pnpm quibble lives in node_modules/.pnpm/quibble@x/node_modules/quibble, so (#84): - with hoisting off, the import fails with "Cannot find package 'x' imported from .../quibble/lib/esm-import-functions.js" - in a monorepo where packages need different versions of a dependency, quibble can silently mock a different copy than the one the caller imports Find the caller for bare specifiers the same way as for relative ones (honoring ignoreCallsFromThisFile, which wrappers like testdouble.js's td.replaceEsm() rely on), send its URL along in the __quibbleresolveurl marker, and have both the async and sync resolve hooks resolve from it by passing nextResolve a copy of the context with that parentURL. Without a caller, the marker carries no URL and resolution is unchanged. Tests use fake packages in test/esm-lib/node_modules, which the tests can resolve but quibble's lib directory can't (or, for is-number, resolves to the real copy in the root node_modules instead). --- lib/esm-import-functions.js | 14 ++++--- lib/loader-helpers.js | 37 ++++++++++++++++--- lib/quibble-sync-hooks.js | 3 +- lib/quibble.js | 18 +++------ lib/quibble.mjs | 3 +- test/esm-fixtures/a-quibble-esm-wrapper.mjs | 7 ++++ .../a-dependency-of-the-tests/index.js | 2 + .../a-dependency-of-the-tests/package.json | 7 ++++ test/esm-lib/node_modules/is-number/index.js | 1 + .../node_modules/is-number/package.json | 7 ++++ .../quibble-cjs-esmImportWithPath.test.js | 11 ++++++ test/esm-lib/quibble-esm.test.mjs | 28 ++++++++++++++ 12 files changed, 113 insertions(+), 25 deletions(-) create mode 100644 test/esm-fixtures/a-quibble-esm-wrapper.mjs create mode 100644 test/esm-lib/node_modules/a-dependency-of-the-tests/index.js create mode 100644 test/esm-lib/node_modules/a-dependency-of-the-tests/package.json create mode 100644 test/esm-lib/node_modules/is-number/index.js create mode 100644 test/esm-lib/node_modules/is-number/package.json diff --git a/lib/esm-import-functions.js b/lib/esm-import-functions.js index 1340544..fc5ebab 100644 --- a/lib/esm-import-functions.js +++ b/lib/esm-import-functions.js @@ -1,11 +1,14 @@ const path = require('path') const { pathToFileURL } = require('url') -exports.dummyImportModuleToGetAtPath = async function dummyImportModuleToGetAtPath (modulePath) { +// `parentUrl` is the file the specifier should be resolved from. Without it, +// the specifier is resolved from this file, which can't see the caller's +// dependencies when they're not installed in a flat node_modules (e.g. pnpm). +exports.dummyImportModuleToGetAtPath = async function dummyImportModuleToGetAtPath (modulePath, parentUrl) { try { const moduleUrl = path.isAbsolute(modulePath) ? pathToFileURL(modulePath) : modulePath - await import(addQueryToUrl(moduleUrl, '__quibbleresolveurl')) + await import(addQueryToUrl(moduleUrl, '__quibbleresolveurl', parentUrl)) } catch (error) { if (error.code === 'QUIBBLE_RESOLVED_URL') { return error.resolvedUrl @@ -23,12 +26,13 @@ exports.importOriginalModule = async (fullImportPath) => { return import(addQueryToUrl(fullImportPath, '__quibbleoriginal')) } -function addQueryToUrl (url, query) { +function addQueryToUrl (url, query, value) { + const queryWithValue = value ? `${query}=${encodeURIComponent(value)}` : query try { const urlObject = new URL(url) urlObject.searchParams.set(query, '') - return urlObject.href.replace(`${query}=`, query) + return urlObject.href.replace(`${query}=`, queryWithValue) } catch { - return url + '?' + query + return url + '?' + queryWithValue } } diff --git a/lib/loader-helpers.js b/lib/loader-helpers.js index c7b70a2..6b9c21b 100644 --- a/lib/loader-helpers.js +++ b/lib/loader-helpers.js @@ -3,9 +3,9 @@ // (registerHooks). Kept as CJS so both can load it without an extra // module-format dance. // -// Only planResolve/finishResolve/recoverResolve/stubbedLoadResult and -// stripQueryAndHash are used outside this file; everything else here is a -// private helper for them. +// Only planResolve/nextResolveContext/finishResolve/recoverResolve/ +// stubbedLoadResult and stripQueryAndHash are used outside this file; +// everything else here is a private helper for them. /** * @typedef {{hasDefaultExport: boolean, namedExports: [string]}} ModuleLoaderMockInfo @@ -60,15 +60,29 @@ function stripQueryAndHash (url) { return url.replace(/\?.*/, '').replace(/#.*/, '') } +const RESOLVE_URL_MARKER = /[?&]__quibbleresolveurl(?:=([^&#]*))?/ + /** Removes the marker query params the `import()` helpers add to specifiers */ function stripMarkers (specifier) { return specifier.includes('__quibble') ? specifier - .replace(/[?&]__quibbleresolveurl/, '') + .replace(RESOLVE_URL_MARKER, '') .replace(/[?&]__quibbleoriginal/, '') : specifier } +/** + * The context to call `nextResolve` with for a plan from `planResolve`: the + * same one, unless the plan has to be resolved from a different parent. + * Call it after flagging `context.__quibbleSuppressed`, so the copy is + * flagged too. + */ +function nextResolveContext (plan, context) { + return plan.resolveFromURL + ? { ...context, parentURL: plan.resolveFromURL } + : context +} + function addQueryToUrl (url, query, value) { const urlObject = new URL(url) urlObject.searchParams.set(query, value) @@ -93,13 +107,23 @@ function unresolvedUrl (specifier, parentURL, generation) { * @param {object} context * @returns { * | { kind: 'reentrant' } - * | { kind: 'resolveUrl' | 'passthrough', nextSpecifier: string } + * | { kind: 'resolveUrl', nextSpecifier: string, resolveFromURL: string | undefined } + * | { kind: 'passthrough', nextSpecifier: string } * | { kind: 'quibble', nextSpecifier: string, specifier: string, parentURL: string, stubModuleGeneration: number } * } */ function planResolve (state, specifier, context) { if (specifier.includes('__quibbleresolveurl')) { - return { kind: 'resolveUrl', nextSpecifier: stripMarkers(specifier) } + // The marker's value, if any, is the URL of the file that asked quibble + // to resolve `specifier`: resolve from there and not from the `import()` + // in esm-import-functions.js, which can't see that file's dependencies + // when they're not installed in a flat node_modules (e.g. pnpm). + const encodedResolveFromURL = specifier.match(RESOLVE_URL_MARKER)[1] + return { + kind: 'resolveUrl', + nextSpecifier: stripMarkers(specifier), + resolveFromURL: encodedResolveFromURL && decodeURIComponent(encodedResolveFromURL) + } } if (specifier === 'quibble' || specifier.includes('__quibbleoriginal')) { @@ -209,6 +233,7 @@ function stubbedLoadResult (state, url) { module.exports = { planResolve, + nextResolveContext, finishResolve, recoverResolve, stubbedLoadResult, diff --git a/lib/quibble-sync-hooks.js b/lib/quibble-sync-hooks.js index 67508b4..6967036 100644 --- a/lib/quibble-sync-hooks.js +++ b/lib/quibble-sync-hooks.js @@ -6,6 +6,7 @@ const { planResolve, + nextResolveContext, finishResolve, recoverResolve, stubbedLoadResult, @@ -36,7 +37,7 @@ exports.createSyncHooks = function createSyncHooks (state) { context.__quibbleSuppressed = true try { - const result = nextResolve(plan.nextSpecifier, context) + const result = nextResolve(plan.nextSpecifier, nextResolveContext(plan, context)) if (plan.kind === 'quibble') { // Same off-thread-loader caveat as below: work with untagged URLs const url = removeQuibbleQuery(result.url) diff --git a/lib/quibble.js b/lib/quibble.js index dfbc02b..fb88971 100644 --- a/lib/quibble.js +++ b/lib/quibble.js @@ -105,12 +105,9 @@ quibble.esm = async function (specifier, namedExportStubs, defaultExportStub) { delete finalNamedExportStubs.default } - const importPathIsBareSpecifier = isBareSpecifier(specifier) - const parentUrl = importPathIsBareSpecifier - ? undefined - : hackErrorStackToGetCallerFile(true, true) - const moduleUrl = importPathIsBareSpecifier - ? await importFunctions.dummyImportModuleToGetAtPath(specifier) + const parentUrl = hackErrorStackToGetCallerFile(true, true) + const moduleUrl = isBareSpecifier(specifier) + ? await importFunctions.dummyImportModuleToGetAtPath(specifier, parentUrl) : new URL(specifier, parentUrl).href await quibbleUserToLoaderCommunication().addMockedModule(moduleUrl, { @@ -133,12 +130,9 @@ quibble.isLoaderLoaded = function () { quibble.esmImportWithPath = async function esmImportWithPath (specifier) { checkThatLoaderIsLoaded() - const importPathIsBareSpecifier = isBareSpecifier(specifier) - const parentUrl = importPathIsBareSpecifier - ? undefined - : hackErrorStackToGetCallerFile(true, true) - const moduleUrl = importPathIsBareSpecifier - ? await importFunctions.dummyImportModuleToGetAtPath(specifier) + const parentUrl = hackErrorStackToGetCallerFile(true, true) + const moduleUrl = isBareSpecifier(specifier) + ? await importFunctions.dummyImportModuleToGetAtPath(specifier, parentUrl) : new URL(specifier, parentUrl).href return { diff --git a/lib/quibble.mjs b/lib/quibble.mjs index c8494b5..339d8d8 100644 --- a/lib/quibble.mjs +++ b/lib/quibble.mjs @@ -5,6 +5,7 @@ import importGraphModule from './import-graph.js' const { planResolve, + nextResolveContext, finishResolve, recoverResolve, stubbedLoadResult, @@ -47,7 +48,7 @@ export async function resolve (specifier, context, nextResolve) { context.__quibbleSuppressed = true try { - const result = await nextResolve(plan.nextSpecifier, context) + const result = await nextResolve(plan.nextSpecifier, nextResolveContext(plan, context)) if (plan.kind === 'quibble') { importGraph.recordImport(context.parentURL, result.url) if (!importGraph.needsQuibbling(result.url)) { diff --git a/test/esm-fixtures/a-quibble-esm-wrapper.mjs b/test/esm-fixtures/a-quibble-esm-wrapper.mjs new file mode 100644 index 0000000..b01832e --- /dev/null +++ b/test/esm-fixtures/a-quibble-esm-wrapper.mjs @@ -0,0 +1,7 @@ +// Calls quibble.esm() on behalf of whoever calls it, like testdouble.js's td.replaceEsm() +import quibble from '../../lib/quibble.mjs' + +quibble.ignoreCallsFromThisFile() + +export default (specifier, namedExportStubs, defaultExportStub) => + quibble.esm(specifier, namedExportStubs, defaultExportStub) diff --git a/test/esm-lib/node_modules/a-dependency-of-the-tests/index.js b/test/esm-lib/node_modules/a-dependency-of-the-tests/index.js new file mode 100644 index 0000000..710291d --- /dev/null +++ b/test/esm-lib/node_modules/a-dependency-of-the-tests/index.js @@ -0,0 +1,2 @@ +export default 'default-export' +export const namedExport = 'named-export' diff --git a/test/esm-lib/node_modules/a-dependency-of-the-tests/package.json b/test/esm-lib/node_modules/a-dependency-of-the-tests/package.json new file mode 100644 index 0000000..e318bbc --- /dev/null +++ b/test/esm-lib/node_modules/a-dependency-of-the-tests/package.json @@ -0,0 +1,7 @@ +{ + "name": "a-dependency-of-the-tests", + "description": "Fake dependency that only the tests (not quibble's lib directory) can resolve, like a dependency installed with pnpm", + "version": "1.0.0", + "type": "module", + "main": "index.js" +} diff --git a/test/esm-lib/node_modules/is-number/index.js b/test/esm-lib/node_modules/is-number/index.js new file mode 100644 index 0000000..0fa0795 --- /dev/null +++ b/test/esm-lib/node_modules/is-number/index.js @@ -0,0 +1 @@ +export default () => 'the copy only the tests can see' diff --git a/test/esm-lib/node_modules/is-number/package.json b/test/esm-lib/node_modules/is-number/package.json new file mode 100644 index 0000000..218cc04 --- /dev/null +++ b/test/esm-lib/node_modules/is-number/package.json @@ -0,0 +1,7 @@ +{ + "name": "is-number", + "description": "Fake second copy of a dependency quibble's lib directory can also resolve (to the real one in the root node_modules), like a pnpm monorepo with two versions of a package", + "version": "100.0.0", + "type": "module", + "main": "index.js" +} diff --git a/test/esm-lib/quibble-cjs-esmImportWithPath.test.js b/test/esm-lib/quibble-cjs-esmImportWithPath.test.js index b18b149..a1e357b 100644 --- a/test/esm-lib/quibble-cjs-esmImportWithPath.test.js +++ b/test/esm-lib/quibble-cjs-esmImportWithPath.test.js @@ -27,6 +27,17 @@ module.exports = { assert.deepEqual(isPromise(Promise.resolve()), true) assert.deepEqual(isPromise(42), false) }, + 'support importing esm and returning the path for a bare specifier only the caller can resolve': async function () { + // Lives in test/esm-lib/node_modules, so it can't be resolved from quibble's + // own lib directory (like a dependency installed by pnpm) + const { moduleUrl, module } = await quibble.esmImportWithPath('a-dependency-of-the-tests') + + assert.deepEqual(moduleUrl, pathToFileURL(path.resolve(__dirname, 'node_modules/a-dependency-of-the-tests/index.js')).href) + assert.deepEqual({ ...module }, { + default: 'default-export', + namedExport: 'named-export' + }) + }, 'support importing esm and returning the path even when relative path quibbled': async function () { await quibble.esm('./a-module.mjs', { namedExport: 'replacement', diff --git a/test/esm-lib/quibble-esm.test.mjs b/test/esm-lib/quibble-esm.test.mjs index 074adb3..d9575ea 100644 --- a/test/esm-lib/quibble-esm.test.mjs +++ b/test/esm-lib/quibble-esm.test.mjs @@ -104,6 +104,34 @@ export default { const { default: result } = await import('is-promise') assert.equal(result(), 42) }, + 'mock a 3rd party lib that only the caller can resolve': async function () { + // Lives in test/esm-lib/node_modules, so it can't be resolved from quibble's + // own lib directory (like a dependency installed by pnpm) + await quibble.esm('a-dependency-of-the-tests', { namedExport: 'replacement' }, 'default-export-replacement') + + const result = await import('a-dependency-of-the-tests') + assert.equal(result.default, 'default-export-replacement') + assert.equal(result.namedExport, 'replacement') + }, + 'mock the caller\'s copy of a 3rd party lib when quibble can resolve a different copy': async function () { + // test/esm-lib/node_modules/is-number shadows the real one in the root + // node_modules, which is the one quibble's own lib directory resolves to + // (like a pnpm monorepo where packages need different versions of it) + await quibble.esm('is-number', undefined, () => 'replacement') + + const { default: isNumber } = await import('is-number') + assert.equal(isNumber(), 'replacement') + }, + 'mock a 3rd party lib that only the caller of a wrapper that ignores itself can resolve': async function () { + // Like testdouble.js's td.replaceEsm(): the wrapper's own directory can't + // resolve the specifier, so it has to be resolved from this file + const { default: wrappedQuibbleEsm } = await import('../esm-fixtures/a-quibble-esm-wrapper.mjs') + await wrappedQuibbleEsm('a-dependency-of-the-tests', { namedExport: 'replacement' }, 'default-export-replacement') + + const result = await import('a-dependency-of-the-tests') + assert.equal(result.default, 'default-export-replacement') + assert.equal(result.namedExport, 'replacement') + }, 'isLoaderLoader returns true if loader as loaded': async function () { assert.equal(quibble.isLoaderLoaded(), true) },