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 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 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,17 @@
"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

"import": "./lib/esm/constructorio.mjs",

@constructor-claude-bedrock constructor-claude-bedrock Bot Jul 29, 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 "types" condition inside the exports map points to ./lib/types/index.d.ts, which contains export default ConstructorIO (ESM-style). When a TypeScript consumer uses moduleResolution: "node16" or "bundler" and resolves through the "import" condition, TS will correctly find this declaration and expect a .default import. However, when the same map resolves through the "require" condition (which points to a CJS file using module.exports = …), TypeScript still uses the same index.d.ts. This mismatch between the runtime shape (module.exports) and the type declaration (export default) is exactly the ambiguity the author calls out in limitation #2 of the PR description.

The safest fix is to provide a separate types condition per entry-point kind:

"." : {
  "import": {
    "types": "./lib/types/index.d.mts",
    "default": "./lib/esm/constructorio.mjs"
  },
  "require": {
    "types": "./lib/types/index.d.ts",
    "default": "./lib/constructorio.js"
  }
}

Where index.d.mts uses export default and index.d.ts uses export =. This is the pattern recommended in the TypeScript 4.7 docs for dual CJS/ESM packages. The limitation is already noted in the PR description, but shipping it in a known-broken state for node16 ESM consumers without a follow-up ticket creates silent TS errors for those users.

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

There's no node condition here. So import in Node, and also webpack/Next.js server builds now get the browser bundle (platform: 'browser', global replaced with window). Before this PR they got the CJS build, because module was only read by bundlers.
https://github.com/Constructor-io/constructorio-client-javascript/blob/cdx-498-client-js-fix-require-resolution-broken-by-the-2900-esm/src/constructorio.js#L108
typeof window !== "undefined" && window.CLIENT_VERSION

Suggested change
"import": "./lib/esm/constructorio.mjs",
"node": "./lib/constructorio.js",
"import": "./lib/esm/constructorio.mjs",

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

interesting. I would've thought that Node environments would use the Node SDK, but no harm keeping it consistent post-change unless it requires something weird. Good catch

"require": "./lib/constructorio.js",
"default": "./lib/constructorio.js"
},
"./lib/*": "./lib/*",

@constructor-claude-bedrock constructor-claude-bedrock Bot Jul 29, 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: This catch-all pattern restores access to deep paths like ./lib/src/modules/search, but the PR description states (limitation #1) that extension-less deep imports are no longer supported. Because this wildcard maps ./lib/* → ./lib/* literally (no extension appended), consumers who import without an extension (e.g. require('@constructor-io/…/lib/src/modules/search')) will still get MODULE_NOT_FOUND at runtime. The wildcard gives the appearance of support while the limitation remains.

This is worth either:

  1. Documenting explicitly in a CHANGELOG or migration guide for this release, or
  2. Enumerating the most common deep-import paths explicitly so the error is obvious at publish time rather than at consumer run-time.

At minimum, a code comment would help future maintainers understand why the wildcard is there and what it does/does not support.

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

You already listed this as limitation number 1, but I think we can just fix it

Suggested change
"./lib/*": "./lib/*",
"./lib/*": "./lib/*.js",
"./lib/*.js": "./lib/*.js",
"./lib/*.mjs": "./lib/*.mjs",
"./lib/*.d.ts": "./lib/*.d.ts",
"./lib/*.map": "./lib/*.map",

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

huh. we'll have to consider other extensions as well like .css but I'll dig into it

"./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',

@constructor-claude-bedrock constructor-claude-bedrock Bot Jul 29, 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 comment above this line is clear and well-written. One small addition would be to also note that any consumers who previously referenced lib/esm/constructorio.js (e.g. direct CDN links, test helpers, or legacy module field users) will now get a 404/MODULE_NOT_FOUND. Since lib/ is included in the published files array this is a published path change; a brief note here (or in CHANGELOG) would help teams bisect breakage. Low severity since the exports map already routes bundlers correctly.

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