Skip to content

Conversation

@narekhovhannisyan
Copy link
Collaborator

@narekhovhannisyan narekhovhannisyan commented Oct 30, 2025

Motivation

Installing the package directly from a Git URL didn’t include the built dist/ output, breaking imports. We need the package to self-build on install (for Git installs) and to build only on publish for the npm registry. Also, we want a single source of truth for the build command to avoid drift.

Fixes #82

Changes

  • Added a build script: rm -rf dist && tsc --project tsconfig.build.json
  • Switched to prepare to build on install (covers Git URL installs)
  • Replaced legacy prepublish with prepublishOnly to build only on npm publish
  • Pointed both prepare and prepublishOnly to the build script (DRY)

How to test

  • Install from GitHub URL and verify dist/ exists:
    - rm -rf node_modules && npm i github:railsware/mailtrap-nodejs#fix-#84
    - Check node_modules/mailtrap/dist is present
    - Import it in a small script and run it (e.g., node -e "require('mailtrap')")

  • Verify publish-time build path:
    - Run npm run prepublishOnly manually and confirm dist/ is built
    - Optionally npm publish --dry-run and confirm build occurs without publishing

Summary by CodeRabbit

  • Chores
    • Improved build process and development workflow configuration.

@coderabbitai
Copy link

coderabbitai bot commented Oct 30, 2025

Walkthrough

The pull request updates the package.json build configuration by adding rimraf as a devDependency, replacing shell-based cleanup (rm -rf) with the cross-platform rimraf utility in the build script, and introducing prepare and prepublishOnly lifecycle scripts while removing the deprecated prepublish script.

Changes

Cohort / File(s) Summary
Build Configuration
package.json
Added rimraf devDependency; refactored build script to use rimraf for cross-platform dist cleanup; added prepare and prepublishOnly lifecycle scripts (both run build); removed deprecated prepublish script

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Straightforward configuration change with no logic or structural complexity
  • Single file modified with clear, cosmetic improvements (platform-independent build tooling)

Suggested reviewers

  • IgorDobryn
  • mklocek

Poem

🐰 Hops with glee, the build scripts dance,
Rimraf sweeps with graceful prance,
No more platform tears to shed,
Cross-platform builds ahead!

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Fix #82' is vague and generic, using a simple issue reference without describing the actual change being made. Provide a more descriptive title that summarizes the main change, such as 'Add build scripts for Git and npm installs' or 'Fix package build on install and publish'.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description includes all required sections (Motivation, Changes, How to test) and provides clear, actionable information aligned with the template.
Linked Issues check ✅ Passed The PR directly addresses issue #82 by implementing build scripts that ensure the dist/ directory is included when installing from Git URLs, resolving the module resolution error.
Out of Scope Changes check ✅ Passed All changes (adding rimraf, build/prepare/prepublishOnly scripts, removing prepublish) are directly related to fixing the Git install build issue and establishing a DRY build process.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-#82

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3bbee1d and 35b86cf.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (1)
  • package.json (2 hunks)
🔇 Additional comments (3)
package.json (3)

10-10: Excellent cross-platform fix addressing previous feedback.

The addition of rimraf as a devDependency resolves the Windows compatibility issue with rm -rf that was flagged in the prior review.


57-57: Build script correctly updated to use rimraf.

This change ensures the cleanup command works across Windows, macOS, and Linux, enabling Git URL installs to work for all developers.


61-62: Proper npm lifecycle hooks for the install/publish distinction.

This approach correctly replaces the deprecated prepublish hook and aligns with modern npm best practices.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@narekhovhannisyan narekhovhannisyan changed the base branch from main to fix-#84 October 30, 2025 06:34
@narekhovhannisyan narekhovhannisyan self-assigned this Oct 30, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
package.json (1)

60-61: Consider npm/yarn compatibility for broader install support.

The prepare and prepublishOnly hooks call yarn build, which requires Yarn to be globally available. Users installing from Git URLs via npm (e.g., npm install github:railsware/mailtrap-nodejs) may encounter failures if Yarn is not installed, even though your package specifies Yarn in engines.

For maximum compatibility, consider using npm run build instead, which works with both npm and Yarn:

-    "prepare": "yarn build",
-    "prepublishOnly": "yarn build",
+    "prepare": "npm run build",
+    "prepublishOnly": "npm run build",

This ensures the build runs regardless of which package manager the user prefers.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 70796f4 and 3bbee1d.

📒 Files selected for processing (3)
  • package.json (1 hunks)
  • src/types/mailtrap.ts (3 hunks)
  • src/types/transport.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/types/transport.ts (1)
src/types/mailtrap.ts (1)
  • TemplateVariables (36-36)
🔇 Additional comments (2)
src/types/mailtrap.ts (1)

29-36: Well-structured recursive type for flexible template variables.

The recursive TemplateValue type correctly supports nested objects and arrays, allowing for complex template variable structures. The type definition terminates properly with primitive base cases.

This change is backward-compatible—existing code using simple string/number/boolean values will continue to work, while new code can leverage nested structures.

src/types/transport.ts (1)

39-39: Type change verified and backward compatible.

The change from Record<string, string | number | boolean> to TemplateVariables is correct. TemplateValue is recursively defined to support primitives, arrays, and nested objects—enabling nested template variables while maintaining backward compatibility with existing flat structures. All current usages (examples, tests, adapters) work without modification.

Base automatically changed from fix-#84 to main November 4, 2025 09:01
@narekhovhannisyan narekhovhannisyan merged commit 00c7d79 into main Nov 6, 2025
4 checks passed
@narekhovhannisyan narekhovhannisyan deleted the fix-#82 branch November 6, 2025 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot find module 'mailtrap' or its corresponding type declarations.ts(2307)

4 participants