Conversation
|
@codex review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward config-only fix for ESM support.
Extended reasoning...
Overview
This PR fixes broken ESM support by introducing a tsup.config.ts with separate CJS and ESM build configs, updating package.json with proper exports/main/module/types fields pointing to the respective outputs, and bumping the tsconfig.json target from es6 to es2022. No source files are modified.
Security Risks
None. These are build and package metadata config changes only — no auth, crypto, or data-handling logic is touched.
Level of Scrutiny
Low. The change is limited to three config files and follows standard dual-CJS/ESM package conventions. The exports map pattern (nested conditions under require/import) is unconventional but widely used by popular packages and TypeScript bundler tooling.
Other Factors
The one bug found (dead external: ['ky'] in the ESM build config) is a nit with zero runtime impact — it's already covered by an inline comment. No outstanding reviewer concerns remain.
| { | ||
| entry: ['src/index.ts'], | ||
| sourcemap: true, | ||
| external: ['ky'], |
There was a problem hiding this comment.
🟡 The ESM build config in tsup.config.ts includes external: ['ky'], but ky is not imported anywhere in src/ and is absent from both dependencies and devDependencies in package.json. This is dead configuration — a likely leftover from a refactor — and the asymmetry with the CJS build (which has no such entry) could cause a bundling discrepancy if ky is ever added as a dependency.
Extended reasoning...
What the bug is: tsup.config.ts line 17 declares external: ['ky'] for the ESM build only. The external list tells tsup not to bundle that package — instead treating it as a peer/runtime dependency. However, ky is not present anywhere in the codebase.
The specific code path: In the ESM build config object (the second element of the defineConfig array), external: ['ky'] is set. The CJS build config (first element) has no external array at all. The SDK uses native fetch throughout, not ky.
Why existing code does not prevent it: tsup does not validate that externalized packages are actually used in source. It silently accepts any package name and simply excludes it from bundling if encountered during tree-shaking — which never happens here because ky is never imported.
The impact: Currently zero — since ky is never imported, neither build ever encounters it during bundling and the external declaration has no effect. However, the asymmetry is a maintenance hazard: if a developer adds ky as a dependency in the future, the ESM build will correctly leave it external (peer dependency), while the CJS build will bundle it inline. This split behavior would be surprising and hard to debug.
How to fix it: Remove external: ['ky'] from the ESM build config. If ky is intentionally planned as a future dependency, add a comment explaining this and add the same entry to the CJS config to make the behavior symmetric.
Step-by-step proof:
- Search
src/for anykyimport: grep returns zero matches. - Check
package.jsondependenciesanddevDependencies:kyis absent from both. - The ESM build config at line 14–23 of
tsup.config.tscontainsexternal: ['ky']. - The CJS build config at lines 3–13 has no
externalfield. - Conclusion:
external: ['ky']is unreachable dead config —kyis never imported, so tsup never evaluates whether to externalize it. The entry is noise left over from a refactor, and its one-sided presence creates asymmetric build behavior ifkyis added later.
Addressing the refutation: The refutation agrees ky is unused and calls this purely cosmetic. The distinction between cosmetic nit and actionable nit is minor — both conclude the same: remove the dead entry. The future-risk angle (asymmetric bundling if ky is added) is a real maintenance concern, not a hypothetical stretch, making it worth flagging even if currently inert.
2 participants
No description provided.