fix: nest types under import/require in exports maps - #360
Open
OsamaAnsar wants to merge 1 commit into
Open
OsamaAnsar wants to merge 1 commit into
OsamaAnsar wants to merge 1 commit into
Conversation
…base#182) TypeScript's Node16/NodeNext resolver matches exports conditions by JSON key order against the active condition set. With a top-level `types` key sitting as a sibling of `import`/`require`, `types` always won regardless of which module system was actually being resolved - so a CJS consumer's `require` would still get pointed at the ESM `.d.ts`, which then recursively resolves its own imports in ESM mode and produces TS1479 ("this file is CommonJS ... but the referenced file is an ECMAScript module"). Fixes it in both @libsql/client and @libsql/core (the bug affects @libsql/core's own exports map too, and @libsql/client's CJS entry imports from it) by nesting `types` inside each `import`/`require` block instead, and flipping `declaration: false` -> `true` in each package's tsconfig.build-cjs.json so lib-cjs actually has .d.ts files for the `require` condition to point at - the previous shape assumed CJS types could just reuse the ESM ones, which is exactly what broke CJS resolution. Verified against the issue's own reproduction (a Node16-resolution CJS+TS consumer importing `@libsql/client`): `tsc --noEmit` failed with TS1479 before this change and passes after it. A parallel ESM consumer was checked too, to confirm no regression there. The full jest suite is unaffected: same 183 passing / 14 pre-existing failures before and after (those 14 are unrelated Windows temp-dir EPERM issues in the test harness, reproduced identically on unmodified main).
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #182.
The bug
TypeScript's Node16/NodeNext resolver matches
exportsconditions by JSON key order against the currently-active condition set. Both@libsql/clientand@libsql/corehadtypesas a sibling key listed beforeimport/require:Since
typesitself is always one of the active conditions (require, types, nodeorimport, types, node), and it's the first matching key in the object, TypeScript picks it regardless of whether resolution is actually happening in CJS or ESM mode. So a CJS consumer'srequireresolution still lands on the ESM.d.ts, which then recursively resolves its own imports in ESM mode — producing exactly theTS1479error from #182 ("this file is CommonJS ... but the referenced file is an ECMAScript module").This affects
@libsql/coretoo, which matters because@libsql/client's own entry point imports from it — fixing only the client package wasn't enough to fully resolve the repro.The fix
typesinside eachimport/requireblock instead of as a shared sibling, in both packages'exportsmaps (preserving every existing conditional target —workerd/deno/edge-light/etc. mappings are unchanged, just relocated).declaration: false→truein both packages'tsconfig.build-cjs.json, since the newrequire.typesentries point atlib-cjs/*.d.ts, which didn't previously exist.Verification
Reproduced the issue exactly as described: a Node16-resolution CJS+TS consumer importing
@libsql/clientfailstsc --noEmitwithTS1479on currentmain, and passes cleanly after this fix. Checked a parallel ESM consumer too, to confirm no regression on that side (still resolves and typechecks correctly).Ran the full
jestsuite before and after: identical result both times, 183 passing / 14 failing (all 14 are a pre-existing, unrelated Windows-specificEPERMissue in the test harness's temp-directory cleanup — reproduced identically on unmodifiedmain, confirmed viagit stash).