Skip to content

[CDX-398] Backwards Compatibility Fix for CJS users #480

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

Open
Mudaafi wants to merge 2 commits into
base: master
Choose a base branch
from
+18 −2
Open

[CDX-398] Backwards Compatibility Fix for CJS users #480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6887783
add patch fix
Mudaafi Jul 29, 2026
15f16bb
Route Node to CJS build and append .js to lib/* deep imports
Mudaafi Oct 21, 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
16 changes: 15 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,22 @@
"version": "2.90.0",
"description": "Constructor.io JavaScript client",
"main": "lib/constructorio.js",
"module": "lib/esm/constructorio.js",

@Alexey-Pavlov Alexey-Pavlov Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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": {
Comment thread
Mudaafi marked this conversation as resolved.
".": {
"types": "./lib/types/index.d.ts",

@Alexey-Pavlov Alexey-Pavlov Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe, adding something likenpx @arethetypeswrong/cli --pack . to CI would have caught the 2.90.0 bug too 🤔

https://www.npmjs.com/package/@arethetypeswrong/cli

@Mudaafi Mudaafi Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I considered that actually. I discarded it though, but I'll need to dig up the reason

@constructor-claude-bedrock constructor-claude-bedrock Bot Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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).

"node": "./lib/constructorio.js",

@constructor-claude-bedrock constructor-claude-bedrock Bot Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

"import": "./lib/esm/constructorio.mjs",
Comment thread
constructor-claude-bedrock[bot] marked this conversation as resolved.
Comment thread
Alexey-Pavlov marked this conversation as resolved.
"require": "./lib/constructorio.js",
"default": "./lib/constructorio.js"
},
"./lib/*": "./lib/*.js",

@constructor-claude-bedrock constructor-claude-bedrock Bot Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

"./lib/*.js": "./lib/*.js",
"./lib/*.mjs": "./lib/*.mjs",
"./lib/*.d.ts": "./lib/*.d.ts",
"./lib/*.map": "./lib/*.map",
"./package.json": "./package.json"
},
"scripts": {
"clean": "sudo rm -rf node_modules package-lock.json",
"version": "node ./src/generateVersion.js && npm run docs && git add ./docs/* && npm run bundle && git add -A ./dist && git add ./src/version.js",
Expand Down
4 changes: 3 additions & 1 deletion scripts/build-esm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,7 @@ require('esbuild').build({
define: {
global: 'window',
},
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',
Comment thread
constructor-claude-bedrock[bot] marked this conversation as resolved.

@constructor-claude-bedrock constructor-claude-bedrock Bot Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

}).catch(() => process.exit(1));
Loading