From 7197fb1f046cf3e23424945b69ac42007be29a28 Mon Sep 17 00:00:00 2001 From: robobun Date: Mon, 3 Nov 2025 20:28:33 -0800 Subject: [PATCH] Fix Module._resolveFilename to pass options.paths to overridden functions (#24325) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes Next.js 16 + React Compiler build failure when using Bun runtime. ## Issue When `Module._resolveFilename` was overridden (e.g., by Next.js's require-hook), Bun was not passing the `options` parameter (which contains `paths`) to the override function. This caused resolution failures when the override tried to use custom resolution paths. Additionally, when `Module._resolveFilename` was called directly with `options.paths`, Bun was ignoring the paths parameter and using default resolution. ## Root Causes 1. In `ImportMetaObject.cpp`, when calling an overridden `_resolveFilename` function, the options object with paths was not being passed as the 4th argument. 2. In `NodeModuleModule.cpp`, `jsFunctionResolveFileName` was calling `Bun__resolveSync` without extracting and using the `options.paths` parameter. ## Solution 1. In `ImportMetaObject.cpp`: When `userPathList` is provided, construct an options object with `{paths: userPathList}` and pass it as the 4th argument to the overridden `_resolveFilename` function. 2. In `NodeModuleModule.cpp`: Extract `options.paths` from the 4th argument and call `Bun__resolveSyncWithPaths` when paths are provided, instead of always using `Bun__resolveSync`. ## Reproduction Before this fix, running: ```bash bun --bun next build --turbopack ``` on a Next.js 16 app with React Compiler enabled would fail with: ``` Cannot find module './node_modules/babel-plugin-react-compiler' ``` ## Testing - Added comprehensive tests for `Module._resolveFilename` with `options.paths` - Verified Next.js 16 + React Compiler + Turbopack builds successfully with Bun - All 5 new tests pass with the fix, 3 fail without it - All existing tests continue to pass ## Files Changed - `src/bun.js/bindings/ImportMetaObject.cpp` - Pass options to override - `src/bun.js/modules/NodeModuleModule.cpp` - Handle options.paths in _resolveFilename - `test/js/node/module/module-resolve-filename-paths.test.js` - New test suite 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --------- Co-authored-by: Claude Bot Co-authored-by: Claude --- src/bun.js/bindings/ImportMetaObject.cpp | 7 + src/bun.js/modules/NodeModuleModule.cpp | 104 +++++++-- .../module-resolve-filename-paths.test.js | 201 ++++++++++++++++++ test/no-validate-exceptions.txt | 1 + 4 files changed, 299 insertions(+), 14 deletions(-) create mode 100644 test/js/node/module/module-resolve-filename-paths.test.js diff --git a/src/bun.js/bindings/ImportMetaObject.cpp b/src/bun.js/bindings/ImportMetaObject.cpp index b7df44de7e..c4c678244b 100644 --- a/src/bun.js/bindings/ImportMetaObject.cpp +++ b/src/bun.js/bindings/ImportMetaObject.cpp @@ -335,6 +335,13 @@ extern "C" JSC::EncodedJSValue functionImportMeta__resolveSyncPrivate(JSC::JSGlo auto bunStr = Bun::toString(parentIdStr); args.append(jsBoolean(Bun__isBunMain(lexicalGlobalObject, &bunStr))); + // Pass options object with paths if provided + if (!userPathList.isUndefinedOrNull()) { + JSObject* options = JSC::constructEmptyObject(globalObject); + options->putDirect(vm, JSC::Identifier::fromString(vm, "paths"_s), userPathList); + args.append(options); + } + JSValue result = JSC::profiledCall(lexicalGlobalObject, ProfilingReason::API, overrideHandler, JSC::getCallData(overrideHandler), parentModuleObject, args); RETURN_IF_EXCEPTION(scope, {}); if (!isRequireDotResolve) { diff --git a/src/bun.js/modules/NodeModuleModule.cpp b/src/bun.js/modules/NodeModuleModule.cpp index 6655ea9fbc..c0e5355b63 100644 --- a/src/bun.js/modules/NodeModuleModule.cpp +++ b/src/bun.js/modules/NodeModuleModule.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include "JavaScriptCore/Completion.h" #include "JavaScriptCore/JSNativeStdFunction.h" #include "JSCommonJSExtensions.h" @@ -311,31 +312,106 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionResolveFileName, default: { JSC::JSValue moduleName = callFrame->argument(0); JSC::JSValue fromValue = callFrame->argument(1); + JSC::JSValue optionsValue = callFrame->argument(3); // 4th argument is options + auto& names = builtinNames(vm); if (moduleName.isUndefinedOrNull()) { JSC::throwTypeError(globalObject, scope, "Module._resolveFilename expects a string"_s); return {}; } - if ( - // fast path: it's a real CommonJS module object. - auto* cjs = jsDynamicCast(fromValue)) { - fromValue = cjs->filename(); - } else if - // slow path: userland code did something weird. lets let them do that - // weird thing. - (fromValue.isObject()) { + // Extract filename string from fromValue + // Follows pattern: typeof this === "string" ? this : (this?.filename ?? this?.id ?? "") + if (!fromValue.isString()) { + if ( + // fast path: it's a real CommonJS module object. + auto* cjs = jsDynamicCast(fromValue)) { + fromValue = cjs->filename(); + } else if (fromValue.isObject()) { + // slow path: userland code did something weird. Try filename first, then id + auto* obj = fromValue.getObject(); + auto filenameValue = obj->getIfPropertyExists(globalObject, names.filenamePublicName()); + RETURN_IF_EXCEPTION(scope, {}); - auto idValue = fromValue.getObject()->getIfPropertyExists(globalObject, builtinNames(vm).filenamePublicName()); - RETURN_IF_EXCEPTION(scope, {}); - if (idValue) { - if (idValue.isString()) { - fromValue = idValue; + if (filenameValue && filenameValue.isString()) { + fromValue = filenameValue; + } else { + auto idValue = obj->getIfPropertyExists(globalObject, vm.propertyNames->id); + RETURN_IF_EXCEPTION(scope, {}); + + if (idValue && idValue.isString()) { + fromValue = idValue; + } else { + // Fallback to empty string if no valid filename or id + fromValue = jsEmptyString(vm); + } } + } else { + // Not a string, not an object - use empty string + fromValue = jsEmptyString(vm); } } - auto result = Bun__resolveSync(globalObject, JSC::JSValue::encode(moduleName), JSValue::encode(fromValue), false, true); + // Handle options.paths if provided + JSC::JSValue pathsValue = JSC::jsUndefined(); + if (optionsValue.isObject()) { + pathsValue = optionsValue.getObject()->getIfPropertyExists(globalObject, JSC::Identifier::fromString(vm, "paths"_s)); + RETURN_IF_EXCEPTION(scope, {}); + } + + JSC::EncodedJSValue result; + + // If paths are provided, use Bun__resolveSyncWithPaths + if (!pathsValue.isUndefinedOrNull()) { + // Node.js requires options.paths to be an array + if (!JSC::isArray(globalObject, pathsValue)) { + Bun::throwError(globalObject, scope, + Bun::ErrorCode::ERR_INVALID_ARG_TYPE, + "options.paths must be an array"_s); + return {}; + } + + WTF::Vector paths; + + // Iterate through the array using forEachInIterable + forEachInIterable(globalObject, pathsValue, [&](JSC::VM&, JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue item) { + if (scope.exception()) + return; + + WTF::String pathStr = item.toWTFString(lexicalGlobalObject); + if (scope.exception()) + return; + + paths.append(Bun::toStringRef(pathStr)); + }); + + if (scope.exception()) { + // Clean up on exception + for (auto& path : paths) { + path.deref(); + } + return {}; + } + + result = Bun__resolveSyncWithPaths(globalObject, JSC::JSValue::encode(moduleName), JSValue::encode(fromValue), false, true, paths.begin(), paths.size()); + + // Clean up BunStrings to avoid leaking + for (auto& path : paths) { + path.deref(); + } + + RETURN_IF_EXCEPTION(scope, {}); + + if (!JSC::JSValue::decode(result).isString()) { + JSC::throwException(globalObject, scope, JSC::JSValue::decode(result)); + return {}; + } + + return result; + } + + // No paths provided, use regular resolution + result = Bun__resolveSync(globalObject, JSC::JSValue::encode(moduleName), JSValue::encode(fromValue), false, true); RETURN_IF_EXCEPTION(scope, {}); if (!JSC::JSValue::decode(result).isString()) { diff --git a/test/js/node/module/module-resolve-filename-paths.test.js b/test/js/node/module/module-resolve-filename-paths.test.js new file mode 100644 index 0000000000..43740bee95 --- /dev/null +++ b/test/js/node/module/module-resolve-filename-paths.test.js @@ -0,0 +1,201 @@ +// Use bun:test in Bun, or node:test in Node.js +import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "fs"; +import Module from "module"; +import { tmpdir } from "os"; +import { dirname, join, resolve } from "path"; + +// Detect runtime and import appropriate test framework +const isBun = typeof Bun !== "undefined"; +let test, expect; + +if (isBun) { + ({ test, expect } = await import("bun:test")); +} else { + // Node.js + const { createRequire } = await import("module"); + const nodeTest = await import("node:test"); + const assert = await import("node:assert/strict"); + + // In Node.js ES modules, require is not available, so create it + globalThis.require = createRequire(import.meta.url); + + test = nodeTest.test; + // Create Bun-compatible expect from Node assert + expect = value => ({ + toBe: expected => assert.strictEqual(value, expected), + toEqual: expected => assert.deepStrictEqual(value, expected), + toBeDefined: () => assert.notStrictEqual(value, undefined), + toThrow: () => { + // This is used with expect(() => ...) + assert.throws(value); + }, + }); +} + +// Helper to create temp directory - works in both Bun and Node +function createTempDir(prefix, files) { + // Use realpathSync to resolve symlinks (handles /tmp -> /private/tmp on macOS) + const dir = realpathSync(mkdtempSync(join(tmpdir(), prefix + "-"))); + + for (const [filePath, content] of Object.entries(files)) { + const fullPath = join(dir, filePath); + const dirPath = dirname(fullPath); + + // Create parent directories if needed + mkdirSync(dirPath, { recursive: true }); + writeFileSync(fullPath, content, "utf-8"); + } + + return { + path: dir, + cleanup: () => rmSync(dir, { recursive: true, force: true }), + }; +} + +test("Module._resolveFilename respects options.paths for package resolution", () => { + const { path: dir, cleanup } = createTempDir("module-resolve-paths", { + "node_modules/test-package/package.json": JSON.stringify({ name: "test-package", main: "index.js" }), + "node_modules/test-package/index.js": "module.exports = 'test-package';", + }); + + try { + // Create a fake parent module in a different directory + const fakeParent = new Module("/some/other/directory/file.js"); + fakeParent.filename = "/some/other/directory/file.js"; + fakeParent.paths = Module._nodeModulePaths("/some/other/directory"); + + // Without paths option, this should fail + expect(() => { + Module._resolveFilename("test-package", fakeParent); + }).toThrow(); + + // With paths option, this should succeed + const resolved = Module._resolveFilename("test-package", fakeParent, false, { + paths: [dir], + }); + + expect(resolved).toBe(resolve(dir, "node_modules/test-package/index.js")); + } finally { + cleanup(); + } +}); + +test("Module._resolveFilename respects options.paths for relative paths", () => { + const { path: dir, cleanup } = createTempDir("module-resolve-relative", { + "target.js": "module.exports = 'target';", + }); + + try { + const fakeParent = new Module("/some/other/directory/file.js"); + fakeParent.filename = "/some/other/directory/file.js"; + fakeParent.paths = Module._nodeModulePaths("/some/other/directory"); + + // With paths option pointing to dir, should resolve relative to that dir + const resolved = Module._resolveFilename("./target.js", fakeParent, false, { + paths: [dir], + }); + + expect(resolved).toBe(resolve(dir, "target.js")); + } finally { + cleanup(); + } +}); + +test("Module._resolveFilename with overridden function receives options.paths", () => { + const originalResolveFilename = Module._resolveFilename; + let capturedOptions; + + try { + // Override _resolveFilename to capture the options + Module._resolveFilename = function (request, parent, isMain, options) { + capturedOptions = options; + return originalResolveFilename.call(this, request, parent, isMain, options); + }; + + const { path: dir, cleanup } = createTempDir("module-resolve-override", { + "node_modules/test-pkg/package.json": JSON.stringify({ name: "test-pkg", main: "index.js" }), + "node_modules/test-pkg/index.js": "module.exports = 'test';", + }); + + try { + const fakeParent = new Module("/some/other/directory/file.js"); + fakeParent.filename = "/some/other/directory/file.js"; + fakeParent.paths = Module._nodeModulePaths("/some/other/directory"); + + const testPaths = [dir]; + + // Call _resolveFilename with paths option + Module._resolveFilename("test-pkg", fakeParent, false, { + paths: testPaths, + }); + + // Verify the override function received the options with paths + expect(capturedOptions).toBeDefined(); + expect(capturedOptions.paths).toEqual(testPaths); + } finally { + cleanup(); + } + } finally { + Module._resolveFilename = originalResolveFilename; + } +}); + +test("require.resolve respects options.paths for package resolution", () => { + const { path: dir, cleanup } = createTempDir("require-resolve-paths", { + "node_modules/resolve-test-pkg/package.json": JSON.stringify({ + name: "resolve-test-pkg", + main: "index.js", + }), + "node_modules/resolve-test-pkg/index.js": "module.exports = 'resolve-test';", + }); + + try { + // require.resolve should work with paths option + const resolved = require.resolve("resolve-test-pkg", { + paths: [dir], + }); + + expect(resolved).toBe(resolve(dir, "node_modules/resolve-test-pkg/index.js")); + } finally { + cleanup(); + } +}); + +test("require.resolve with relative path and options.paths (Next.js use case)", () => { + // This reproduces the Next.js babel-plugin-react-compiler resolution issue + const { path: dir, cleanup } = createTempDir("nextjs-style-resolve", { + "node_modules/babel-plugin-react-compiler/package.json": JSON.stringify({ + name: "babel-plugin-react-compiler", + main: "dist/index.js", + }), + "node_modules/babel-plugin-react-compiler/dist/index.js": "module.exports = {};", + }); + + try { + // Simulate what Next.js does: resolve a relative path with explicit paths + const resolved = require.resolve("./node_modules/babel-plugin-react-compiler", { + paths: [dir], + }); + + expect(resolved).toBe(resolve(dir, "node_modules/babel-plugin-react-compiler/dist/index.js")); + } finally { + cleanup(); + } +}); + +test("Module._resolveFilename throws ERR_INVALID_ARG_TYPE if options.paths is not an array", () => { + // Test with string (which is iterable but not an array) + expect(() => { + Module._resolveFilename("path", __filename, false, { paths: "/some/path" }); + }).toThrow(); + + // Test with Set (which is iterable but not an array) + expect(() => { + Module._resolveFilename("path", __filename, false, { paths: new Set(["/some/path"]) }); + }).toThrow(); + + // Test with object (not iterable) + expect(() => { + Module._resolveFilename("path", __filename, false, { paths: { 0: "/some/path" } }); + }).toThrow(); +}); diff --git a/test/no-validate-exceptions.txt b/test/no-validate-exceptions.txt index 67563774a7..62134a7a00 100644 --- a/test/no-validate-exceptions.txt +++ b/test/no-validate-exceptions.txt @@ -26,6 +26,7 @@ test/js/node/test/parallel/test-worker.mjs test/js/node/test/system-ca/test-native-root-certs.test.mjs test/js/node/events/event-emitter.test.ts test/js/node/module/node-module-module.test.js +test/js/node/module/module-resolve-filename-paths.test.js test/js/node/process/call-constructor.test.js test/js/node/stubs.test.js test/js/node/timers/node-timers.test.ts