Skip to content

fix: add CJS require() support to package exports #472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
carlos-alm merged 9 commits into from
Mar 17, 2026
+51 −1
Merged

fix: add CJS require() support to package exports #472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0e4d911
fix: add CJS require() support to package exports (#455)
carlos-alm Mar 16, 2026
1970265
style: use node: protocol for module import in test
carlos-alm Mar 16, 2026
39092fd
test: strengthen CJS export assertion to verify value identity
carlos-alm Mar 16, 2026
483f0bf
test: verify package.json exports map points to CJS wrapper
carlos-alm Mar 16, 2026
cda361f
fix: correct JSDoc in CJS wrapper and add destructuring warning
carlos-alm Mar 16, 2026
a02f519
fix: address Greptile review feedback on CJS exports
carlos-alm Mar 16, 2026
939963c
Merge branch 'main' into fix/cjs-exports-455
carlos-alm Mar 16, 2026
94b84bf
fix: add missing arrow function parens for Biome lint
carlos-alm Mar 16, 2026
491a629
Merge branch 'fix/cjs-exports-455' of https://github.com/optave/codeg…
carlos-alm Mar 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
"main": "src/index.js",
"exports": {
".": {
"import": "./src/index.js"
"import": "./src/index.js",
"require": "./src/index.cjs",
"default": "./src/index.cjs"
},
"./cli": {
"import": "./src/cli.js"
Expand Down
16 changes: 16 additions & 0 deletions src/index.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* CJS compatibility wrapper — delegates to ESM via dynamic import().
*
* This wrapper always returns a Promise on every Node version, because
* import() is unconditionally async. You must always await the result:
*
* const codegraph = await require('@optave/codegraph');
*
* // Named destructuring at require-time does NOT work — always await the full result first.
* // BAD: const { buildGraph } = require('@optave/codegraph'); // buildGraph is undefined
* // GOOD: const { buildGraph } = await require('@optave/codegraph');
*/
// 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');
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 function

buildGraph 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:

Suggested change
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.

Copy link
Contributor Author

@carlos-alm carlos-alm Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added an explicit warning comment that named destructuring at require-time does not work and consumers must always await the result first.

Copy link
Contributor

@greptile-apps greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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');

Copy link
Contributor Author

@carlos-alm carlos-alm Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added a comment documenting the Promise caching behavior.

32 changes: 32 additions & 0 deletions tests/unit/index-exports.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
import { readFileSync } from 'node:fs';
import { createRequire } from 'node:module';
import { dirname, resolve } from 'node:path';
import { fileURLToPath } from 'node:url';
import { describe, expect, it } from 'vitest';

const __dirname = dirname(fileURLToPath(import.meta.url));
const pkg = JSON.parse(readFileSync(resolve(__dirname, '../../package.json'), 'utf8'));

describe('index.js re-exports', () => {
it('package.json exports map points to CJS wrapper', () => {
expect(pkg.exports['.']).toBeDefined();
expect(pkg.exports['.'].require).toBe('./src/index.cjs');
});

it('all re-exports resolve without errors', async () => {
// Dynamic import validates that every re-exported module exists and
// all named exports are resolvable. If any source file is missing,
Expand All @@ -9,4 +21,24 @@ describe('index.js re-exports', () => {
expect(mod).toBeDefined();
expect(typeof mod).toBe('object');
});

it('CJS wrapper resolves to the same exports', async () => {
const require = createRequire(import.meta.url);
const cjs = await require('../../src/index.cjs');
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
const cjs = await require('../../src/index.cjs');
const cjs = await require('@optave/codegraph');

Copy link
Contributor Author

@carlos-alm carlos-alm Mar 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const esm = await import('../../src/index.js');
// Every named ESM export should resolve to a real value, not undefined.
// CJS import() produces a separate module namespace so reference equality
// (toBe) is not possible, but we verify the export exists, is defined,
// and has the same type as its ESM counterpart.
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]);
}
Comment on lines +33 to +37
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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]);
}

Copy link
Contributor Author

@carlos-alm carlos-alm Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — added symmetric key check to verify CJS has no extra keys beyond ESM exports.


// Symmetric check: CJS should not have extra keys beyond ESM exports.
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);
});
});
Loading