Skip to content

Conversation

@charliecreates
Copy link
Contributor

@charliecreates charliecreates bot commented Dec 9, 2025

Component / Package Name:

create-mail

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, please include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

resolves #384
refs #379

Description

Adds react-dom to the generated project’s devDependencies (matching the existing react range), and updates the create-mail CLI test to assert react/react-dom are only present in devDependencies.

Verification

# Build (mirrors CI)
$ pnpm moon run jsx-email:build
$ pnpm moon run create-mail:build
$ pnpm moon run :build --query "project~plugin-*"

# CLI tests
$ FORCE_COLOR=1 pnpm moon run test-cli:test.run
# Test Files 3 passed (3)
# Tests 3 passed (3)

# Lint
$ pnpm moon run repo:lint
# 0 errors (31 warnings)

Self-review notes:

  • reviewChanges reported changes in shared/tsconfig.base.json, pnpm-workspace.yaml, and other files not touched by this PR (confirmed by git diff origin/next/v3...HEAD); treating as false positives.
  • Kept the existing CLI output snapshots as-is; in test mode (IS_CLI_TEST=true) the scaffolder forces pnpm, so the install instructions are deterministic.

Copy link
Contributor Author

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

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

Overall change matches the PR goal, and the test coverage improvement is good. The main opportunities are making the CLI test assertions more explicit (better failure messages / less reliance on type assertions) and reducing the risk of future React version drift in the scaffold template by centralizing the version range.

Additional notes (2)
  • Maintainability | test/cli/create-mail.test.ts:24-40
    The test uses JSON.parse(... ) as PackageJson, which makes the parse result type-safe only by assertion and can hide cases where packageJson.devDependencies is undefined (leading to less clear failures) or where the JSON is malformed (which will throw, but without context). Since the intent is to validate presence in devDependencies, add explicit guards to produce clearer, targeted test failures.

  • Maintainability | packages/create-mail/generators/package.json.mustache:14-16
    The template hardcodes react/react-dom to ^19.1.0. If the scaffolder intends to keep React versions aligned with jsx-email compatibility over time, consider sourcing this version range from a single constant/template variable to avoid future drift (e.g., react bumped but react-dom forgotten, or vice versa).

Summary of changes

What changed

  • Updated the create-mail scaffolded package.json template to include react-dom in devDependencies alongside react.

    • Template: packages/create-mail/generators/package.json.mustache
    • Adds "react-dom": "^19.1.0" and adjusts the trailing {{{ typeDep }}} placement.
  • Extended the CLI integration test to assert:

    • react and react-dom exist in devDependencies
    • react and react-dom do not exist in dependencies
    • File: test/cli/create-mail.test.ts

Why this matters

  • Ensures generated projects have the expected React runtime packages available for tooling/build, while keeping them out of production dependencies as intended.

@charliecreates charliecreates bot removed the request for review from CharlieHelps December 9, 2025 23:25
Reorder imports and adjust spacing in documentation examples to match the updated create-mail scaffold conventions, keeping import groupings consistent across docs.

Refs: #384
@charliecreates
Copy link
Contributor Author

Superseded by #391 (combined PR targeting next/v3).

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.

3 participants