-
Notifications
You must be signed in to change notification settings - Fork 7
build: add file extensions to ESM imports #139
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.
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
Conversation
- Replace TypeScript compiler with esbuild for ESM build - Add esbuild-plugin-file-path-extensions to generate proper .mjs imports - Fix "Cannot find module" errors in Node.js ESM resolution - Ensure published packages work correctly in production environments resolves: trufnetwork/truf-network#1205
WalkthroughAdds a new Node.js build script to produce an ESM distribution (.mjs) using esbuild, writes a module-typed package.json in dist-esm, and post-processes imports to include .mjs. Updates package.json to point to .mjs outputs, adjusts exports, files, scripts, and adds build-time dev dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev
participant Script as build-esm.js
participant Glob as glob
participant ESB as esbuild
participant FS as fs
participant Proc as process
Dev->>Script: node build-esm.js
Script->>FS: clean/create dist-esm
Script->>Glob: find src/**/*.ts entry points
Glob-->>Script: entry list
Script->>ESB: build({format: esm, outExtension: .mjs, plugins})
ESB-->>Script: output files in dist-esm
Script->>FS: write dist-esm/package.json {"type":"module"}
loop post-process outputs
Script->>FS: read .mjs file
Script->>Script: rewrite relative imports to *.mjs
Script->>FS: write .mjs file
end
Script-->>Dev: log success ✅
alt on error
Script->>Proc: console.error + exit(1)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Time Submission Status
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
24-32: Don’t publish ESM test artifacts; drop unused pattern.You exclude .test.js but not .test.mjs under dist‑esm. Also, “dist-esm/**/*.js” is no longer needed if you only emit .mjs.
"files": [ "dist/**/*.js", "dist/**/*.d.ts", - "dist-esm/**/*.js", "dist-esm/**/*.mjs", "dist-esm/**/*.d.ts", "dist-esm/package.json", "!dist/**/*.map", "!dist/**/*.test.js", "!dist/**/*.test.ts", "!dist-esm/**/*.map", + "!dist-esm/**/*.test.mjs",
🧹 Nitpick comments (4)
build-esm.js (3)
18-31: Tune esbuild target and output base for Node 18 ESM.Minor polish: use platform "node" and target "node18"; set outbase to keep stable paths derived from src/.
await esbuild.build({ entryPoints, bundle: false, // Don't bundle - keep separate files outdir: './dist-esm', - format: 'esm', - target: 'es2021', - platform: 'neutral', + format: 'esm', + target: 'node18', + platform: 'node', sourcemap: false, + outbase: 'src', outExtension: { '.js': '.mjs' },
27-31: Avoid double‑touching specifiers (plugin + post‑process).The plugin already rewrites specifiers; keep the post‑process as a safety net, but ensure it’s idempotent (see prior diff) and consider removing the plugin or the fixer to reduce drift.
Also applies to: 36-76
8-11: Nit: centralize output dir and use path utilities.Small safety/readability tweak.
- if (fs.existsSync('./dist-esm')) { - fs.rmSync('./dist-esm', { recursive: true, force: true }); - } - fs.mkdirSync('./dist-esm', { recursive: true }); + const path = require('path'); + const outDir = path.resolve('./dist-esm'); + if (fs.existsSync(outDir)) fs.rmSync(outDir, { recursive: true, force: true }); + fs.mkdirSync(outDir, { recursive: true });package.json (1)
36-39: Minor: avoid double clean of dist‑esm.Since build‑esm.js already removes dist‑esm, consider simplifying clean or the script to have a single owner of deletion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
build-esm.js(1 hunks)package.json(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: MicBun
PR: trufnetwork/kwil-js#137
File: package.json:36-38
Timestamp: 2025-09-18T12:49:31.799Z
Learning: In the trufnetwork/kwil-js project, changing tsconfig.esm.json from "module": "ES2020" with "moduleResolution": "node" to "module": "NodeNext" with "moduleResolution": "NodeNext" causes issues for the user MicBun. The current ES2020 configuration is preferred for their specific use case.
📚 Learning: 2025-09-18T12:49:31.799Z
Learnt from: MicBun
PR: trufnetwork/kwil-js#137
File: package.json:36-38
Timestamp: 2025-09-18T12:49:31.799Z
Learning: In the trufnetwork/kwil-js project, changing tsconfig.esm.json from "module": "ES2020" with "moduleResolution": "node" to "module": "NodeNext" with "moduleResolution": "NodeNext" causes issues for the user MicBun. The current ES2020 configuration is preferred for their specific use case.
Applied to files:
build-esm.jspackage.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
package.json (2)
6-13: Exports alignment looks good.import points to .mjs, require to CJS, and types to .d.ts. This should satisfy both ESM and CJS consumers.
66-72: Verified: devDependency versions exist and no direct advisories found.esbuild@^0.25.10, esbuild-plugin-file-path-extensions@^2.1.4, glob@^11.0.3 resolve on npm and show no direct advisories in Snyk/npm listings. (npmjs.com)
GHSA-67mh-4wv8-2f99 (esbuild dev-server CORS) was patched in 0.25.0 — the specified range (>=0.25.10) is unaffected. Scanners may still flag esbuild prebuilt binaries for related Go CVEs (e.g., CVE‑2025‑22874); if your CI reports issues, confirm the binaries were rebuilt or upgrade to a fixed esbuild release. (github.com)
build-esm.js (1)
77-81: Add post-build verifier for dist‑esm (fail on .js imports / missing extensions / test files)Insert a post-build check immediately after the success log in build-esm.js that scans dist-esm/*.mjs for: (1) relative imports missing .mjs/.js, (2) relative imports pointing to .js (should be .mjs), and (3) emitted test/spec files — exit non‑zero if any matches. Run locally after
pnpm run build(dist-esm was not present here so I couldn't verify).
resolves: https://github.com/trufnetwork/truf-network/issues/1205
Summary by CodeRabbit
New Features
Chores