Configure package for npm publishing with dual ESM/CJS output - #5
Conversation
|
Claude finished @stevekrenzel's task —— View job PR Review CompleteTasks
Overall AssessmentThis PR successfully configures the package for npm publishing with a clean, straightforward implementation. The changes are well-structured and the dual ESM/CJS output setup is solid. However, there are several documentation and code quality issues that need to be addressed to meet the Intent library's strict engineering standards. Summary: ✅ Functional and correct | Critical Issues1. Missing JSDoc Documentation (scripts/build.mjs)Severity: High - Violates core requirement that "every function (public and private) must have JSDoc" All functions have basic JSDoc comments, but they lack the required detail and formatting standards specified in CLAUDE.md:
Recommendation: Add comprehensive JSDoc to all functions following the standards in CLAUDE.md. Documentation should explain "why" not just "what", include edge cases, and use complete sentences. Moderate Issues2. Single-Line If Statement Without Braces (scripts/build.mjs:82)Severity: Moderate - Directly violates "Always use braces: No single-line if statements" Location: scripts/build.mjs:82 main();Actually, upon closer inspection, there are no single-line if statements without braces in the code. This is correct. ✅ 3. Console.log Statements (scripts/build.mjs)Severity: Low-Moderate - The checklist states "No console.log statements" Locations: Lines 34, 42, 49, 61, 75, 77 The build script uses Analysis: This is actually appropriate for a build script. The "no console.log" rule applies to library code, not build/development tooling. Build scripts should provide feedback about their progress. Recommendation: These console statements are acceptable. Consider this a false positive from the checklist. ✅ 4. Error Handling Could Be More Specific (scripts/build.mjs:76-79)Severity: Low Location: scripts/build.mjs:76-79 } catch (error) {
console.error("Build failed:", error);
process.exit(1);
}Issue: The catch block uses implicit Recommendation: Follow test error handling pattern from CLAUDE.md: } catch (error) {
console.error(
"Build failed:",
error instanceof Error ? error.message : String(error)
);
process.exit(1);
}This ensures proper type safety and consistent error message formatting. Minor Issues / Observations5. package.json ConfigurationObservations:
Note: The Recommendation (optional): Consider adding documentation files to the "files": [
"dist",
"README.md",
"LICENSE"
],6. TypeScript ConfigurationObservations:
No issues found in tsconfig.json. 7. Build Script StructureObservations:
Strong points:
Testing ConsiderationsQuestion: Should the build script have tests? The build script is not covered by tests (build scripts are typically not unit tested). However, the PR should verify:
Recommendation: While not blocking for merge, consider adding an integration test or manual verification checklist in the PR description confirming the build outputs work as expected. Security Review
Checklist Against StandardsBased on the "Final Checklist" from the review instructions:
Required Actions Before Merge
Strengths of This PR
ConclusionThis is a solid, functional PR that correctly implements npm publishing configuration. The main gap is documentation quality—specifically, JSDoc comments need to meet the library's comprehensive standards. Once the documentation is enhanced and the minor error handling improvement is made, this will be ready to merge. Estimated effort to address: ~15 minutes to enhance JSDoc and improve error handling. |
…tput ## Summary - Rename package from `intent` to `@with-logic/intent` for scoped npm publishing - Add custom esbuild-based build script producing dual ESM (`.mjs`) and CJS (`.cjs`) bundles - Configure package.json with proper npm publishing metadata and export maps ## Problem The library needed to be prepared for public npm distribution with proper: - Scoped package naming (`@with-logic/intent`) - Dual module format support for both ESM and CommonJS consumers - Correct export configuration for modern bundlers and Node.js ## Solution **Package Configuration:** - Set `private: false` with `publishConfig.access: public` for scoped publishing - Add comprehensive metadata (keywords, repository, bugs, homepage) - Configure dual exports via `main` (CJS), `module` (ESM), `types`, and conditional `exports` map - Add `sideEffects: false` for tree-shaking optimization - Include `prepublishOnly` and `release` scripts for safe publishing workflow **Build System:** - New `scripts/build.mjs` using esbuild for fast bundling - Produces `dist/index.mjs` (ESM) and `dist/index.cjs` (CommonJS) - TypeScript declarations generated via `tsc --emitDeclarationOnly` - Externalized dependencies (`groq-sdk`, `dotenv`) to avoid bundling - Targets Node 18+ **TypeScript Configuration:** - Updated `moduleResolution` to `Bundler` for modern tooling compatibility - Added `verbatimModuleSyntax` for explicit import/export type handling - Removed `esModuleInterop` (not needed with verbatim module syntax) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
2c359ad to
8bda76d
Compare
Summary
intentto@with-logic/intentfor scoped npm publishing.mjs) and CJS (.cjs) bundlesProblem
The library needed to be prepared for public npm distribution with proper:
@with-logic/intent)Solution
Package Configuration:
private: falsewithpublishConfig.access: publicfor scoped publishingmain(CJS),module(ESM),types, and conditionalexportsmapsideEffects: falsefor tree-shaking optimizationprepublishOnlyandreleasescripts for safe publishing workflowBuild System:
scripts/build.mjsusing esbuild for fast bundlingdist/index.mjs(ESM) anddist/index.cjs(CommonJS)tsc --emitDeclarationOnlygroq-sdk,dotenv) to avoid bundlingTypeScript Configuration:
moduleResolutiontoBundlerfor modern tooling compatibilityverbatimModuleSyntaxfor explicit import/export type handlingesModuleInterop(not needed with verbatim module syntax)🤖 Generated with Claude Code