Fix #245 CJS resolution failure #346

seratch · 2025-08-15T13:31:10Z

This pull request adds the integration test correction on top of @CharlieGreenman's amazing contribution #258; it resolves #245

You can verify this PR resolves the CJS issue by the following steps:

Step 1: Before merging the changes in this PR; follow the steps at https://github.com/openai/openai-agents-js/blob/main/integration-tests/README.md the test fails this way:

 FAIL  integration-tests/node.test.ts > Node.js > should be able to run using CommonJS
ExecaError: Command failed with exit code 1: npm run 'start:cjs'

(node:67335) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
/Users/seratch/code/openai-agents-js/integration-tests/node/node_modules/@openai/agents/dist/index.js:1
import { setDefaultModelProvider } from '@openai/agents-core';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at wrapSafe (node:internal/modules/cjs/loader:1472:18)
    at Module._compile (node:internal/modules/cjs/loader:1501:20)
    at Module._extensions..js (node:internal/modules/cjs/loader:1613:10)
    at Module.load (node:internal/modules/cjs/loader:1275:32)
    at Module._load (node:internal/modules/cjs/loader:1096:12)
    at Module.require (node:internal/modules/cjs/loader:1298:19)
    at require (node:internal/modules/helpers:182:18)
    at Object.<anonymous> (/Users/seratch/code/openai-agents-js/integration-tests/node/index.cjs:9:5)
    at Module._compile (node:internal/modules/cjs/loader:1529:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1613:10)

Node.js v20.19.4


> start:cjs
> node --no-experimental-require-module index.cjs

Step 2: With the changes in this PR, all the integration tests pass.

vrtnis · 2025-08-15T20:21:48Z

Quick thought - noticed tsc-multi.json now flips the .js target to CJS and keeps ESM as .mjs. That effectively makes .mjs the public ESM surface for the packages, right?

Just to better understand the trajectory here, would there be breakage we’d expect for projects that: rely on "type":"module" and therefore expect ESM at .js, and/or deep‑import files under dist/*.js (not ideal of course, but we may see it in the wild).

Also re: exports, with .js now CJS and .mjs ESM, will each package’s exports map (incl. subpaths like ./realtime, ./utils, ./_shims/*) be updated to route import -> *.mjs and require -> CJS *.js? And we won’t keep "type":"module" in packages where .js is CJS?

For comparison, counterpart tsc-multi approach in draft #333 keeps ESM as .js in dist/ and adds a CJS dist-cjs/ with exports.import/require wiring, plus the same loadEnv() eval(import.meta) guard.

Apologies for the many questions, I’m happy to rebase #333 as I draft it on top of #346 - just wanted to clarify these tsc-multi specific points.

seratch · 2025-08-15T13:31:53Z

integration-tests/node/package.json

  "scripts": {
-    "start:cjs": "node index.cjs",
-    "start:esm": "node index.mjs"
+    "start:cjs": "node --no-experimental-require-module index.cjs",


this option is necessary to turn the esmodule interop option off

got it! sounds good :)

seratch · 2025-08-15T13:32:09Z

integration-tests/node.test.ts

+  cwd: './integration-tests/node',
+  env: {
+    ...process.env,
+    NODE_OPTIONS: '',


remove all the pre-set node options too

yep, makes sense ..will prevent hidden loader/interop from leaking in

seratch · 2025-08-15T13:32:38Z

tsc-multi.json

@@ -1,6 +1,6 @@
 {
  "targets": [
-    { "extname": ".js", "module": "es2022", "moduleResolution": "node" },
+    { "extname": ".js", "module": "commonjs", "moduleResolution": "node" },


*.js files are CJS compat; *.mjs are ESModule compat

the .js ---> CJS / .mjs --> ESM flip is clear. wondering if will we align each packages/* exports (incl. subpaths) so import --> *.mjs, require --> *.js, and drop "type":"module" where .js is CJS? If top-level only is intended, all good!

Yes, exactly. I've revised package.json files to be simpler and clearer to use "require" and "import". Verified the behavior with the integration tests, plus did additional manual tests with a simple project too.

seratch · 2025-08-15T23:10:26Z

tsc-multi.json

@@ -1,6 +1,6 @@
 {
  "targets": [
-    { "extname": ".js", "module": "es2022", "moduleResolution": "node" },
+    { "extname": ".js", "module": "commonjs", "moduleResolution": "node" },


Yes, exactly. I've revised package.json files to be simpler and clearer to use "require" and "import". Verified the behavior with the integration tests, plus did additional manual tests with a simple project too.

seratch · 2025-08-15T23:10:52Z

packages/agents-core/package.json

@@ -14,67 +14,49 @@
  },
  "exports": {
    ".": {
-      "require": {


no need to repeat types and default under require; simplified much

seratch · 2025-08-15T23:11:48Z

packages/agents-core/package.json

      "types": "./dist/index.d.ts",
-      "default": "./dist/index.mjs"
+      "require": "./dist/index.js",
+      "import": "./dist/index.mjs"


"default" works too but "import" works with clearer intention with ESModule

seratch · 2025-08-15T23:13:01Z

packages/agents-realtime/package.json

      },
-      "types": "./dist/shims/shims-node.d.ts",
-      "default": "./dist/shims/shims-node.mjs"
+      "types": "./dist/shims/shims.d.ts",


shims/shims is just a reference to shims/shims-node, but there is no downside of this change

seratch · 2025-08-15T23:17:55Z

@vrtnis

Quick thought - noticed tsc-multi.json now flips the .js target to CJS and keeps ESM as .mjs. That effectively makes .mjs the public ESM surface for the packages, right?

Yes, you're right.

Just to better understand the trajectory here, would there be breakage we’d expect for projects that: rely on "type":"module" and therefore expect ESM at .js, and/or deep‑import files under dist/*.js (not ideal of course, but we may see it in the wild).

We basically do not support deep-imports. When a user needs some additional exports, we're happy to add exports of sub directories and/or export those modules under existing exports.

Also re: exports, with .js now CJS and .mjs ESM, will each package’s exports map (incl. subpaths like ./realtime, ./utils, ./_shims/*) be updated to route import -> *.mjs and require -> CJS *.js? And we won’t keep "type":"module" in packages where .js is CJS?

Yes, this is correct.

For comparison, counterpart tsc-multi approach in draft #333 keeps ESM as .js in dist/ and adds a CJS dist-cjs/ with exports.import/require wiring, plus the same loadEnv() eval(import.meta) guard.

I don't see the necessity to have dist and dist-cjs. However, if there is anything to add on top of this PR, I am happy to check your PRs!

vrtnis · 2025-08-15T23:26:16Z

@seratch --thanks so much for the detailed explanation and context here, all sounds great :)

changeset-bot · 2025-08-16T00:28:50Z

⚠️ No Changeset found

Latest commit: c894a8e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

seratch · 2025-08-16T00:32:53Z

Did more tests and this should work well for both CJS and ESModule projects

CharlieGreenman and others added 2 commits August 15, 2025 22:20

issue-245: fix CommonJS interoperability in tsc-multi and shim files

27ba1ed

Correct CJS tests

025c769

seratch self-assigned this Aug 15, 2025

seratch added bug Something isn't working package:agents-core package:agents-realtime package:agents-openai package:agents-extensions labels Aug 15, 2025

seratch requested a review from dkundel-openai August 15, 2025 13:32

This was referenced Aug 15, 2025

issue-245: fix CommonJS interoperability in tsc-multi and shim files #258

Closed

feat: add dual esm + cjs build with shim guard, updated exports, and lambda support #333

Closed

seratch commented Aug 15, 2025

View reviewed changes

Streamline package.json files

6838603

seratch commented Aug 15, 2025

View reviewed changes

Fix

c894a8e

seratch merged commit 8c4e116 into main Aug 16, 2025
5 checks passed

seratch deleted the CharlieGreenman-issue-245-branch-commonjs-interop branch August 16, 2025 00:33

seratch added a commit that referenced this pull request Aug 16, 2025

Add changeset for #346

a3afa2d

seratch mentioned this pull request Aug 16, 2025

Add changeset for #346 #348

Merged

seratch added a commit that referenced this pull request Aug 16, 2025

Add changeset for #346 (#348)

5d247a5

Fix #245 CJS resolution failure #346

Fix #245 CJS resolution failure #346

Conversation

seratch commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrtnis commented Aug 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seratch commented Aug 15, 2025

Uh oh!

vrtnis commented Aug 15, 2025

Uh oh!

changeset-bot bot commented Aug 16, 2025

⚠️ No Changeset found

Uh oh!

seratch commented Aug 16, 2025

Uh oh!

Uh oh!

Uh oh!

seratch commented Aug 15, 2025 •

edited

Loading