fix: subpath exports resolution for CJS files in dual-format packages#227
fix: subpath exports resolution for CJS files in dual-format packages#227RizonSunny wants to merge 1 commit intoyao-pkg:mainfrom
Conversation
robertsLando
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the core idea (using resolvedViaExports instead of isESM to gate exports-aware resolution) is the right fix. However there are a few issues that need to be addressed before merging.
1. rewriteMjsRequirePaths removal — this will break .mjs imports
The removal of rewriteMjsRequirePaths in walker.ts is unrelated to the CJS subpath exports fix and is unsafe. The call chain it supports is:
- ESM
.mjsfiles are transformed to CJS by esbuild →import './foo.mjs'becomesrequire('./foo.mjs') - The packer renames
.mjs→.jsin the snapshot (packer.ts:84-86) rewriteMjsRequirePathsrewritesrequire('./foo.mjs')→require('./foo.js')to match
Without step 3, transformed .mjs files with relative imports to other .mjs files will have dangling require() paths at runtime. Please keep this code.
2. Deduplicate the extensions array
The extensions list ['.js', '.json', '.node', '.mjs', '.cjs'] is now duplicated across three files (walker.ts, follow.ts, resolver.ts). Please export MODULE_RESOLVE_EXTENSIONS from a shared location (e.g. common.ts) and import it in all three places instead of maintaining three independent copies.
3. PR description mentions missing feature
The description says "Handle nested condition objects (e.g., require.default) in synthetic main inference" in walker.ts, but there's no such change in the diff. Please either add the implementation or update the description.
4. Please add a test
This fix needs a test case that covers subpath exports resolution for CJS files in dual-format packages (e.g., a package with an exports field that maps a subpath to a .cjs file). This will prevent regressions and validate the resolvedViaExports flag works as intended.
| import { log, wasReported } from './log'; | ||
| import * as detector from './detector'; | ||
| import { transformESMtoCJS, rewriteMjsRequirePaths } from './esm-transformer'; | ||
| import { transformESMtoCJS } from './esm-transformer'; |
There was a problem hiding this comment.
Please keep the rewriteMjsRequirePaths import and usage — it's needed for .mjs files that get transformed to CJS. The packer renames .mjs → .js in the snapshot, and without this rewrite, require('./foo.mjs') paths generated by esbuild will point to files that no longer exist.
| extensions = [opts.extensions as string]; | ||
| } else { | ||
| extensions = ['.js', '.json', '.node']; | ||
| extensions = ['.js', '.json', '.node', '.mjs', '.cjs']; |
There was a problem hiding this comment.
This extensions array is now duplicated in three places (walker.ts:80, follow.ts:76, resolver.ts:152). Please extract it to a shared constant (e.g. export MODULE_RESOLVE_EXTENSIONS from common.ts) and import it everywhere.
| @@ -149,7 +150,7 @@ export function resolveModule( | |||
| specifier: string, | |||
| options: ResolveOptions, | |||
| ): ResolveResult { | |||
There was a problem hiding this comment.
Same here — import the shared MODULE_RESOLVE_EXTENSIONS constant instead of hardcoding.
Packages like @langchain/langgraph use the "exports" field to map subpaths (e.g., ./prebuilt) to .cjs files. The resolver correctly found these files but discarded the result because it gated on
isESM— which is false for .cjs files. This caused a fallback to the legacy resolver that doesn't understand exports, resulting in MODULE_NOT_FOUND errors.Changes:
resolvedViaExportsflag to distinguish "resolvedvia exports map" from "file is ESM"
resolvedViaExportsinstead ofisESMso CJSfiles resolved through exports are accepted
in synthetic main inference
Testing library: