Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts the published entrypoints to restore CommonJS require() behavior after an earlier ESM packaging change inadvertently made CJS consumers receive { default: ... }. The PR does this by switching to conditional exports in package.json and emitting the ESM bundle as .mjs so ESM can be supported without flipping the entire package to "type": "module".
Changes:
- Emit the ESM build as
lib/esm/constructorio.mjsso Node treats it as ESM without changing package-wide module classification. - Replace the
modulefield with conditionalexportsto routeimportto the ESM bundle andrequireto the CJS entrypoint. - Add an export pattern for
./lib/*and explicitly export./package.json.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/build-esm.js | Changes ESM build output from .js to .mjs to ensure correct ESM detection without "type": "module". |
| package.json | Introduces conditional exports to resolve CJS vs ESM entrypoints correctly and removes the module field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Alexey-Pavlov
left a comment
There was a problem hiding this comment.
@Mudaafi Thanks for working on this! Left a few comments, please take a look when you get a chance
Analyzing this PR is quite challenging because there are a lot of different cases, but I think we can fix limitations 1 and 3
| "version": "2.90.0", | ||
| "description": "Constructor.io JavaScript client", | ||
| "main": "lib/constructorio.js", | ||
| "module": "lib/esm/constructorio.js", |
There was a problem hiding this comment.
Modern bundlers ignore the module when exports match, so keeping it changes nothing for them. It only helps old tooling, which just reads the fields and now falls back to CJS
Can we just put it back as
"module": "lib/esm/constructorio.mjs"?
Then limitation #3 is gone then
| "types": "lib/types/index.d.ts", | ||
| "exports": { | ||
| ".": { | ||
| "types": "./lib/types/index.d.ts", |
There was a problem hiding this comment.
Maybe, adding something likenpx @arethetypeswrong/cli --pack . to CI would have caught the 2.90.0 bug too 🤔
There was a problem hiding this comment.
I considered that actually. I discarded it though, but I'll need to dig up the reason
- Add a `node` condition ahead of `import` so Node and node-targeted bundler builds (SSR) resolve to the CJS build instead of the browser ESM bundle, where `global` is rewritten to `window`. - Map `./lib/*` to `./lib/*.js` so extension-less deep imports resolve again, with passthrough patterns for the extensions published under lib/ (.js, .mjs, .d.ts, .map). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This PR fixes a CJS backwards-compatibility regression by replacing the deprecated module field with a proper exports map and renaming the ESM output to .mjs. The approach is technically sound and the inline comment in build-esm.js is appreciated, but there are a few important issues worth addressing.
Inline comments: 4 discussions added
Overall Assessment:
| "require": "./lib/constructorio.js", | ||
| "default": "./lib/constructorio.js" | ||
| }, | ||
| "./lib/*": "./lib/*.js", |
There was a problem hiding this comment.
Important Issue: The wildcard subpath export "./lib/*": "./lib/*.js" maps all extension-less deep imports to .js files. This silently breaks consumers that do extension-less CJS deep imports like require('@constructor-io/constructorio-client-javascript/lib/src/modules/search') — they will now resolve to ./lib/src/modules/search.js only if that exact .js file exists in lib/, but consumers who previously relied on Babel-compiled files without extensions will see MODULE_NOT_FOUND. The PR description acknowledges limitation #1 but marks it as accepted; however, this wildcard pattern could be removed entirely (forcing users to use explicit extensions) rather than giving false hope that extension-less imports still work. At minimum, add a comment here explaining the intentional trade-off so future maintainers understand why it's structured this way.
| "exports": { | ||
| ".": { | ||
| "types": "./lib/types/index.d.ts", | ||
| "node": "./lib/constructorio.js", |
There was a problem hiding this comment.
Important Issue: The "node" condition appears before "import" and "require" in the exports map. In the Node.js exports resolution algorithm, conditions are evaluated in the order they appear in the object. Because "node" matches both CJS and ESM Node.js environments, it will always win for Node consumers — meaning the "import" condition directly below it will never be reached for Node ESM (import statement) users. If the intent is to serve the .mjs file to Node ESM consumers, "node" should either be removed (the "import" / "require" split is sufficient) or moved after "import" so that ESM Node users get the proper .mjs bundle. As-is, Node ESM users (import ... from '@constructor-io/constructorio-client-javascript') will receive the CJS .js file instead of the .mjs file, which may work via Node's CJS interop but defeats the purpose of building the .mjs artifact.
| "types": "lib/types/index.d.ts", | ||
| "exports": { | ||
| ".": { | ||
| "types": "./lib/types/index.d.ts", |
There was a problem hiding this comment.
Suggestion: The PR description mentions (limitation #2) that TypeScript consumers using moduleResolution: "node16" or "bundler" may encounter issues because the JS uses module.exports while the types use export default. For moduleResolution: "node16", TypeScript requires separate type entries for each condition — e.g., a "types" entry alongside the "require" condition and a separate one alongside "import". Consider expanding this to:
".": {
"import": {
"types": "./lib/types/index.d.mts",
"default": "./lib/esm/constructorio.mjs"
},
"require": {
"types": "./lib/types/index.d.ts",
"default": "./lib/constructorio.js"
}
}This is a known best-practice for dual CJS/ESM packages with TypeScript support. If splitting .d.ts / .d.mts is out of scope for this PR, at least track it as a follow-up task (the PR description does list it, which is good).
| outfile: './lib/esm/constructorio.js', | ||
| // .mjs so Node classifies this as ESM without `"type": "module"`, which would | ||
| // reclassify every .js in the package and break CJS consumers. | ||
| outfile: './lib/esm/constructorio.mjs', |
There was a problem hiding this comment.
Suggestion: The sourcemap: true option in this build step generates constructorio.mjs.map. The new exports map includes "./lib/*.map": "./lib/*.map" to expose source maps, which is intentional. However, the build-esm.js script no longer generates constructorio.js (it now generates constructorio.mjs), so any tooling or CI steps that previously referenced lib/esm/constructorio.js (e.g., the old "module" field consumers, integration tests, or the test:bundled:esm npm script which bundles via build-test-iife.js) should be verified to still work. The test:bundled:esm script in package.json runs bundle-esm.js, which builds from source — not from lib/esm/ — so that path appears fine. But it's worth double-checking that no other script or CI step hardcodes the old .js output path.
When we released the ESM bundled version earlier, we unintentionally made it the default export. This resulted in CJS environments breaking when they upgraded to the newer minor version since
requirenow returns{ default: xxx }.This patch fix removes the typical ESM-structure in package.json in favor of
exportswhich allows us to retain the typicalrequirepattern while also supporting the usualimportpattern for ESM. There are limitations however, namely:Extension-less deep imports are no longer supported
.jsShared types declaration is ambiguous and might fail for
node16ESM consumersmodule.exportsbut the types useexport default, TypeScript under moduleResolution: "node16" might think .default is required when it isn't..tsand.mtsOlder bundlers that do not support
exportswill not work for ESM environments since package.json no longer has themodulefield