🚀 release: v2.1.1 #19
Conversation
warengonzaga
added
maintainers only
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR releases version 2.1.1, adds dedicated ESM/CJS tsconfig files, and introduces a new dual-build system via the forklift scripts.
- Added
tsconfig.esm.jsonandtsconfig.cjs.jsonfor separate module outputs - Introduced
scripts/forklift.js(core dual-build logic) andscripts/forklift-runner.js(CLI entry) - Updated
package.jsonwith version bump,exportsfield, and new build scripts
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.esm.json | New ESM-targeted TypeScript config |
| tsconfig.cjs.json | New CommonJS-targeted TypeScript config |
| scripts/forklift.js | Core dual-build implementation |
| scripts/forklift-runner.js | CLI runner for the forklift build tool |
| package.json | Version bump, module exports, and script updates |
Comments suppressed due to low confidence (2)
package.json:29
- The root
typesfield points to the ESM declaration, but theexports.require.typespath uses the CJS declaration. Consider aligning the roottypespath or documenting the discrepancy to avoid confusion for TypeScript consumers.
"types": "./dist/esm/index.d.ts",
scripts/forklift.js:96
- [nitpick] Core security helper functions like
validatePath(andvalidateCommand) are critical but untested. Adding unit tests for these functions will help ensure path and command validation remain secure over time.
function validatePath(filePath, basePath = process.cwd()) {
📝 WalkthroughWalkthroughSir, the project’s build system has been refactored to support both CommonJS and ES module outputs, with explicit export mappings and dedicated TypeScript configurations for each format. A custom build orchestration script, “forklift,” has been introduced, accompanied by a CLI runner and enhanced build scripts in Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/forklift-runner.js (1)
46-63: A minor defensive programming enhancement, if I may suggest.The dynamic import assumes the forklift module exports the expected functions. Adding validation would prevent runtime errors if the module structure changes.
import('./forklift.js').then(async (forklift) => { try { + // Validate required exports + const requiredExports = ['validateConfig', 'build', 'watchMode']; + for (const exportName of requiredExports) { + if (typeof forklift[exportName] !== 'function') { + throw new Error(`Missing required export: ${exportName}`); + } + } + forklift.validateConfig(config);scripts/forklift.js (2)
290-310: A minor documentation discrepancy detected, sir.The JSDoc for
addBuildmethod is missing the@param {string} outputPathdocumentation./** * Add build result for a specific format * * @param {string} format - Build format ('esm' or 'cjs') * @param {boolean} success - Whether the build succeeded * @param {number} duration - Build duration in milliseconds + * @param {string} outputPath - Relative path to output directory */ addBuild(format, success, duration, outputPath) {
948-1013: A potential race condition requires attention, sir.The watch mode implementation has robust debouncing, but the
debounceTimeoutandqueuedBuildTimeoutcould leak if the process is interrupted during a pending timeout.Add cleanup on process termination:
// Add after line 1013 process.on('SIGINT', () => { if (debounceTimeout) clearTimeout(debounceTimeout); if (queuedBuildTimeout) clearTimeout(queuedBuildTimeout); console.log('\n👋 Forklift watch mode terminated.'); process.exit(0); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.json(3 hunks)scripts/forklift-runner.js(1 hunks)scripts/forklift.js(1 hunks)tsconfig.cjs.json(1 hunks)tsconfig.esm.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
scripts/forklift-runner.js (1)
scripts/forklift.js (3)
config(198-206)config(1049-1049)success(1055-1055)
🔇 Additional comments (10)
tsconfig.cjs.json (1)
1-14: Sir, the CommonJS configuration appears to be in perfect working order.The TypeScript configuration for CommonJS output is properly structured with appropriate compiler options. The ES2020 target ensures modern JavaScript features while maintaining compatibility, and the separation of output directories (./dist/cjs) provides clear module format distinction.
tsconfig.esm.json (1)
1-14: The ESM configuration is equally pristine, sir.The ES module configuration maintains symmetry with its CommonJS counterpart while properly targeting the ESNext module system. The consistent use of ES2020 as the compilation target across both configurations ensures behavioral parity.
package.json (3)
3-3: Version increment acknowledged, sir.The patch version bump to 2.1.1 is appropriate for this infrastructure enhancement.
27-41: Exemplary dual module configuration, sir.The explicit exports field with conditional paths for both CommonJS and ES modules is precisely configured. The type declarations are correctly mapped for each module format, ensuring proper TypeScript support across consumption patterns.
52-54: Build orchestration successfully modernized.The migration from direct TypeScript compilation to the forklift build system provides enhanced control over the dual module output process.
scripts/forklift.js (5)
1-1: Sir, the shebang is present and accounted for.The executable script declaration ensures proper CLI functionality.
87-182: Exceptional security measures implemented, sir.The path traversal and command injection prevention mechanisms are comprehensive and well-designed. The validation functions properly sanitize inputs and restrict operations to safe boundaries.
525-623: Sir, the ESM import resolution logic is particularly ingenious.The intelligent detection of whether to append
.jsor/index.jsbased on filesystem analysis ensures correct module resolution. The security validations within the import path processing prevent malicious path manipulation.
1088-1180: The CommonJS require statement fixing is symmetrical to the ESM implementation, sir.The parallel implementation for fixing
.cjsextensions in require statements maintains consistency with the ESM import fixing logic.
1182-1182: Export manifest properly configured.The named exports provide a clean API surface for both direct usage and the CLI runner.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/forklift.js (2)
458-463: Dry-run messaging protocols require calibration, Mr. Stark.The success message logic in dry-run mode could be more precise to avoid confusion about actual operations performed.
477-503: Configuration redundancy detected, sir.The inline compiler options duplicate settings that may exist in external tsconfig files, creating maintenance overhead.
🧹 Nitpick comments (3)
scripts/forklift.js (3)
196-281: Manual argument parsing detected, Mr. Stark.While functional, consider leveraging a battle-tested CLI library like
commanderoryargsfor enhanced argument validation, automatic help generation, and improved error messaging.Alternatively, if maintaining the current approach, consider this formatting improvement:
console.log(` -🚀 @wgtechlabs/forklift - Dual Build System +🚀 @wgtechlabs/forklift - Dual Build System -Usage: node scripts/forklift.js [options] +Usage: node scripts/forklift.js [options] -Options: +Options:
574-583: Sophisticated import resolution algorithms detected, sir.The regex patterns comprehensively handle various import syntaxes. Consider extracting these patterns to module-level constants for better maintainability and potential performance gains through pattern reuse.
+// Module-level constants for better performance and maintainability +const ESM_IMPORT_PATTERNS = [ + /from\s+['"](\.[^'"]*?)(?<!\.js|\.mjs|\.json)['"]/g, + /export\s+.*?\s+from\s+['"](\.[^'"]*?)(?<!\.js|\.mjs|\.json)['"]/g, + /import\s*\(\s*['"](\.[^'"]*?)(?<!\.js|\.mjs|\.json)['"]\s*\)/g, + /export\s*\{\s*[^}]*\}\s*from\s+['"](\.[^'"]*?)(?<!\.js|\.mjs|\.json)['"]/g, +]; - const importPatterns = [ - // import ... from './path' - /from\s+['"](\.[^'"]*?)(?<!\.js|\.mjs|\.json)['"]/g, - // export ... from './path' - /export\s+.*?\s+from\s+['"](\.[^'"]*?)(?<!\.js|\.mjs|\.json)['"]/g, - // import('./path') - /import\s*\(\s*['"](\.[^'"]*?)(?<!\.js|\.mjs|\.json)['"]\s*\)/g, - // export { ... } from './path' - /export\s*\{\s*[^}]*\}\s*from\s+['"](\.[^'"]*?)(?<!\.js|\.mjs|\.json)['"]/g, - ]; + const importPatterns = ESM_IMPORT_PATTERNS;
1135-1140: CommonJS require resolution systems operational, sir.The require statement patterns are comprehensive. Similar to the ESM patterns, consider extracting these to module-level constants for consistency and maintainability.
+// Module-level constants for CJS require patterns +const CJS_REQUIRE_PATTERNS = [ + /require\s*\(\s*['"](\.[^'"]*?)(?<!\.cjs|\.json)['"]\s*\)/g, + /require\s*\(\s*["'](\.[^"']*?)(?<!\.cjs|\.json)["']\s*\)/g, +]; - const requirePatterns = [ - // require('./path') - /require\s*\(\s*['"](\.[^'"]*?)(?<!\.cjs|\.json)['"]\s*\)/g, - // require("./path") - /require\s*\(\s*["'](\.[^"']*?)(?<!\.cjs|\.json)["']\s*\)/g, - ]; + const requirePatterns = CJS_REQUIRE_PATTERNS;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/forklift.js(1 hunks)
🔇 Additional comments (15)
scripts/forklift.js (15)
1-73: Excellent documentation, Mr. Stark.The comprehensive JSDoc header with usage examples, feature list, and architecture overview demonstrates exceptional attention to detail. This level of documentation will significantly enhance the developer experience.
75-83: Clean import architecture detected, sir.The ES module imports are properly structured, and the
__filename/__dirnamesetup correctly addresses ES module compatibility requirements.
88-116: Impressive security protocols, sir.The path traversal prevention logic is expertly crafted. The normalization and base path validation provide robust protection against directory traversal attacks.
126-182: Command injection countermeasures are outstanding.The whitelist approach combined with pattern matching creates a solid defense against command injection vulnerabilities. The Windows-specific cmd handling shows excellent cross-platform awareness.
289-377: Exceptional telemetry system, sir.The BuildStats class demonstrates excellent separation of concerns with comprehensive metrics tracking and elegant reporting functionality. The API design is intuitive and the error tracking is thorough.
388-437: Secure command execution protocols engaged, sir.The command validation integration and Windows-specific npx handling demonstrate excellent security-first design and cross-platform compatibility. The promise-based API is clean and robust.
638-658: Package marker generation protocols operational, sir.The CommonJS package marker creation is elegantly implemented with proper validation and dry-run support.
672-719: File transformation protocols executing flawlessly, sir.The recursive CJS file renaming with comprehensive validation demonstrates excellent file system handling and security awareness.
728-740: Temporary configuration isolation protocols engaged, sir.The hardcoded path approach effectively prevents any potential taint from user input, demonstrating excellent security awareness in temporary file handling.
756-860: Build orchestration systems operating at peak efficiency, sir.The format-specific build logic with proper temporary file cleanup and comprehensive error handling demonstrates exceptional engineering. The try-finally pattern ensures resource cleanup regardless of build outcome.
881-938: Primary build orchestration systems fully operational, sir.The main build function demonstrates excellent architectural design with comprehensive validation, proper error handling, and detailed reporting. The logical flow and error recovery mechanisms are exemplary.
949-1014: Advanced monitoring systems online, sir.The watch mode implementation with debouncing, build queuing, and overlap prevention demonstrates sophisticated event handling. The file filtering and change detection logic is exceptionally well-crafted.
1019-1043: Configuration validation protocols confirmed, sir.The comprehensive validation with clear error messaging and format checking provides excellent user feedback and prevents runtime errors.
1048-1075: Error handling and process management systems optimal, sir.The main function with comprehensive error handling, unhandled rejection management, and proper module execution detection demonstrates excellent Node.js best practices.
1183-1183: Module interface systems perfectly configured, sir.The named exports provide a clean and intuitive API for external consumption of the build system's core functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/forklift.js (2)
450-455: Sir, the dry-run mode issue has been excellently resolved.The current implementation properly differentiates between simulated and actual cleanup operations, addressing the previous concern about misleading success messages. The logic now accurately reflects what actually occurred.
471-495: Brilliant refactoring, sir - the configuration duplication concern has been masterfully addressed.The implementation now properly leverages existing tsconfig files rather than duplicating compiler options inline. This creates the desired single source of truth for TypeScript configurations while maintaining build flexibility.
🧹 Nitpick comments (3)
scripts/forklift.js (3)
196-198: Sir, detecting a minor organizational inefficiency in the code structure.The
defaultConfigis referenced here but defined at line 1180, creating a forward reference. Consider moving the default configuration closer to the top of the file for improved readability and logical flow.+// Move this block closer to the top, after imports +const defaultConfig = { + entry: 'src/index.ts', + outDir: 'dist', + formats: ['esm', 'cjs'], + clean: false, + watch: false, + verbose: false, + dryRun: false, +};
747-851: Sir, while the build orchestration is functionally sound, the buildFormat function has grown quite substantial.The 104-line function handles multiple responsibilities including directory creation, TypeScript compilation, and post-processing. Consider decomposing this into smaller, focused functions for enhanced maintainability and testability.
Consider extracting these operations:
runTypeScriptCompilation(format, config, stats)handlePostProcessing(format, outputPath, config, stats)cleanupTempConfig(tempConfigPath, config)
1068-1172: Sir, detecting a minor architectural inconsistency in the function organization.The
fixCjsRequiresfunction is defined aftermain()but called earlier inbuildFormat(). This creates a confusing execution flow. Consider moving this function definition before its first usage for improved code readability.Move this function definition to appear before line 747 where
buildFormatis defined, maintaining logical code organization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
scripts/forklift-runner.js(1 hunks)scripts/forklift.js(1 hunks)tsconfig.cjs.json(1 hunks)tsconfig.esm.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tsconfig.cjs.json
- tsconfig.esm.json
- scripts/forklift-runner.js
🔇 Additional comments (4)
scripts/forklift.js (4)
1-83: Sir, the documentation is exemplary - comprehensive and worthy of Tony Stark's approval.The architectural overview and usage examples provide excellent guidance for developers. The modular import structure demonstrates sophisticated engineering principles.
84-182: Outstanding security protocols, sir - these defensive measures would make even the Mark 50 armor proud.The path traversal prevention and command injection safeguards demonstrate exceptional security engineering. The whitelist approach for commands and comprehensive argument validation create multiple layers of protection.
275-369: Excellent telemetry system, sir - the BuildStats class provides comprehensive performance analytics worthy of the workshop.The class design demonstrates proper encapsulation and provides valuable metrics tracking. The reporting functionality offers clear visibility into build operations and outcomes.
1174-1190: Perfect module architecture, sir - the export structure demonstrates excellent API design principles.The selective export of core functionality while maintaining internal encapsulation creates a clean, maintainable interface. The default configuration provides sensible starting points for all build scenarios.
Summary by CodeRabbit