-
-
Notifications
You must be signed in to change notification settings - Fork 73
Node: Fix CommonJS packaging in @herb-tools/node #375
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
base: main
Are you sure you want to change the base?
Conversation
Fixes failures when loading the CommonJS entry of @herb-tools/node while keeping the ESM entry working correctly. Fix CommonJS entry by standardizing dual-format packaging approach Adopt the single-source pattern used consistently across @herb-tools/core, @herb-tools/formatter, @herb-tools/highlighter, and @herb-tools/linter. Replace separate .cts/.mts source files with a single src/index.ts that Rollup transforms into both ESM and CJS outputs. Align @herb-tools/node with the established monorepo pattern to ensure consistency, maintainability, and reliability. Preserve the existing single-entry approach with no public API changes.
1ad4f6b to
1c7f6d3
Compare
|
Hey @fc-anjos, thanks for this pull request! It's something that annoyed me too but that I ended up having to do so I could bundle it the right way in the dependent packages. Sadly, I'll have to sit on this one for a bit before I get to play with changing the bundling foundation again! Thank you! |
|
Hey @marcoroth! Of course, thank you for the important work done on this and the other packages. What I am considering a bug:This PR is based on the assumption that this is a bug:
Evidence# Build the package
npm run build
# Test CJS import (fails)
node -e "const { Herb } = require('./dist/herb-node.cjs'); console.log('CJS works')"
# Error: Cannot find module './node-backend'
# Test ESM import (works)
node -e "import('./dist/herb-node.esm.js').then(m => console.log('ESM works'))"...but I'm not sure how changing this file could break things downstream Root Cause, as discussed earlierThe CJS build fails because:
Previous Solution and your concernsThe original PR proposed consolidating to a single source file, which would fix the CJS issue but you mentioned concerns about bundling it the right way in the dependent packages. This suggests there are specific bundling requirements that need to be preserved. Alternative Solution: Minimal CJS FixInstead of changing the bundling foundation, as an alternative, perhaps a minimal fix that would maintain the separate file structure is more adequate: // src/index-cjs.cts
- const { HerbBackendNode } = require("./node-backend.js")
+ const { Visitor, HerbBackend } = require("@herb-tools/core")
+
+ // Inline the HerbBackendNode class to avoid bundling issues
+ const packageJSON = require("../package.json")
+
+ class HerbBackendNode extends HerbBackend {
+ constructor(backendPromise: any) {
+ super(backendPromise)
+ }
+
+ backendVersion() {
+ return `${packageJSON.name}@${packageJSON.version}`
+ }
+ }
+
+ - const Herb = new HerbBackendNode(
+ - new Promise((resolve, _reject) => resolve(libHerbBinary)),
+ - )
+ + const Herb = new HerbBackendNode(
+ + () => new Promise((resolve, _reject) => resolve(libHerbBinary)),
+ + )This approach maintains the separate file structure while fixing the CJS bundling issue, although it introduces a duplication for the HerbBackendNode function. Bundling ConcernsYou mentioned concerns about bundling it the right way in the dependent packages. If you're open to, I'd be happy to properly address these.
This would allow for safer refactoring and clearer expectations about how the package integrates with various build systems, and I can work on those if needed. This would help ensure the CJS issue is fixed without disrupting the existing bundling setup. Let me know and I can push a commit with this focused fix on the Thank you! |
Node: Standardize dual-format packaging to match established codebase patterns
This Pull Request fixes the packaging for CJS consumers of the Node package.
Previously, the CommonJS entry failed due to a Rollup bundling configuration issue in
@herb-tools/node. The package used separate.cts/.mtssource files, but Rollup wasn't properly bundling local dependencies for the CJS build, leaving runtime requires that failed.This change adopts the established pattern: a single
src/index.tssource file that Rollup transforms into both ESM and CJS outputs. This matches the proven strategy used by@herb-tools/core,@herb-tools/formatter,@herb-tools/highlighter, and@herb-tools/linter.This change adopts the established pattern: a single
src/index.tssource file that Rollup transforms into both ESM and CJS outputs. This matches the strategy used by@herb-tools/core,@herb-tools/formatter,@herb-tools/highlighter, and@herb-tools/linter.No public API changes.
Expected vs Actual
@herb-tools/node, callawait Herb.load(), and use the API.Reproduction (before)
Using the built artifacts, with the previous split
.mts/.ctssources:Run:
node test-esm.mjs # ✓ Loads and runs successfullyRun:
node test-cjs.cjs # ✗ Error: Cannot find module './node-backend' (from dist/herb-node.cjs)Even with the path changed, there'd still be the error:
Diagnosis (before)
CJS build problem:
src/index-cjs.ctscontainedrequire("./node-backend.js")dist/herb-node.cjsexpected./node-backend.jsto exist at runtime, but it wasn't emittednew Promise(...)(Promise instance) instead of() => new Promise(...)(Promise factory)ESM build worked because:
node-backend.tsdependency() => new Promise(...)Fix
Adopt the standard, project-wide packaging pattern:
src/index.tsfor both formats..cts/.mtssources to avoid divergence and bundling issues.package.jsontype entries to the standard./dist/types/index.d.ts.This mirrors the approach in other packages and eliminates the non-working CJS path.
Files Changed
src/index-esm.mts→src/index.ts(renamed, now single source)src/index-cjs.cts(deleted, was non-working code path)rollup.config.mjs(updated to use standard pattern)package.json(updated type declarations)node.test.ts(update import path)