fix(create-mail): add react-dom to scaffold devDeps#376
fix(create-mail): add react-dom to scaffold devDeps#376charliecreates[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
The change correctly adds react-dom to the scaffold and locks the behavior in with a CLI test assertion. The only notable improvement is to avoid the blind JSON.parse(...) as PackageJson cast in the test to make failures more informative and reduce reliance on type assertions. No functional regressions are apparent from the diff.
Additional notes (1)
- Maintainability |
test/cli/create-mail.test.ts:24-40
JSON.parse(...) as PackageJsonis effectively a blind trust cast. This test only needs to read a couple of keys, so you can avoid the cast entirely (and avoid masking malformed JSON) by parsing intounknownand then narrowing minimally (or just reading properties off an object with safe defaults). This improves test robustness without adding much complexity.
Summary of changes
What changed
create-mail scaffold
- Updated the scaffolded
package.jsontemplate to includereact-domindevDependenciesalongsidereact. - Adjusted the Mustache placeholder so it now follows the
react-domline (template now emits:"react": "^19.1.0","react-dom": "^19.1.0"+{{{ typeDep }}}).
CLI test coverage
- Extended
test/cli/create-mail.test.tsto parse the generated.test/new/package.jsonand assert:reactandreact-domare present underdevDependencies- neither
reactnorreact-domappear underdependencies.
| "devDependencies": { | ||
| "react": "^19.1.0"{{{ typeDep }}} | ||
| "react": "^19.1.0", | ||
| "react-dom": "^19.1.0"{{{ typeDep }}} | ||
| } |
There was a problem hiding this comment.
The scaffold template now places {{{ typeDep }}} only on react-dom. If typeDep conditionally injects additional dev deps (e.g., @types/react/@types/react-dom) or toggles formatting, attaching it to only the second entry can be brittle: it may rely on template expansion producing valid commas/newlines in all cases. It’s safer to keep template conditionals independent of a specific dependency line (or ensure the conditional itself includes its own leading comma/newline).
Suggestion
To reduce coupling between typeDep and the react-dom line, consider moving {{{ typeDep }}} onto its own line (and ensure it includes any needed leading comma/newline), e.g.:
"devDependencies": {
"react": "^19.1.0",
"react-dom": "^19.1.0"{{{ typeDep }}}
}If typeDep does not include its own leading comma, change it to do so, or restructure like:
"react-dom": "^19.1.0"{{{ typeDep }}}(where typeDep expands to ,\n "@types/react": ... etc.).
Reply with "@CharlieHelps yes please" if you’d like me to add a commit that refactors the template to make the conditional insertion more robust.
Component / Package Name:
create-mail / test-cli
This PR contains:
Are tests included?
Breaking Changes?
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 #374
Description
Ports the
react-domscaffold fix (frommain’screate-jsx-email) intonext/v3’screate-mailgenerator, and locks it in with a CLI test assertion. No version ranges were changed.Verification
Self-review notes
test/cli/create-mail.test.ts:9-38: Left the full CLI output snapshot as-is to keep this PR scoped; the newpackage.jsonassertions cover the behavior needed for charlie: port create-jsx-email scaffold fix (react-dom) into create-mail #374.next/v3-only concerns outside this PR’s diff (e.g.test/smoke/tests/smoke.test.ts:1-29,pnpm-workspace.yaml:1-9,shared/tsconfig.base.json:1-14,test/cli/package.json:1-15,test/cli/preview-build-path.test.ts:1-51,recipes/import-css/package.json:1-20,shared/vitest.config.ts:1-12). Those files are untouched here and were left unchanged to keep this change focused on charlie: port create-jsx-email scaffold fix (react-dom) into create-mail #374.